Date: Sat, 21 Feb 2015 06:34:41 +0000 From: "meloun-miracle-cz (Michal Meloun)" <phabric-noreply@FreeBSD.org> To: freebsd-arm@freebsd.org Subject: [Differential] [Changed Subscribers] D1833: Add memory barriers to buf_ring Message-ID: <cf9fde4f2c6acdb583dd5ea06df326f7@localhost.localdomain> In-Reply-To: <differential-rev-PHID-DREV-ntnotl4wj5wcj2eoukly-req@FreeBSD.org> References: <differential-rev-PHID-DREV-ntnotl4wj5wcj2eoukly-req@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?cf9fde4f2c6acdb583dd5ea06df326f7>