A question about volatile away from the review.
Having both atomics and volatile isn't the usual pattern and the CW has usually been that adding volatile won't fix code like this, so either remove volatile, or comment about what it accomplishes likely is appropriate even if it's a brief reference....

sys/sys/buf_ring.h
49	So do these need to be volatile since we're using atomics on them? And how does forcing a fetch actually ensure that another thread's modification is visible to the current execution unit, or does that race somehow not matter? I'd think the answers to these questions should be, at least in brief form, commented somewhere. It's always hard to reason, exactly, about this stuff and knowing why each layer is needed would be helpful for people that aren't in the minutia of atomics every day enough to know this level of nuance off the top of their heads.

kib accepted this revision.Jul 26 2024, 6:35 PM

kib added inline comments.

sys/sys/buf_ring.h
49	Iif all accesses are consistently use atomic_ primitives, there is no sense in having volatile CV modifiers. The question of visibility is much more nuanced, and typically when using acq/rel pattern, we explain which acq pair with which rel, and what data modifications should be visible to acq consumer when rel store was noticed.

alc accepted this revision.Jul 29 2024, 12:09 AM

andrew added inline comments.Jul 29 2024, 10:26 AM

sys/sys/buf_ring.h
49	There are still a few places we use the values directly and not through an atomic function. We could move all accesses of the head and tail values to use atomic(9) and remove volatile from here in a follow up.