Page MenuHomeFreeBSD

Mark more nodes as CTLFLAG_MPSAFE or CTLFLAG_NEEDGIANT (M of N)
ClosedPublic

Authored by kaktus on Feb 11 2020, 9:14 PM.

Details

Summary

r357614 added CTLFLAG_NEEDGIANT to make it easier to find nodes that are still not MPSAFE (or already are but aren’t properly marked). Use it in preparation for a general review of all nodes.
This is non-functional change that adds annotations to SYSCTL_NODE and SYSCTL_PROC nodes using one of the soon-to-be-required flags.

You’re asked for a review based on src/MAINTAINERS entry.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Thanks, I have a couple of comments about the need for GIANT on some of the sysctls. I have to admit I'm not very familiar with sysctls, so I might be missing something.

sys/dev/xen/blkfront/blkfront.c
931 ↗(On Diff #68150)

I think this should be marked as MPSAFE?

The handler doesn't use any global variables apart from the device softc, and only reads from it.

Or is this done to prevent the device from being removed while the sysctl handler is running?

(It's possible I'm missing something, as I'm no expert on sysctl).

sys/xen/xenbus/xenbusb.c
398 ↗(On Diff #68150)

I think all the sysctls using xenbusb_device_sysctl_handler are MP safe, as they use __BUS_ACCESSOR under the hood and don't write to any of the fields of the device? (again I might be missing something).

Marking them as NEEDGIANT is default to document the current state - all handlers not marked as MPSAFE are implicitly run under Giant. The new flag makes that a bit more explicit and allows for reviews like this to actually assess if that's really needed.
I'll take a look if we have any precedent on how to deal with sysctls operating on devices that can disappear. All other can probably by marked as MPSAFE, I'll update the patch.

sys/dev/xen/blkfront/blkfront.c
931 ↗(On Diff #68150)

I believe that softc should be locked here as this is similar to other parts of the tree. I can prepare other patch to fix this as I'd like to have the NEEDGIANT parts committed sooner than later :-)

This revision is now accepted and ready to land.Feb 24 2020, 6:34 PM