Page MenuHomeFreeBSD
Differential D42411

cam: Make cam_debug macros atomic
ClosedPublic
Actions

Authored by imp on Oct 31 2023, 3:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 15, 11:54 PM
Unknown Object (File)
Wed, Apr 15, 11:40 PM
Unknown Object (File)
Mon, Apr 13, 11:49 AM
Unknown Object (File)
Thu, Apr 9, 6:02 PM
Unknown Object (File)
Tue, Apr 7, 3:22 PM
Unknown Object (File)
Tue, Apr 7, 8:30 AM
Unknown Object (File)
Tue, Apr 7, 1:06 AM
Unknown Object (File)
Mon, Apr 6, 11:27 PM
View All Files
Subscribers
None

Details

Reviewers
ken
mav
jhb
Group Reviewers
cam
Commits
rG70f2356d364f: cam: Make cam_debug macros atomic
Summary

The CAM_DEBUG* macros use multiple printfs to dump the data. This is
suboptimal when tracing things that produce even a moderate amount since
it gets intertwingled. I can't even turn on tracing with a 24-disk HBA
on boot without it getting messed up. Add helper routines to work around
clang's over-use of the stack: that way we only pay the stack penalty
when a trace hits.

Sponsored by: Netflix

Diff Detail

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

Event Timeline

imp created this revision.Oct 31 2023, 3:56 AM
Herald added a reviewer: cam. · View Herald TranscriptOct 31 2023, 3:56 AM
imp requested review of this revision.Oct 31 2023, 3:56 AM
Harbormaster completed remote builds in B54217: Diff 129538.Oct 31 2023, 3:56 AM
imp added reviewers: ken, mav, jhb.Oct 31 2023, 3:57 AM
imp updated this revision to Diff 129539.Oct 31 2023, 4:07 AM

(a) instead of a in arg expansions (except printfargs)

Harbormaster completed remote builds in B54218: Diff 129539.Oct 31 2023, 4:07 AM
ken accepted this revision.Oct 31 2023, 1:36 PM

Good idea. Sbuf didn't exist back when the debug macros were written, this is an improvement.

This revision is now accepted and ready to land.Oct 31 2023, 1:36 PM
mav added inline comments.Oct 31 2023, 2:22 PM
sys/cam/cam_debug.h
88

Won't this eat 100 bytes of stack for each use? Since these are maros, not functions, does compiler free the stack once it get out of scope?

imp added inline comments.Oct 31 2023, 2:28 PM
sys/cam/cam_debug.h
88

Yes. The stack doesn't appear to be used unless the scope is entered. And is freed when the scope exits (at least in simple tests I've written). Would you like me to confirm?

mav added inline comments.Oct 31 2023, 2:31 PM
sys/cam/cam_debug.h
88

I have feeling that I saw opposite long ago, but may be it has changed or I misunderstood it back then. Would be good to confirm.

imp updated this revision to Diff 129557.Oct 31 2023, 6:00 PM

update per review

This revision now requires review to proceed.Oct 31 2023, 6:00 PM
Harbormaster completed remote builds in B54223: Diff 129557.Oct 31 2023, 6:00 PM
imp edited the summary of this revision. (Show Details)Oct 31 2023, 6:01 PM
imp added inline comments.
sys/cam/cam_debug.h
88

Further tests, on the actual code, show that clang, at least, moves the stack the amount of the sum of the scopes in-scope at once. This behaves like you say when we have char buffers like this... the tests I did wound up being bogus because clang completely eliminated the inner scope (doh! Stupid clang getting so smart). I've moved to helper routines to get around this issue.

121

This is unused, so I just removed it.

imp added inline comments.Oct 31 2023, 6:04 PM
sys/cam/cam_xpt.c
5547

The observant may discover these are close, but not quite the same as, other routines. Some refactoring (and moving the delay back into the macro) wouldn't be a bad thing, but is beyond the scope for this thing.

mav accepted this revision.Oct 31 2023, 6:18 PM
mav added inline comments.
sys/cam/cam_xpt.c
3716

Looking on xpt_path_sbuf() I wonder if this should be something like xpt_device_sbuf() with xpt_print_device() as a wrapper, if needed.

This revision is now accepted and ready to land.Oct 31 2023, 6:18 PM
imp added inline comments.Oct 31 2023, 6:28 PM
sys/cam/cam_xpt.c
3716

I think we can just rename it because the only use of it was in these debugging macros.
Good suggestion.

imp updated this revision to Diff 129558.Oct 31 2023, 6:29 PM

rename to xpt_device_sbuf

This revision now requires review to proceed.Oct 31 2023, 6:29 PM
Harbormaster completed remote builds in B54224: Diff 129558.Oct 31 2023, 6:29 PM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 3 2023, 4:18 PM
Closed by commit rG70f2356d364f: cam: Make cam_debug macros atomic (authored by imp). · Explain Why
This revision was automatically updated to reflect the committed changes.
imp added a commit: rG70f2356d364f: cam: Make cam_debug macros atomic.

Revision Contents
Changeset List

PathSize
sys/
cam/
67 lines
4 lines
75 lines

Diff 129558

View Options

sys/cam/cam_debug.h

Loading...
View Options

sys/cam/cam_xpt.h

Loading...
View Options

sys/cam/cam_xpt.c

Loading...