The SYSCTL_NODE / MPSAFE / NEEDGIANT saga

[kaktus]

Summary

This is quick and dirty review for a @hselasky's idea and CTASSERT code that he presented on IRC, that it'd be nicer/easier/prettier (?) to change the SYSCTL_*_NODE macros to not to have to add ~600 SYSCTL_MPSAFE entries for SYSCTL_NODE with empty handlers but instead remove MPSAFE from existing nodes!

Although it survives compilation of LINT/amd64 as is, it doesn't (because of the |) work if the SYSCTL_*_NODE have NEEDGIANT specified…

Ideas welcome.

[• hselasky]

Looks very good. See my two comments.

sys/sys/sysctl.h

327

Ditto here.

360

Ditto here.

[kib]

Can you explain the changes without referring to IRC conversation ?
In particular, if there is a node with non-NULL handler that requires Giant around it, what happens ?

[kaktus]

For documentation only, because committing this breaks the other patches and this review was created for brainstorming, but the whole discussion moved to IRC.

SYSCTL_NODE (and friends) is used to create branches, in most cases it's used without handler what implies that the node is MPSAFE by definition - there is nothing to call, so no locking is needed. This is the case for about 90% of in-tree SYSCTL_NODEs.

If the macro is supplied with a function pointer to a handler, the whole construct behaves similarly to SYSCTL_PROC of type CTLTYPE_NODE, and when the handler function is called in sysctl_root_handler_locked() if it's not marked as MPSAFE it's called under Giant.

Now, the question is: should SYSCTL_NODEs be allowed to specify handler or should all of the use cases be converted to SYSCTL_PROC with CTLTYPE_NODE to make it more explicit in what is really happening. That would require change in the SYSCTL_NODE macros to remove the handler pointer apart of converting current consumers.
Currently, the man page for sysctl(9) mentions that CTLFLAG_MPSAFE should be used with SYSCTL_PROC only, but in reality if the SYSCTL_NODE have non-NULL handler the flag should also be used there.

[kib]

Ok, consider it from the following angle: does doing the sweep for CTLTYPE_NODE->CTLTYPE_PROC improves the code or just allows to avoid adding a flag, and then removing it ?

We already have a buy-in for large disruption while flags will be added and then removed. Adding more different work without ultimate improvements for the code does not make sense.

[kaktus]

The amount of work to change the SYSCTL_NODEs with handlers into SYSCTL_PROCs and remove the handler from SYSCTL_NODEs will cause a lot more code changes.
Yes it'll limit the amount of CTLFLAG_MPSAFE being added (and later removed) to SYSCTL_NODEs without handlers.
But the question of should SYSCTL_NODEs be allowed to have handlers is a very different one, and one may suggest it's a bug or just a rarely used feature.
I would want to concentrate on the current approach: mark SYSCTL_NODES without handlers as MPSAFE, nad masrk SYSCTL_NODES and SYSCT_PROCS with handlers that are not marked as MPSAFE now as NEEDGIANT and continue the work pushing the Giant off the sysctl. Fixing semantics of SYSCTL_NODE can be done later and has nothing to do with the MPSAFE/NEEDGIANT work.