Page MenuHomeFreeBSD

mpi3mr: Reduce the scope of the reset_mutext
ClosedPublic

Authored by imp on Nov 10 2023, 12:19 AM.
Tags
None
Referenced Files
F102689512: D42539.diff
Fri, Nov 15, 9:53 PM
Unknown Object (File)
Sep 28 2024, 9:40 AM
Unknown Object (File)
Sep 24 2024, 3:17 AM
Unknown Object (File)
Sep 14 2024, 4:21 AM
Unknown Object (File)
Sep 8 2024, 3:27 PM
Unknown Object (File)
Sep 5 2024, 10:44 AM
Unknown Object (File)
Aug 30 2024, 12:28 PM
Unknown Object (File)
Aug 29 2024, 3:45 AM
Subscribers
None

Details

Summary

Reduce the scope of reset_mutext to protect the msleep in the watch dog
thread as well as the MPI3MR_FLAGS_SHUTDOWN bit. Use it to protect the
wakeup in mpi3mr_detach so this thread can exit sooner when we're trying
to do an orderly shutdown.

It's an open question if this should protect sc->unrecoverable, and if
we should wakeup the watchdog thread when we set it. We might also want
to move too booleans for the three flags that we have now in
mpi3mr_flags. There are a number of U8s that should really be bools and we
might want to also group them together to pack softc better.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 54463
Build 51353: arc lint + arc unit

Event Timeline

imp requested review of this revision.Nov 10 2023, 12:19 AM
imp created this revision.

The lock seems should protect not only msleep(), that is indeed pointless, but also both MPI3MR_FLAGS_SHUTDOWN accesses and also wakeup(&sc->watchdog_chan) in mpi3mr_pci_detach(). Otherwise the thread may not exit until the timeout expiration, that is a waste of time.

In D42539#972011, @mav wrote:

The lock seems should protect not only msleep(), that is indeed pointless, but also both MPI3MR_FLAGS_SHUTDOWN accesses and also wakeup(&sc->watchdog_chan) in mpi3mr_pci_detach(). Otherwise the thread may not exit until the timeout expiration, that is a waste of time.

I agree. I hadn't had the time to chase this issue to ground.

Do we want to do a wakeup when unrecoverable is set to 1 too? I'm thinking no because the watchdog thread exiting quickly doesn't buy us much. But if so, then we should lock around setting it and issue a wakeup. It's done so many places, an inline function likely should be written.... I don't understand how things wind down once this is set enough to know if there's a benefit or not.

imp edited the summary of this revision. (Show Details)

implement mav's idea about wakeup

sys/dev/mpi3mr/mpi3mr.c
3040–3041

We should check the same condition as below before the msleep(), so we would not enter the sleep while shutdown is already requested.

This version still may go to sleep with MPI3MR_FLAGS_SHUTDOWN flag set, that is suboptimal.

this time with the right change and to the right change.

This revision is now accepted and ready to land.Nov 15 2023, 10:26 PM
This revision was automatically updated to reflect the committed changes.