Attilio,
On Thu, Dec 27, 2012 at 12:36:58PM +0000, Attilio Rao wrote:
A> Author: attilio
A> Date: Thu Dec 27 12:36:58 2012
A> New Revision: 244732
A> URL: http://svnweb.freebsd.org/changeset/base/244732
A>
A> Log:
A> br_prod_tail and br_cons_tail members are used as barrier to
A> signal bug_ring ownership. However, instructions can be reordered
A> around members write leading to stale values for ie. br_prod_bufs.
A>
A> Use correct memory barriers to ensure proper ordering of the
A> ownership tokens updates.
A>
A> Sponsored by: EMC / Isilon storage division
A> MFC after: 2 weeks
Have you profiled this?
After this change the buf_ring actually gains its own hand-rolled
mutex:
while (atomic_load_acq_32(&br->br_prod_tail) != prod_head)
cpu_spinwait();
The only difference with mutex(9) is that this one isn't monitored
by WITNESS.
The idea behind buf_ring was lockless storing and lockless fetching
from a ring and now this vanished.
What does your change actually fixes except precise accounting of
br_prod_bufs that are actually unused and should be better garbage
collected rather than fixed?
A> Modified:
A> head/sys/sys/buf_ring.h
A>
A> Modified: head/sys/sys/buf_ring.h
A>
==============================================================================
A> --- head/sys/sys/buf_ring.h Thu Dec 27 09:15:21 2012 (r244731)
A> +++ head/sys/sys/buf_ring.h Thu Dec 27 12:36:58 2012 (r244732)
A> @@ -114,10 +114,10 @@ buf_ring_enqueue(struct buf_ring *br, vo
A> * that preceeded us, we need to wait for them
A> * to complete
A> */
A> - while (br->br_prod_tail != prod_head)
A> + while (atomic_load_acq_32(&br->br_prod_tail) != prod_head)
A> cpu_spinwait();
A> br->br_prod_bufs++;
A> - br->br_prod_tail = prod_next;
A> + atomic_store_rel_32(&br->br_prod_tail, prod_next);
A> critical_exit();
A> return (0);
A> }
A> @@ -161,10 +161,10 @@ buf_ring_dequeue_mc(struct buf_ring *br)
A> * that preceeded us, we need to wait for them
A> * to complete
A> */
A> - while (br->br_cons_tail != cons_head)
A> + while (atomic_load_acq_32(&br->br_cons_tail) != cons_head)
A> cpu_spinwait();
A>
A> - br->br_cons_tail = cons_next;
A> + atomic_store_rel_32(&br->br_cons_tail, cons_next);
A> critical_exit();
A>
A> return (buf);
A> @@ -209,7 +209,7 @@ buf_ring_dequeue_sc(struct buf_ring *br)
A> panic("inconsistent list cons_tail=%d cons_head=%d",
A> br->br_cons_tail, cons_head);
A> #endif
A> - br->br_cons_tail = cons_next;
A> + atomic_store_rel_32(&br->br_cons_tail, cons_next);
A> return (buf);
A> }
A>
--
Totus tuus, Glebius.
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
