Page MenuHomeFreeBSD

Make dumps safe while the scheduler is running
Needs ReviewPublic

Authored by badger on Nov 4 2016, 8:44 PM.

Details

Summary

A dump may be produced without stopping the scheduler via "reboot -d".
In such a case, it's neither safe nor necessary to call sim_poll.
Additionally, a ccb must not be freed/reused until fully through the CAM
pipeline. When the scheduler is stopped, this is guaranteed by the fact
that only one thread operates on the ccb. To handle the case when other
threads may be running, add a flag to indicate that it is safe to
free/reuse the ccb memory.

Test Plan

Test dumps with "reboot -d" as well as panic induced dumps. Ensure a good dump
is produced and that no crashes occur after many iterations. Try a couple of different SIMs.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6283
Build 6523: arc lint + arc unit

Event Timeline

badger retitled this revision from to Make dumps safe while the scheduler is running.Nov 4 2016, 8:44 PM
badger updated this object.
badger edited the test plan for this revision. (Show Details)
badger updated this revision to Diff 22007.
badger edited the test plan for this revision. (Show Details)Nov 5 2016, 9:27 PM
badger added reviewers: imp, kib.
kib added a reviewer: mav.Nov 6 2016, 2:40 PM
dab added a subscriber: dab.Nov 7 2016, 5:12 PM

Giving this review a gentle prod in case anyone willing to review might have forgotten about it. I'd also appreciate any suggestions of reviewers if folks in the current set don't have time to review or feel it's outside their area. Thanks.

dab added a reviewer: dab.Nov 22 2016, 5:35 PM
dab accepted this revision.
This revision is now accepted and ready to land.Nov 22 2016, 5:35 PM
mav edited edge metadata.Nov 23 2016, 6:00 AM
mav requested changes to this revision.

While the proposed patch may work, I can't say I like it for multiple reasons:

  • from my experience kernel dumping without scheduler stopped is a way to obtain inconsistent dump;
  • if scheduler is not stopped, there is no need to use not only xpt_polled_action(), but the polling at all -- just put calling thread to sleep and wake it when completion callback fired. There is cam_periph_runccb() present for many years for CCB waiting in sleepable context. If context is not sleepable, then then tight spinning for CCB completion may not work by definition on UP systems, since kernel threads (including interrupt threads) may not preempt each other, and so CCB completion may never be processed without polling.
This revision now requires changes to proceed.Nov 23 2016, 6:00 AM

Thanks mav. Originally, I'd thought stopping the scheduler when using 'reboot -d' was the correct way. kib and I discussed a bit though, and I was convinced to look for another option ( https://lists.freebsd.org/pipermail/freebsd-hackers/2016-November/050108.html ). Your first bullet would seem to suggest that stopping the scheduler is necessary.

Perhaps stopping the scheduler on 'reboot -d' should be made a tunable? If a case arises where a dump can't be obtained, that tunable can be twiddled to return to today's behavior.

emaste added a subscriber: emaste.Dec 1 2016, 10:21 PM
badger edited edge metadata.Dec 17 2016, 12:21 AM
badger updated this revision to Diff 23020.

Optionally stop scheduler when using "reboot -d". I considered a couple of
other options:

  1. Changing dumpers (e.g. adadump()) to have a mode that uses

cam_periph_runccb() and friends when the scheduler is not stopped. But there
are many places to make such a change (many of which I know little about and
can't test). It might be that dumpers that support dumping while running could
use this new mode, while others would use the existing dump code (perhaps
stopping the scheduler before doing do), but this all seems overly complicated.

  1. Writing a generic "dump core while running" function that flows through

geom, but this would mean dumping earlier in reboot (before shutting off geom
services), which seems more likely to produce an inconsistent dump.

Stopping the scheduler gives roughly the same behavior as a panic, while the
tunable allows someone who has an issue here to skip this step during a dump.

vangyzen accepted this revision.
dab edited edge metadata.Jan 3 2017, 1:28 PM
dab accepted this revision.