Hi, ----- Original Message ----- > From: "John Baldwin" <j...@freebsd.org> > To: "Peter Grehan" <gre...@freebsd.org> > Cc: svn-src-h...@freebsd.org, svn-src-all@freebsd.org, > src-committ...@freebsd.org > Sent: Thursday, September 13, 2012 7:59:38 AM > Subject: Re: svn commit: r240427 - head/sys/dev/virtio > > On Wednesday, September 12, 2012 8:36:47 pm Peter Grehan wrote: > > Author: grehan > > Date: Thu Sep 13 00:36:46 2012 > > New Revision: 240427 > > URL: http://svn.freebsd.org/changeset/base/240427 > > > > Log: > > Relax requirement of certain mb()s > > > > Submitted by: Bryan Venteicher bryanv at daemoninthecloset org > > > > Modified: > > head/sys/dev/virtio/virtqueue.c > > > > Modified: head/sys/dev/virtio/virtqueue.c > > > ============================================================================== > > --- head/sys/dev/virtio/virtqueue.c Wed Sep 12 22:54:11 2012 > > (r240426) > > +++ head/sys/dev/virtio/virtqueue.c Thu Sep 13 00:36:46 2012 > > (r240427) > > @@ -525,7 +525,7 @@ virtqueue_dequeue(struct virtqueue *vq, > > used_idx = vq->vq_used_cons_idx++ & (vq->vq_nentries - 1); > > uep = &vq->vq_ring.used->ring[used_idx]; > > > > - mb(); > > + rmb(); > > desc_idx = (uint16_t) uep->id; > > if (len != NULL) > > *len = uep->len; > > @@ -623,7 +623,7 @@ vq_ring_update_avail(struct virtqueue *v > > avail_idx = vq->vq_ring.avail->idx & (vq->vq_nentries - 1); > > vq->vq_ring.avail->ring[avail_idx] = desc_idx; > > > > - mb(); > > + wmb(); > > vq->vq_ring.avail->idx++; > > > > /* Keep pending count until virtqueue_notify(). */ > > Would it be possible to use atomic_load/store() instead of direct > memory barriers? For example: >
I've been sitting on a (lightly tested) patch [1] for awhile that does just that, but am not very happy with it. A lot of the fields are 16-bit, which not all architectures have atomic(9) support for. And I think the atomic(9) behavior on UP kernels does not provide the same guarantees as on an SMP kernel (could have an UP kernel on an SMP host). I also found myself wanting an atomic_load_rel_*() type function. > desc_idx = (uint16_t)atomic_load_acq_int(&uep->id); > > and > > atomic_store_rel_int(&vq->vq_ring.avail->ring[avail_idx], desc_idx); avail->ring is an array of uint16's so I'm a bit leery of using a (presumably) 32bit op on it. [1] http://www.daemoninthecloset.org/~bryanv/patches/freebsd/vq_atomic.patch > > -- > John Baldwin > _______________________________________________ > svn-src-h...@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/svn-src-head > To unsubscribe, send any mail to > "svn-src-head-unsubscr...@freebsd.org" > _______________________________________________ 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"
