Page MenuHomeFreeBSD

Make device_busy/unbusy work w/o Giant held
ClosedPublic

Authored by imp on Sep 1 2020, 5:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 30, 3:11 AM
Unknown Object (File)
Thu, Mar 21, 12:59 PM
Unknown Object (File)
Mar 7 2024, 1:33 PM
Unknown Object (File)
Feb 26 2024, 6:29 AM
Unknown Object (File)
Feb 17 2024, 4:34 AM
Unknown Object (File)
Feb 17 2024, 4:34 AM
Unknown Object (File)
Feb 17 2024, 4:34 AM
Unknown Object (File)
Feb 17 2024, 4:34 AM

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
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 41362
Build 38251: arc lint + arc unit

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
3039

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

sys/dev/gpio/gpiopps.c
110

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
2674
3039

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
110

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.