Avoid unfruitful wakeups in bdone

[cem]

Summary

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.

[jhb]

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.

[kib]

This should be fine, assuming you can provide convincing arguments that modifying b_flags in bwait() is safe.

[cem]

In D8098#167544, @kib wrote:

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.

[cem]

Update B_WAITING comment w/ John's suggestion.

[kib]

In D8098#167547, @cem wrote:

In D8098#167544, @kib wrote:

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

No, b_flags is protected by the b_lock. mtxpool in bdone() avoids lost wakeups.

[cem]

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.

[mlaier]

This is not safe to do.

[cem]

Attilio is reworking the patch.

[kib]

In D8098#167664, @cem wrote:

Attilio is reworking the patch.

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.

[rang_acm.org]

In D8098#167666, @kib wrote:

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().

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().

[cem]

Attilio decided it wasn't actually going to help much.