Date: Fri, 14 Sep 2012 00:47:52 -0500 (CDT) From: Bryan Venteicher <bryanv@daemoninthecloset.org> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Peter Grehan <grehan@freebsd.org> Subject: Re: svn commit: r240427 - head/sys/dev/virtio Message-ID: <2045684227.3044.1347601672495.JavaMail.root@daemoninthecloset.org> In-Reply-To: <201209131456.03422.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi
----- Original Message -----
> From: "John Baldwin" <jhb@freebsd.org>
> To: "Bryan Venteicher" <bryanv@daemoninthecloset.org>
> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, "Peter Grehan"
> <grehan@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;
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
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2045684227.3044.1347601672495.JavaMail.root>