[Differential] [Commented On] D1833: Add memory barriers to buf_ring

kmacy (Kip Macy) phabric-noreply at FreeBSD.org
Sun Feb 22 01:12:40 UTC 2015


kmacy added a comment.

>>! In D1833#26, @meloun-miracle-cz wrote:
> 
> 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.

The update to prod_tail happens _after_ the store to br_ring and is done with an atomic_store_rel_int subsequently in dequeue_sc the read of prod_tail will be done with an atomic_load_acq_int before loading br_prod. Since there is a memory barrier after the update in enqueue and one before the read where is the problem?

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

Great, but the only variable that is updated with atomic_cmpset is prod_head to atomically acquire the right to store in to that index in br_ring.


Please clarify what the actual problem is. This change fixes the only real issue that I can glean from your comment.

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