svn commit: r244774 - head/sys/sys

[Attilio Rao]

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);
 }