From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: sysctl: unlock KERN_CONSBUF and KERN_MSGBUF
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: tech@openbsd.org
Date: Thu, 8 May 2025 08:41:36 +0300

On Thu, May 08, 2025 at 03:08:44PM +0200, Alexander Bluhm wrote:
> On Tue, May 06, 2025 at 08:46:11PM +0300, Vitaliy Makkoveev wrote:
> > > > +
> > > > +		memset(&ump, 0, sizeof(ump));
> > > > +		ump.msg_magic = mp->msg_magic;
> > > > +		ump.msg_bufs = mp->msg_bufs;
> > > > +		mtx_enter(&log_mtx);
> > > > +		ump.msg_bufx = mp->msg_bufx;
> > > > +		ump.msg_bufr = mp->msg_bufr;
> > > > +		ump.msg_bufd = mp->msg_bufd;
> > > > +		mtx_leave(&log_mtx);
> > > 
> > > Could you put the memset() and all assignments into the mutex block?
> > > Then the compiler could optmize away the memset of fields that are
> > > set immediately afterwards.  Although our clang is dumb and does
> > > double stores, maybe other compiler behave smarter.
> > > 
> > 
> > I doubt the old gcc is such smart :) The whole `ump' filling under mutex
> > could make sense, because it makes code easier to read, but putting
> > memset() of local data makes code worse.
> > 
> > Also, I doubt that filling of solid memory chunk is slower that filling
> > memory chunk with holes.
> 
> I was considering memset() and mutex again.  Although clang does
> not optimize away the memset stores, the processor still could to
> this.  But putting a mutex barrier between clearing and filling the
> memory of struct ump prevents all optimization.
> 
> Of course sysctl(2) is not the hot path, it does not matter for
> speed, and I have no benchmarks.  It is more about MP programming
> in general.
> 
> What do you think?
> 

I don't know. In my point of view, the optimization very depends on
memset() implementation and how the compiler unrolls it. I doubt there
should be the differences with the atomic_load_*() family.

I'm curious on opinion of person who understand this area better.

> bluhm
> 
> Index: kern/kern_sysctl.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_sysctl.c,v
> diff -u -p -r1.467 kern_sysctl.c
> --- kern/kern_sysctl.c	6 May 2025 18:34:26 -0000	1.467
> +++ kern/kern_sysctl.c	8 May 2025 12:58:42 -0000
> @@ -598,8 +598,8 @@ kern_sysctl(int *name, u_int namelen, vo
>  		} else 
>  			return (0);
>  
> -		memset(&ump, 0, sizeof(ump));
>  		mtx_enter(&log_mtx);
> +		memset(&ump, 0, sizeof(ump));
>  		ump.msg_magic = mp->msg_magic;
>  		ump.msg_bufs = mp->msg_bufs;
>  		ump.msg_bufx = mp->msg_bufx;