Remove Giant from init creation and vfs_mountroot.

[imp]

Test Plan

Test boots for UFS for me.

[kib]

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.

[imp]

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

[imp]

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.

[imp]

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

[kib]

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

Is this intended change ? It is not clear.

sys/sparc64/sbus/sbus.c

414

Are these interrupt handlers call to pyscho_XXX functions which you modified ?

[imp]

sys/arm/at91/at91_rst.c

127

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

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]

sys/sparc64/sbus/sbus.c

414

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]

sys/sparc64/sbus/sbus.c

414

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]

sys/sparc64/sbus/sbus.c

414

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().

[marius]

sys/sparc64/sbus/sbus.c

414

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]

sys/sparc64/sbus/sbus.c

414

If they are MPSAFE, I'll just do that instead. Thats' easy.

414

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.

[imp]

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

[imp]

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

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]

sys/sparc64/sbus/sbus.c

414

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

[mckusick]

This all looks good to me. Glad to see more of Giant retired.

[imp]

In D14712#309639, @mckusick wrote:

This all looks good to me. Glad to see more of Giant retired.

I'd love to finally kill it, but that will be having some kind of topology lock in newbus after all these years... :)

[emaste]

Tested successfully in my working tree

[emaste]

This comment was removed by emaste.

[imp]

Committed

[emaste]

Was committed in rS331294