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

Konstantin Belousov kostikbel at gmail.com
Fri Sep 14 07:55:34 UTC 2012


On Fri, Sep 14, 2012 at 12:47:52AM -0500, Bryan Venteicher wrote:
> Hi
> 
> ----- Original Message -----
> > From: "John Baldwin" <jhb at freebsd.org>
> > To: "Bryan Venteicher" <bryanv at daemoninthecloset.org>
> > Cc: svn-src-head at freebsd.org, svn-src-all at freebsd.org, src-committers at freebsd.org, "Peter Grehan"
> > <grehan at freebsd.org>
> > Sent: Thursday, September 13, 2012 1:56:03 PM
> > Subject: Re: svn commit: r240427 - head/sys/dev/virtio
> > 
> > On Thursday, September 13, 2012 12:40:42 pm Bryan Venteicher wrote:
> > > > 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).
> > 
> > That is the one thing I was worried about (the fields being defined
> > to be 16-bit).  I presume that is required by the virtio de facto
> > standard?  Shame we can't clue-by-four people putting 16-bit fields
> > in these sort of things. :-P
> > 
> 
> Yes, the 16-bit fields are mandated by the VirtIO spec. The guest/host
> shared memory is rounded up to next full page, so there actually isn't
> any memory savings for typical queue sizes. Doubt it is any worse than
> actual hardware regardless.
> 
> > > I also found myself wanting an atomic_load_rel_*() type function.
> > 
> > That would be odd I think.  _rel barriers only affect stores, so
> > there would be no defined ordering between the load and the
> > subsequent stores.  (With our current definitions of _acq and
> > _rel.)  If you need a full fence for some reason, than a plain
> > mb() may be the best thing in that case.
> > 
> 
> I'm able to batch add descriptors (via vq_ring_update_avail()),
> but when checking if I must notify the host, I need to make sure
> the latest avail->idx is visible before checking the flag from
> the host on whether notifications are disabled. Gratuitous
> notifications are fine, but skipping one is not.
> 
> In the patch, I kludge this with:
>     atomic_add_rel_16(&flags, 0);
>     foo = flags;
Don't you need
	atomic_store_rel_16(&foo, flags);
instead ?

You might do a cas_rel over the containing 32bit word as well.
> 
> Hoping the dependency would prevent the assignment to foo from
> floating above the atomic_add_rel(). 
> 
> I originally did the atomic(9) work just to see if there would be
> any performance difference between the two - I wasn't able to
> measure any, but I don't have the most modern hardware either.
> 
> Bryan
> 
> > --
> > John Baldwin
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-head/attachments/20120914/8261594f/attachment.pgp


More information about the svn-src-head mailing list