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

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

Details

Reviewers

kib
royger

Commits

rS358316: Mark more nodes as CTLFLAG_MPSAFE or CTLFLAG_NEEDGIANT (16 of many)

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

Tests Skipped

Event Timeline

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

Herald added a subscriber: asomers. · View Herald TranscriptFeb 11 2020, 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.

kaktus mentioned this in D23110: Introduce CTLFLAG_NEEDGIANT.Feb 16 2020, 5:18 PM

kaktus added inline comments.Feb 20 2020, 1:33 PM

sys/dev/xen/blkfront/blkfront.c
931	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 :-)