Re: [Qemu-devel] [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)

2015-12-21 Thread Stefano Stabellini
On Mon, 21 Dec 2015, David Vrabel wrote: > On 20/12/15 09:25, Michael S. Tsirkin wrote: > > > > I noticed that drivers/xen/xenbus/xenbus_comms.c uses > > full memory barriers to communicate with the other side. > > For example: > > > > /* Must write data /after/ reading the consum

Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)

2015-12-21 Thread Michael S. Tsirkin
On Mon, Dec 21, 2015 at 10:47:49AM +, David Vrabel wrote: > On 20/12/15 09:25, Michael S. Tsirkin wrote: > > > > I noticed that drivers/xen/xenbus/xenbus_comms.c uses > > full memory barriers to communicate with the other side. > > For example: > > > > /* Must write data /afte

Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)

2015-12-21 Thread David Vrabel
On 20/12/15 09:25, Michael S. Tsirkin wrote: > > I noticed that drivers/xen/xenbus/xenbus_comms.c uses > full memory barriers to communicate with the other side. > For example: > > /* Must write data /after/ reading the consumer index. * */ > mb(); > >

Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)

2015-12-20 Thread Michael S. Tsirkin
On Sun, Dec 20, 2015 at 08:59:44PM +0100, Peter Zijlstra wrote: > On Sun, Dec 20, 2015 at 05:07:19PM +, Andrew Cooper wrote: > > > > Very much +1 for fixing this. > > > > Those names would be fine, but they do add yet another set of options in > > an already-complicated area. > > > > An alte

Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)

2015-12-20 Thread Peter Zijlstra
On Sun, Dec 20, 2015 at 05:07:19PM +, Andrew Cooper wrote: > > Very much +1 for fixing this. > > Those names would be fine, but they do add yet another set of options in > an already-complicated area. > > An alternative might be to have the regular smp_{w,r,}mb() not revert > back to nops if

Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)

2015-12-20 Thread Andrew Cooper
On 20/12/15 09:25, Michael S. Tsirkin wrote: > On Thu, Dec 17, 2015 at 03:39:10PM +0100, Peter Zijlstra wrote: >> On Thu, Dec 17, 2015 at 04:33:44PM +0200, Michael S. Tsirkin wrote: >>> On Thu, Dec 17, 2015 at 02:57:26PM +0100, Peter Zijlstra wrote: You could of course go fix that instead of m

new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)

2015-12-20 Thread Michael S. Tsirkin
On Thu, Dec 17, 2015 at 03:39:10PM +0100, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 04:33:44PM +0200, Michael S. Tsirkin wrote: > > On Thu, Dec 17, 2015 at 02:57:26PM +0100, Peter Zijlstra wrote: > > > > > > You could of course go fix that instead of mutilating things into > > > sort-of func

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Michael S. Tsirkin
On Thu, Dec 17, 2015 at 03:52:24PM +, Will Deacon wrote: > On Thu, Dec 17, 2015 at 04:09:17PM +0100, Peter Zijlstra wrote: > > On Thu, Dec 17, 2015 at 04:34:57PM +0200, Michael S. Tsirkin wrote: > > > On Thu, Dec 17, 2015 at 03:02:12PM +0100, Peter Zijlstra wrote: > > > > > > > commit 9e

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Will Deacon
On Thu, Dec 17, 2015 at 04:09:17PM +0100, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 04:34:57PM +0200, Michael S. Tsirkin wrote: > > On Thu, Dec 17, 2015 at 03:02:12PM +0100, Peter Zijlstra wrote: > > > > > commit 9e1a27ea42691429e31f158cce6fc61bc79bb2e9 > > > > Author: Alexan

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Peter Zijlstra
On Thu, Dec 17, 2015 at 04:34:57PM +0200, Michael S. Tsirkin wrote: > On Thu, Dec 17, 2015 at 03:02:12PM +0100, Peter Zijlstra wrote: > > > commit 9e1a27ea42691429e31f158cce6fc61bc79bb2e9 > > > Author: Alexander Duyck > > > Date: Mon Apr 13 21:03:49 2015 +0930 > > > > > > virtio_ri

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Michael S. Tsirkin
On Thu, Dec 17, 2015 at 03:39:10PM +0100, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 04:33:44PM +0200, Michael S. Tsirkin wrote: > > On Thu, Dec 17, 2015 at 02:57:26PM +0100, Peter Zijlstra wrote: > > > > > > You could of course go fix that instead of mutilating things into > > > sort-of func

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Peter Zijlstra
On Thu, Dec 17, 2015 at 04:33:44PM +0200, Michael S. Tsirkin wrote: > On Thu, Dec 17, 2015 at 02:57:26PM +0100, Peter Zijlstra wrote: > > > > You could of course go fix that instead of mutilating things into > > sort-of functional state. > > Yes, we'd just need to touch all architectures, all for

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Michael S. Tsirkin
On Thu, Dec 17, 2015 at 03:02:12PM +0100, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 03:26:29PM +0200, Michael S. Tsirkin wrote: > > > Note that virtio_mb() is weirdly inconsistent with virtio_[rw]mb() in > > > that they use dma_* ops for weak_barriers, while virtio_mb() uses > > > smp_mb(). >

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Michael S. Tsirkin
On Thu, Dec 17, 2015 at 02:57:26PM +0100, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 03:16:20PM +0200, Michael S. Tsirkin wrote: > > On Thu, Dec 17, 2015 at 11:52:38AM +0100, Peter Zijlstra wrote: > > > On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote: > > > > +static inline

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Peter Zijlstra
On Thu, Dec 17, 2015 at 03:26:29PM +0200, Michael S. Tsirkin wrote: > > Note that virtio_mb() is weirdly inconsistent with virtio_[rw]mb() in > > that they use dma_* ops for weak_barriers, while virtio_mb() uses > > smp_mb(). > > It's a hack really. I think I'll clean it up a bit to > make it more

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Peter Zijlstra
On Thu, Dec 17, 2015 at 03:16:20PM +0200, Michael S. Tsirkin wrote: > On Thu, Dec 17, 2015 at 11:52:38AM +0100, Peter Zijlstra wrote: > > On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote: > > > +static inline void virtio_store_mb(bool weak_barriers, > > > +

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Michael S. Tsirkin
On Thu, Dec 17, 2015 at 12:22:22PM +0100, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote: > > Seems to give a speedup on my box but I'm less sure about this one. E.g. as > > xchng faster than mfence on all/most intel CPUs? Anyone has an opinion? > > Wou

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Michael S. Tsirkin
On Thu, Dec 17, 2015 at 11:52:38AM +0100, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote: > > +static inline void virtio_store_mb(bool weak_barriers, > > + __virtio16 *p, __virtio16 v) > > +{ > > +#ifdef CONFIG_SMP > > + if

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Peter Zijlstra
On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote: > Seems to give a speedup on my box but I'm less sure about this one. E.g. as > xchng faster than mfence on all/most intel CPUs? Anyone has an opinion? Would help if you Cc people who would actually know this :-) Yes, we've rece

Re: [PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Peter Zijlstra
On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote: > +static inline void virtio_store_mb(bool weak_barriers, > +__virtio16 *p, __virtio16 v) > +{ > +#ifdef CONFIG_SMP > + if (weak_barriers) > + smp_store_mb(*p, v); > + else > +#en

[PATCH] virtio_ring: use smp_store_mb

2015-12-17 Thread Michael S. Tsirkin
We need a full barrier after writing out event index, using smp_store_mb there seems better than open-coding. As usual, we need a wrapper to account for strong barriers/non smp. It's tempting to use this in vhost as well, for that, we'll need a variant of smp_store_mb that works on __user pointers