Page MenuHomeFreeBSD

Make device_busy/unbusy work w/o Giant held
ClosedPublic

Authored by imp on Sep 1 2020, 5:22 PM.

Details

Summary

The vast majority of the busy/unbusy users in the tree don't acquire Giant
before calling device_busy/unbusy. However, if multiple threads are opening a
file, say, that causes the device to busy/unbusy, then we can race to the root
marking things busy. Create a new device_busy_locked and device_unbusy_locked
that are the current implemntations of device_busy and device_unbusy. Make
device_busy and unbusy acquire Giant before calling the _locked versrions. Since
we never sleep in the busy/unbusy path, Giant's single threaded semantics
suffice to keep this safe.

Diff Detail

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

Event Timeline

imp requested review of this revision.Sep 1 2020, 5:22 PM
imp created this revision.
hselasky added a subscriber: hselasky.
hselasky added inline comments.
sys/kern/subr_bus.c
144

Use uint32_t as below?

This revision is now accepted and ready to land.Sep 2 2020, 3:38 PM

I'd rather axe device_busy and instead require drivers to use device_quiesce.

However, this does mean that bus drivers need to use a bus_generic_quiesce that recurses to preserve the "device_busy of the parent" semantic.

In D26284#584505, @jhb wrote:

I'd rather axe device_busy and instead require drivers to use device_quiesce.

However, this does mean that bus drivers need to use a bus_generic_quiesce that recurses to preserve the "device_busy of the parent" semantic.

Good point. I'll rework to eliminate this where it's now not needed and see if we can migrate the rest to quiesce.

Rebase and take out the fdc tweaks. Those aren't needed anymore.

This revision now requires review to proceed.Sep 4 2021, 4:34 AM

There is a spelling mistake in the description:

versrions -> versions

sys/kern/subr_bus.c
3034

Shouldn't you use a refcount function to read the busy value? I mean atomic_xxx

sys/dev/gpio/gpiopps.c
109

Note that this only sort of works, and that really one has to use cdevpriv(9) to be fully safe here (you would do the device_unbusy in the cdevpriv dtor). There are some edge cases when open() is not paired with close even in the D_TRACKCLOSE case.

sys/kern/subr_bus.c
2654
3034

It is cleaner (but functionally identical) to use atomic_load_int() here (not sure if we have a refcount_* wrapper for that)

sys/dev/gpio/gpiopps.c
109

There's half a dozen or so drivers in the tree that use a similar trick. I'd like to leave this in this commit and address those in a separate commit.

This revision is now accepted and ready to land.Nov 30 2021, 6:37 PM
This revision was automatically updated to reflect the committed changes.