Remove Giant from init creation and vfs_mountroot.
ClosedPublic
Actions

Authored by imp on Mar 16 2018, 8:51 PM.

Details

Reviewers

kib
mckusick
marius

Test Plan

Test boots for UFS for me.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 15603
Build 15639: arc lint + arc unit

Event Timeline

imp created this revision.Mar 16 2018, 8:51 PM

Harbormaster completed remote builds in B15603: Diff 40359.Mar 16 2018, 8:51 PM

emaste added a subscriber: emaste.Mar 16 2018, 9:01 PM

imp edited the test plan for this revision. (Show Details)Mar 16 2018, 9:14 PM

imp added reviewers: kib, mckusick.

There is one more user of vfs_mountroot(), from kern_reroot(). We have Giant locked around kern_reboot() call in kern_shutdown.c. I believe I cleaned geom from useless asserts that Giant is not owned, so may be the leftover is innocent.

Or may be, try to remove Giant from kern_shutdown.c as well. I suspect that other caller of kern_reboot(), namely shutdown_nice(), is not locked consistently. E.g. it seems that vt(4) does not lock Giant when calls it.

audit callers of shutdown_now

Remove Giant from init creation and vfs_mountroot.
These interrupts call shutdown_nice() which should be called Giant
This is MPSAFE on this platform, so don't take Giant out while running
Unlock giant when calling shutdown_nice()
fold
Mark this as MPSAFE -- unsure
Drop giant before calling shutdown_nice. Not sure if these are called

Harbormaster completed remote builds in B15605: Diff 40364.Mar 16 2018, 10:12 PM

In D14712#309256, @kib wrote:

There is one more user of vfs_mountroot(), from kern_reroot(). We have Giant locked around kern_reboot() call in kern_shutdown.c. I believe I cleaned geom from useless asserts that Giant is not owned, so may be the leftover is innocent.

Or may be, try to remove Giant from kern_shutdown.c as well. I suspect that other caller of kern_reboot(), namely shutdown_nice(), is not locked consistently. E.g. it seems that vt(4) does not lock Giant when calls it.

I believe that I've cleaned up the whole mess, but I'm a little uneasy about psycho.c, the rest I'm confident on, assuming that my understanding of DROP_GIANT() will drop, if held, Giant, otherwise it won't matter that Giant is already unlocked.

emaste mentioned this in D11866: Allow vt_timer() callout to run during mountroot input loop in vt(4).Mar 16 2018, 10:19 PM

Another round of careful preening of when / where we need to drop
Giant. It appears we don't need to drop in vt at all. Better fix for
psycho since I don't think those interrupts can be marked MPSAFE (at
least I'm unsure, so err on the side of caution).

Remove Giant from init creation and vfs_mountroot.
bufshutdown is no longer called with Giant held, so there's no need to
These interrupts call shutdown_nice() which should be called Giant
This is MPSAFE on this platform, so don't take Giant out while running
Unlock giant when calling shutdown_nice()
It's unclear if these interrupts can be marked MPSAFE, so drop Giant

Harbormaster completed remote builds in B15611: Diff 40375.Mar 17 2018, 5:14 AM

kib mentioned this in D14715: Switch mount.conf(8) support to use functions in md(4)..Mar 17 2018, 12:22 PM

I think that you are right in that your patch explicitly drop Giant before calling in shutdown_nice(), because shutdown uses delay and this might not drop GIant automatically.

For the sparc64 changes, I suggest to ask Marius.

sys/arm/at91/at91_rst.c
127 ↗	(On Diff #40375)	Is this intended change ? It is not clear.
sys/sparc64/sbus/sbus.c
414 ↗	(On Diff #40375)	Are these interrupt handlers call to pyscho_XXX functions which you modified ?

This revision is now accepted and ready to land.Mar 17 2018, 12:28 PM

kib added a reviewer: marius.Mar 17 2018, 12:29 PM

imp added inline comments.Mar 17 2018, 3:26 PM

sys/arm/at91/at91_rst.c
127 ↗	(On Diff #40375)	Yes. The callout_init function takes an mpsafe arg. We don't need giant to protect the timer routine, I don't think. It's intended, but I'll double check.
sys/sparc64/sbus/sbus.c
414 ↗	(On Diff #40375)	No. These are the handlers to the sbus interrupt routines that do something similar. Here I could just make the interrupts MPSAFE because they are (now) MPSAFE since all they do is call shutdown_nice()

marius added inline comments.Mar 17 2018, 3:38 PM

sys/sparc64/sbus/sbus.c
414 ↗	(On Diff #40375)	No, these bits set up sbus_overtemp() and sbus_pwrfail() as interrupt handlers. I once did something similar with the power fail and overtemperature interrupt handlers of psycho(4) and sbus(4) as part of r247601 which was changing them into interrupt filters (which are implicitly INTR_MPSAFE). As it turned out, the other code that I looked at and which called shutdown_nice(9) from filters was was wrong at that time as shutdown_nice(9) in fact acquired a sleep lock and kern_reboot(9) seemed to still require Giant to be hold. Thus, I had to revert corresponding parts of r247601 in r247621 again. With the patch of D14712 in place, is it safe to just call shutdown_nice(9) from a filter? If yes, the preferable approach is to revert r247621 (modulo the s,FILTER_HANDLED,RB_POWEROFF, part that is).

kib added inline comments.Mar 17 2018, 3:58 PM

sys/sparc64/sbus/sbus.c
414 ↗	(On Diff #40375)	Calling something from the interrupt filter is almost same as calling the same code while owning a spinlock. Since shutdown_nice() does many top-half operations like locking a sleepable mutexes and sending signals, it cannot be called from the filter.

marius requested changes to this revision.Mar 17 2018, 4:10 PM

marius added inline comments.

sys/sparc64/sbus/sbus.c
414 ↗	(On Diff #40375)	Okay, than it still would be preferable to modify psycho_set_intr() to set up the filters and handlers as INTR_MPSAFE instead of doing the mtx_unlock(&Giant) kludges in psycho_overtemp() and psycho_powerdown().

This revision now requires changes to proceed.Mar 17 2018, 4:10 PM

marius added inline comments.Mar 17 2018, 5:39 PM

sys/sparc64/sbus/sbus.c
414 ↗	(On Diff #40375)	That also means that there still is incorrect code in the tree, i. e. filters that call shutdow_nice(9), for example rt305x_gpio_intr() (I didn't do an exhaustive search, though).

imp added inline comments.Mar 17 2018, 5:43 PM

sys/sparc64/sbus/sbus.c
414 ↗	(On Diff #40375)	If they are MPSAFE, I'll just do that instead. Thats' easy.
414 ↗	(On Diff #40375)	Likely correct. When I looked at all the shutdown_nice() usages, I wasn't looking for call from filter, just MPSAFE or not. I don't recall seeing this one, so I'll do another search of the tree for that.

Update pyscho drivers per Marius.

Remove Giant from init creation and vfs_mountroot.
bufshutdown is no longer called with Giant held, so there's no need to
These interrupts call shutdown_nice() which should be called Giant
This is MPSAFE on this platform, so don't take Giant out while running
Unlock giant when calling shutdown_nice()
Mark psycho interrupts as MPSAFE. It's safe to do so now that we don't

Harbormaster completed remote builds in B15619: Diff 40386.Mar 17 2018, 5:49 PM

marius accepted this revision.Mar 17 2018, 5:51 PM

This revision is now accepted and ready to land.Mar 17 2018, 5:51 PM

Updated, per the review, but haven't gone back and re-checked to see if any of these routines are in a filter. I Think when I checked them, they were all MPSAFE, but I'll admit I wasn't looking for filters at the time.

sys/sparc64/sbus/sbus.c
414 ↗	(On Diff #40375)	There is no rt305x_gpio_intr in the tree that I can find. It was from the obsolete port to the mediatek stuff that was deleted in favor of sys/mips/mediatek.

marius added inline comments.Mar 17 2018, 6:13 PM

sys/sparc64/sbus/sbus.c
414 ↗	(On Diff #40375)	Ah, this then means that the head tree of fxr.watson.org currently is out of date :) http://fxr.watson.org/fxr/ident?i=rt305x_gpio_intr