On Tue, May 16, 2023 at 08:26:37PM +0300, Vitaliy Makkoveev wrote: > > On 16 May 2023, at 18:35, Alexander Bluhm <alexander.bl...@gmx.net> wrote: > > > > I saw one issue in sysctl_niq(). Another CPU could write mq_maxlen > > and our logic is inconsistent. Below is a fix with read once. Each > > CPU detects its own change, last write wins. Or should we protect > > everything with mq_mtx? Then sysctl 123 -> 345 would have the > > correct old value on both CPUs. > > I don’t assume this case as issue, since if we have two concurrent > sysctl calls modifying the same mib we can’t predict which value will > be set last. To me, the "maxlen != mq->mq_maxlen” check could be > avoided and the "!error” or "error == 0” check is pretty enough: > > error = sysctl_int(oldp, oldlenp, newp, newlen, &maxlen); > if (!error) > mq_set_maxlen(mq, maxlen); > > If you want to keep the check, I prefer to protect everything with > mq_mtx, and push the check within mq_set_maxlen(). At least this is > clean and strictly predictable comparing with > oldmax = newmax = READ_ONCE(). >
We have "error == 0" in assertion, so I used this idiom instead of "!error". This is not the fast path, so dropping "maxlen != mq->mq_maxlen" doesn't provide any performance impact. ok? Index: sys/kern/uipc_mbuf.c =================================================================== RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v retrieving revision 1.285 diff -u -p -r1.285 uipc_mbuf.c --- sys/kern/uipc_mbuf.c 5 May 2023 01:19:51 -0000 1.285 +++ sys/kern/uipc_mbuf.c 16 May 2023 19:34:28 -0000 @@ -1801,7 +1801,7 @@ sysctl_mq(int *name, u_int namelen, void case IFQCTL_MAXLEN: maxlen = mq->mq_maxlen; error = sysctl_int(oldp, oldlenp, newp, newlen, &maxlen); - if (!error && maxlen != mq->mq_maxlen) + if (error == 0) mq_set_maxlen(mq, maxlen); return (error); case IFQCTL_DROPS: