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?

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;