o Giant held
ClosedPublic
Actions

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

Details

Reviewers

jhb
• hselasky

Commits

rG1c7d15b03071: Make device_busy/unbusy work w/o Giant held
rG08e781915363: Make device_busy/unbusy work w/o Giant held

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.

Harbormaster completed remote builds in B33306: Diff 76505.Sep 1 2020, 5:22 PM

imp added a reviewer: jhb.Sep 1 2020, 5:24 PM

• hselasky accepted this revision.Sep 2 2020, 3:38 PM

• 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.

kevans added a subscriber: kevans.Sep 2 2020, 5:29 PM

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

Harbormaster completed remote builds in B41362: Diff 94647.Sep 4 2021, 4:34 AM

rebase

Harbormaster completed remote builds in B41364: Diff 94649.Sep 4 2021, 4:52 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

jhb added inline comments.Nov 29 2021, 6:03 PM

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)

jhb added a child revision: D31831: Create wrapper for Giant taken for newbus.Nov 29 2021, 6:17 PM

imp added inline comments.Nov 30 2021, 7:53 AM

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.