Page MenuHomeFreeBSD

buf_ring.h: fix memory order issues.
Needs ReviewPublic

Authored by oleg on Nov 25 2016, 11:57 AM.

Details

Summary

Properly use acqure/release atomics.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

oleg updated this revision to Diff 22507.Nov 25 2016, 11:57 AM
oleg retitled this revision from to buf_ring.h: fix memory order issues..
oleg updated this object.
oleg edited the test plan for this revision. (Show Details)
oleg added reviewers: kmacy, alc, kib.
oleg added a subscriber: freebsd-net-list.
emaste added a subscriber: emaste.Dec 1 2016, 10:06 PM
alc edited edge metadata.Dec 5 2016, 5:50 PM

Have you looked at D1945, in particular, the most recent postings by sbahra_repnop.org? It's not clear to me that these changes will address the problem described in sbahra_repnop.org's postings. That said, your proposed changes do correct the most obvious remaining issues with the use of acquires and releases in this code.

sys/sys/buf_ring.h
78

Was there a reason for moving this load?

138

You may need to use a load acquire on br_prod_tail here to establish an unbroken synchronizes-with chain between the thread that enqueues an item X and the thread that later dequeues it if there are other concurrent enqueues.

157

Was there a reason for moving this load?

230

Was there a reason for swapping the order of the preceding loads?

250

Was there a reason for swapping the order of the preceding loads?

oleg added a comment.Dec 6 2016, 12:54 AM
In D8637#180625, @alc wrote:

Have you looked at D1945, in particular, the most recent postings by sbahra_repnop.org? It's not clear to me that these changes will address the problem described in sbahra_repnop.org's postings. That said, your proposed changes do correct the most obvious remaining issues with the use of acquires and releases in this code.

Yes, i've looked at D1945 but somehow i've missed sbahra_repnop.org's comments. Since i was trying to fix different issue (https://lists.freebsd.org/pipermail/freebsd-net/2013-December/037265.html) and remove my own rmb() hack the problem described by sbahra_repnop.org is still here. I'll post updated diff tomorrow.

sys/sys/buf_ring.h
138

AFAIK 'synchronizes-with chains' are about RMW operations in between load_acq/store_rel sequence. Can you explain why load_acq is necessary here? I think producer should not care about visibility of br->br_ring[prod_head] stores of other producers.
Correct order of our own stores guaranteed by store_rel(&br->br_prod_tail) (and load_acq() in consumer).

oleg updated this revision to Diff 22728.Dec 6 2016, 1:26 PM
oleg edited edge metadata.
  1. fix issues reported by sbahra_repnop.org here: https://reviews.freebsd.org/D1945
  2. fix synchronization chain (producer -> producer -> consumer) noted by alc.
  3. massive comments.
sbahra_repnop.org resigned from this revision.Jul 28 2017, 5:31 PM