svn commit: r240427 - head/sys/dev/virtio

[John Baldwin]

On Thursday, September 13, 2012 12:40:42 pm Bryan Venteicher wrote:
> Hi,
> 
> ----- Original Message -----
> > From: "John Baldwin" <jhb at freebsd.org>
> > To: "Peter Grehan" <grehan at freebsd.org>
> > Cc: svn-src-head at freebsd.org, svn-src-all at freebsd.org, src-
committers at 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-head at freebsd.org mailing list
> > http://lists.freebsd.org/mailman/listinfo/svn-src-head
> > To unsubscribe, send any mail to
> > "svn-src-head-unsubscribe at freebsd.org"
> > 
> 

-- 
John Baldwin