[PULL] vhost: cleanups and fixes
The following changes since commit 9f9499ae8e6415cefc4fe0a96ad0e27864353c89: Linux 4.4-rc5 (2015-12-13 17:42:58 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus for you to fetch changes up to 74a599f09bec7419b2490039f0fb33bc8581ef7c: virtio/s390: handle error values in irb (2015-12-17 10:37:33 +0200) virtio: fixes on top of 4.4-rc5 This includes a single fix for virtio ccw error handling. Signed-off-by: Michael S. Tsirkin Cornelia Huck (1): virtio/s390: handle error values in irb drivers/s390/virtio/virtio_ccw.c | 62 1 file changed, 37 insertions(+), 25 deletions(-) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH RFC] smp_store_mb should use smp_mb
On some architectures smp_store_mb() calls mb() which is stronger
than implied by both the name and the documentation.
smp_store_mb is only used by core kernel code at the moment, so
we know no one mis-uses it for an MMIO barrier.
Make it call smp_mb consistently before some arch-specific
code uses it as such by mistake.
Signed-off-by: Michael S. Tsirkin
---
Note: I'm guessing an ack from arch maintainers will be needed, but
I'm working on a bigger cleanup moving a bunch of duplicated code
into asm-generic/barrier.h which depends on this, so not Cc'ing
widely yet.
Please Ack if appropriate but do not merge yet.
diff --git a/arch/ia64/include/asm/barrier.h b/arch/ia64/include/asm/barrier.h
index df896a1..425552b 100644
--- a/arch/ia64/include/asm/barrier.h
+++ b/arch/ia64/include/asm/barrier.h
@@ -77,7 +77,7 @@ do {
\
___p1; \
})
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); }
while (0)
+#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb();
} while (0)
/*
* The group barrier in front of the rsm & ssm are necessary to ensure
diff --git a/arch/powerpc/include/asm/barrier.h
b/arch/powerpc/include/asm/barrier.h
index 0eca6ef..4f0169e 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -34,7 +34,7 @@
#define rmb() __asm__ __volatile__ ("sync" : : : "memory")
#define wmb() __asm__ __volatile__ ("sync" : : : "memory")
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); }
while (0)
+#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb();
} while (0)
#ifdef __SUBARCH_HAS_LWSYNC
#define SMPWMB LWSYNC
diff --git a/arch/s390/include/asm/barrier.h b/arch/s390/include/asm/barrier.h
index d68e11e..6c1d8b5 100644
--- a/arch/s390/include/asm/barrier.h
+++ b/arch/s390/include/asm/barrier.h
@@ -36,7 +36,7 @@
#define smp_mb__before_atomic()smp_mb()
#define smp_mb__after_atomic() smp_mb()
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value);
mb(); } while (0)
+#define smp_store_mb(var, value) do { WRITE_ONCE(var, value);
smp_mb(); } while (0)
#define smp_store_release(p, v)
\
do { \
diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
index b42afad..0f45f93 100644
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -93,7 +93,7 @@
#endif /* CONFIG_SMP */
#ifndef smp_store_mb
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while
(0)
+#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); }
while (0)
#endif
#ifndef smp_mb__before_atomic
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
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 alternative might be to have the regular smp_{w,r,}mb() not revert
> > back to nops if CONFIG_PARAVIRT, or perhaps if pvops have detected a
> > non-native environment. (I don't know how feasible this suggestion is,
> > however.)
>
> So a regular SMP kernel emits the LOCK prefix and will patch it out with
> a DS prefix (iirc) when it finds but a single CPU. So for those you
> could easily do this.
>
> However an UP kernel will not emit the LOCK and do no patching.
>
> So if you're willing to make CONFIG_PARAVIRT depend on CONFIG_SMP or
> similar, this is doable.
One of the uses for virtio is to allow testing an existing kernel on
kvm just by loading a module, and this will break this usecase.
> I don't see people going to allow emitting the LOCK prefix (and growing
> the kernel text size) for UP kernels.
Thinking about this more, maybe __smp_*mb is a better set of names.
The nice thing about it is that we can then have generic code
that does basically
#ifdef CONFIG_SMP
#define smp_mb() __smp_mb()
#else
#define smp_mb() barrier()
#endif
and reuse this on all architectures.
So instead of a maintainance burden, we are actually
removing code duplication.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
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 CONFIG_PARAVIRT, or perhaps if pvops have detected a
> non-native environment. (I don't know how feasible this suggestion is,
> however.)
So a regular SMP kernel emits the LOCK prefix and will patch it out with
a DS prefix (iirc) when it finds but a single CPU. So for those you
could easily do this.
However an UP kernel will not emit the LOCK and do no patching.
So if you're willing to make CONFIG_PARAVIRT depend on CONFIG_SMP or
similar, this is doable.
I don't see people going to allow emitting the LOCK prefix (and growing
the kernel text size) for UP kernels.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
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 mutilating things into
sort-of functional state.
>>> Yes, we'd just need to touch all architectures, all for
>>> the sake of UP which almost no one uses.
>>> Basically, we need APIs that explicitly are
>>> for talking to another kernel on a different CPU on
>>> the same SMP system, and implemented identically
>>> between CONFIG_SMP and !CONFIG_SMP on all architectures.
>>>
>>> Do you think this is something of general usefulness,
>>> outside virtio?
>> I'm not aware of any other case, but if there are more parts of virt
>> that need this then I see no problem adding it.
> When I wrote this, I assumed there are no other users, and I'm still not
> sure there are other users at the moment. Do you see a problem then?
>
>> That is, virt in general is the only use-case that I can think of,
>> because this really is an artifact of interfacing with an SMP host while
>> running an UP kernel.
> Or another guest kernel on an SMP host.
>
>> But I'm really not familiar with virt, so I do not know if there's more
>> sites outside of virtio that could use this.
>> Touching all archs is a tad tedious, but its fairly straight forward.
> So I looked and I was only able to find other another possible user in Xen.
>
> Cc Xen folks.
>
> 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();
>
> memcpy(dst, data, avail);
> data += avail;
> len -= avail;
>
> /* Other side must not see new producer until data is *
> there. */
> wmb();
> intf->req_prod += avail;
>
> /* Implies mb(): other side will see the updated producer. */
> notify_remote_via_evtchn(xen_store_evtchn);
>
> To me, it looks like for guests compiled with CONFIG_SMP, smp_wmb and smp_mb
> would be sufficient, so mb() and wmb() here are only needed if
> a non-SMP guest runs on an SMP host.
>
> Is my analysis correct?
Correct. The reason full barriers are used is so non-SMP guests still
function correctly. It is certainly inefficient.
>
> So what I'm suggesting is something like the below patch,
> except instead of using virtio directly, a new set of barriers
> that behaves identically for SMP and non-SMP guests will be introduced.
>
> And of course the weak barriers flag is not needed for Xen -
> that's a virtio only thing.
>
> For example:
>
> smp_pv_wmb()
> smp_pv_rmb()
> smp_pv_mb()
>
> I'd like to get confirmation from Xen folks before posting
> this patchset.
>
> Comments/suggestions?
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 CONFIG_PARAVIRT, or perhaps if pvops have detected a
non-native environment. (I don't know how feasible this suggestion is,
however.)
~Andrew
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
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 functional state.
> >
> > Yes, we'd just need to touch all architectures, all for
> > the sake of UP which almost no one uses.
> > Basically, we need APIs that explicitly are
> > for talking to another kernel on a different CPU on
> > the same SMP system, and implemented identically
> > between CONFIG_SMP and !CONFIG_SMP on all architectures.
> >
> > Do you think this is something of general usefulness,
> > outside virtio?
>
> I'm not aware of any other case, but if there are more parts of virt
> that need this then I see no problem adding it.
When I wrote this, I assumed there are no other users, and I'm still not
sure there are other users at the moment. Do you see a problem then?
> That is, virt in general is the only use-case that I can think of,
> because this really is an artifact of interfacing with an SMP host while
> running an UP kernel.
Or another guest kernel on an SMP host.
> But I'm really not familiar with virt, so I do not know if there's more
> sites outside of virtio that could use this.
> Touching all archs is a tad tedious, but its fairly straight forward.
So I looked and I was only able to find other another possible user in Xen.
Cc Xen folks.
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();
memcpy(dst, data, avail);
data += avail;
len -= avail;
/* Other side must not see new producer until data is * there.
*/
wmb();
intf->req_prod += avail;
/* Implies mb(): other side will see the updated producer. */
notify_remote_via_evtchn(xen_store_evtchn);
To me, it looks like for guests compiled with CONFIG_SMP, smp_wmb and smp_mb
would be sufficient, so mb() and wmb() here are only needed if
a non-SMP guest runs on an SMP host.
Is my analysis correct?
So what I'm suggesting is something like the below patch,
except instead of using virtio directly, a new set of barriers
that behaves identically for SMP and non-SMP guests will be introduced.
And of course the weak barriers flag is not needed for Xen -
that's a virtio only thing.
For example:
smp_pv_wmb()
smp_pv_rmb()
smp_pv_mb()
I'd like to get confirmation from Xen folks before posting
this patchset.
Comments/suggestions?
Signed-off-by: Michael S. Tsirkin
--
compile-tested only.
diff --git a/drivers/xen/xenbus/xenbus_comms.c
b/drivers/xen/xenbus/xenbus_comms.c
index fdb0f33..a28f049 100644
--- a/drivers/xen/xenbus/xenbus_comms.c
+++ b/drivers/xen/xenbus/xenbus_comms.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -123,14 +124,14 @@ int xb_write(const void *data, unsigned len)
avail = len;
/* Must write data /after/ reading the consumer index. */
- mb();
+ virtio_mb(true);
memcpy(dst, data, avail);
data += avail;
len -= avail;
/* Other side must not see new producer until data is there. */
- wmb();
+ virtio_wmb(true);
intf->req_prod += avail;
/* Implies mb(): other side will see the updated producer. */
@@ -180,14 +181,14 @@ int xb_read(void *data, unsigned len)
avail = len;
/* Must read data /after/ reading the producer index. */
- rmb();
+ virtio_rmb(true);
memcpy(data, src, avail);
data += avail;
len -= avail;
/* Other side must not see free space until we've copied out */
- mb();
+ virtio_mb(true);
intf->rsp_cons += avail;
pr_debug("Finished read of %i bytes (%i to go)\n", avail, len);
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
