Page MenuHomeFreeBSD

Needs ReviewPublic

Authored by kaktus on Thu, Feb 13, 10:26 PM.



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.

Diff Detail

Lint Skipped
Unit Tests Skipped

Event Timeline

kaktus created this revision.Thu, Feb 13, 10:26 PM
kaktus edited the summary of this revision. (Show Details)Thu, Feb 13, 10:32 PM
kaktus removed a reviewer: transport.

Looks very good. See my two comments.


Ditto here.


Ditto here.

kib added a comment.Fri, Feb 14, 4:11 PM

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 ?

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 added a comment.Fri, Feb 14, 7:28 PM

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.

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.