Page MenuHomeFreeBSD

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

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

Details

Reviewers
kib
royger
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

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kaktus created this revision.Tue, Feb 11, 9:14 PM

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

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

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.