On Tue, May 16, 2023 at 08:26:37PM +0300, Vitaliy Makkoveev wrote:
> > On 16 May 2023, at 18:35, Alexander Bluhm <[email protected]> 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:
