From: Alexander Bluhm Subject: Re: sysctl: unlock KERN_CONSBUF and KERN_MSGBUF To: Vitaliy Makkoveev Cc: tech@openbsd.org Date: Thu, 8 May 2025 15:08:44 +0200 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;