HWPMC CPU traces show massive CPU time on wakeups under a SPEC SFS 08
workload. Reduce CPU spent on such useless wakeups by only attempting
them when needed.
Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 5377 Build 5573: CI src build Jenkins Build 5572: arc lint + arc unit
Event Timeline
sys/sys/buf.h | ||
---|---|---|
202 | I would make the description describe what it does, not what it prevents, so something like "Waiters active in bwait()" or some such. |
This should be fine, assuming you can provide convincing arguments that modifying b_flags in bwait() is safe.
b_flags seems to be protected by mtxpool lock. The same protection is used to modify b_flags in bdone().
From buf.h:
All fields are protected by the buffer lock except those marked (V), (Q), (D).
sys/sys/buf.h | ||
---|---|---|
202 | Ok. |
Actually bwait() is used in bufwait() and generally with KERNPROC held. Even if this is not enforced by asserts (just like bdone() is not asserting the fact that it can only run in b_iodone), it is always true.
In order to modify b_flags you need the buffer lock and while it is KERNPROC nothing else can acquire it.
So exactly how do you expect that b_flags can be modified?
KERNPROC does not mean free lock, but lock held by someone which is not curthread.
LK_KERNPROC (and in general b_lock) makes it safe to touch it.
mtx_pool makes it safe against bdone().
We can assert b_lock is held.
In fact, I think, with relatively high confidence, that the patch is fine. The main point is that bwait() is performed by threads which are not allowed to modify buffer until B_DONE is observed. Then, as was noted, mtxpool provides the exclusion between g_up thread (executing bdone()) and whatever threads happen to do bwait().
To make above hand-waving into the proof all callers of bwait() must be inspected and confirmed that my statement is true. I would expect the patch author to complete the review of the callers.
However, here b_flags is being written while B_DONE is not set. That won't be atomic with any I/O threads doing work on the buffer. For instance, I think it could race with B_CACHE being set in bufdone_finish().