[Differential] [Changed Subscribers] D1833: Add memory barriers to buf_ring

meloun-miracle-cz (Michal Meloun) phabric-noreply at FreeBSD.org
Sat Feb 21 06:34:42 UTC 2015


meloun-miracle-cz added a subscriber: meloun-miracle-cz.
meloun-miracle-cz added a comment.


Even I’m not able to evaluate all aspect, I see some serious defects here - at least from ARM point of view.
The buf_ring_enqueue() guarantees proper write ordering (and visibility):
-	Store with  acquire to br->br_prod_head
-	Normal store to br_ring[]
-	Store with release to br_prod_tail.
Unfortunately buf_ring_dequeue_sc() have not defined any read ordering and code can see
updated br_prod_tail and stale br_ring[]. (imho, this is true for amd64/i386 too).
Unlike of Semihalf guys, I see little different solution for race in buf_ring_dequeue_sc() read logic.
The br_ring[] must be read after br_prod_tail, but read order of br_cons_head and  br_prod_tail is not important.
So (line 183)


  buf = atomic_load_rel_32(&br->br_ring[cons_head]);


looks more correct to me.


On ARM, all stores to variable referenced by atomic_cmpset() must be done by atomic_store (or by other
atomic_* functions), normal store doesn’t clear exclusive monitor. 
Thus, for ARM we **!!MUST!!** use atomic_store() for each store to variable referenced by atomic_cmpset() !

Ohh, and note – current atomic_store() implementation on ARM is broken too, but fix is easy (see atomic store_*_64().

REVISION DETAIL
  https://reviews.freebsd.org/D1833

To: zbb, kmacy, rpaulo, imp
Cc: meloun-miracle-cz, onwahe-gmail-com, andrew, ian, adrian, freebsd-arm


More information about the freebsd-arm mailing list