Date: Fri, 28 Dec 2012 17:41:36 +0000 (UTC) From: Attilio Rao <attilio@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r244774 - head/sys/sys Message-ID: <201212281741.qBSHfaYL071179@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: attilio Date: Fri Dec 28 17:41:36 2012 New Revision: 244774 URL: http://svnweb.freebsd.org/changeset/base/244774 Log: Improve bufring impl: - Remove unused br_prod_bufs member - Fixup r241037: buf_ring pads br_prod_* and br_cons_* members at 128 bytes, assuming a fixed cache line size for all the architectures. However, the above mentioned revision broke the padding. Use explicit padding to the CACHE_LINE_SIZE on the members that mark the initial new padded sections. Of course, the padding is not important for performance reasons in the DEBUG_BUFRING case, leaving br_cons members to share the cache line with br_lock. - Fixup r244732: by removing incorrectly added membar in buf_ring_dequeue_sc() where surrounding locking shoud be enough. - Drastically reduce the number of membar used (pratically reverting r244732) by switching rmb() in buf_ring_dequeue_mc() and wmb() in buf_ring_enqueue() to be complete barriers. This, along with br_prod_bufs departure, should fix ordering issues as explained in the provided comments. This patch is not targeted for MFC. Sponsored by: EMC / Isilon storage division Reviewed by: glebius Modified: head/sys/sys/buf_ring.h Modified: head/sys/sys/buf_ring.h ============================================================================== --- head/sys/sys/buf_ring.h Fri Dec 28 14:47:34 2012 (r244773) +++ head/sys/sys/buf_ring.h Fri Dec 28 17:41:36 2012 (r244774) @@ -47,25 +47,14 @@ struct buf_ring { int br_prod_size; int br_prod_mask; uint64_t br_drops; - uint64_t br_prod_bufs; - /* - * Pad out to next L2 cache line - */ - uint64_t _pad0[11]; - - volatile uint32_t br_cons_head; + volatile uint32_t br_cons_head __aligned(CACHE_LINE_SIZE); volatile uint32_t br_cons_tail; int br_cons_size; int br_cons_mask; - - /* - * Pad out to next L2 cache line - */ - uint64_t _pad1[14]; #ifdef DEBUG_BUFRING struct mtx *br_lock; #endif - void *br_ring[0]; + void *br_ring[0] __aligned(CACHE_LINE_SIZE); }; /* @@ -103,17 +92,21 @@ buf_ring_enqueue(struct buf_ring *br, vo panic("dangling value in enqueue"); #endif br->br_ring[prod_head] = buf; - wmb(); + + /* + * The full memory barrier also avoids that br_prod_tail store + * is reordered before the br_ring[prod_head] is full setup. + */ + mb(); /* * If there are other enqueues in progress * that preceeded us, we need to wait for them * to complete */ - while (atomic_load_acq_32(&br->br_prod_tail) != prod_head) + while (br->br_prod_tail != prod_head) cpu_spinwait(); - br->br_prod_bufs++; - atomic_store_rel_32(&br->br_prod_tail, prod_next); + br->br_prod_tail = prod_next; critical_exit(); return (0); } @@ -150,17 +143,22 @@ buf_ring_dequeue_mc(struct buf_ring *br) #ifdef DEBUG_BUFRING br->br_ring[cons_head] = NULL; #endif - rmb(); + + /* + * The full memory barrier also avoids that br_ring[cons_read] + * load is reordered after br_cons_tail is set. + */ + mb(); /* * If there are other dequeues in progress * that preceeded us, we need to wait for them * to complete */ - while (atomic_load_acq_32(&br->br_cons_tail) != cons_head) + while (br->br_cons_tail != cons_head) cpu_spinwait(); - atomic_store_rel_32(&br->br_cons_tail, cons_next); + br->br_cons_tail = cons_next; critical_exit(); return (buf); @@ -205,7 +203,7 @@ buf_ring_dequeue_sc(struct buf_ring *br) panic("inconsistent list cons_tail=%d cons_head=%d", br->br_cons_tail, cons_head); #endif - atomic_store_rel_32(&br->br_cons_tail, cons_next); + br->br_cons_tail = cons_next; return (buf); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201212281741.qBSHfaYL071179>