Page MenuHomeFreeBSD

buf_ring: Consistently use atomic_*_32
ClosedPublic

Authored by andrew on Jul 26 2024, 8:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 13, 2:36 PM
Unknown Object (File)
Thu, Oct 3, 5:02 AM
Unknown Object (File)
Wed, Oct 2, 1:29 AM
Unknown Object (File)
Fri, Sep 27, 4:03 AM
Unknown Object (File)
Thu, Sep 19, 9:48 AM
Unknown Object (File)
Sep 8 2024, 6:47 PM
Unknown Object (File)
Sep 8 2024, 7:46 AM
Unknown Object (File)
Sep 8 2024, 3:31 AM
Subscribers

Details

Diff Detail

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

Event Timeline

This revision is now accepted and ready to land.Jul 26 2024, 12:27 PM

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

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.

sys/sys/buf_ring.h
49

+1 for removing the volatile annotation.

This revision was automatically updated to reflect the committed changes.