* Re: [Xen-devel] [PATCH RFC 0/3] Xen on Virtio
From: Michael S. Tsirkin @ 2015-12-15 20:40 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stefano Stabellini, Benjamin Herrenschmidt,
linux-kernel@vger.kernel.org, Linux Virtualization, David Vrabel,
xen-devel@lists.xenproject.org
In-Reply-To: <CALCETrVEfrXNJE1Wq3piF7KPExk+4ByBP+rHHq2jWhCAURjcyA@mail.gmail.com>
On Mon, Dec 14, 2015 at 10:27:52AM -0800, Andy Lutomirski wrote:
> On Mon, Dec 14, 2015 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Dec 14, 2015 at 02:00:05PM +0000, David Vrabel wrote:
> >> On 07/12/15 16:19, Stefano Stabellini wrote:
> >> > Hi all,
> >> >
> >> > this patch series introduces support for running Linux on top of Xen
> >> > inside a virtual machine with virtio devices (nested virt scenario).
> >> > The problem is that Linux virtio drivers use virt_to_phys to get the
> >> > guest pseudo-physical addresses to pass to the backend, which doesn't
> >> > work as expected on Xen.
> >> >
> >> > Switching the virtio drivers to the dma APIs (dma_alloc_coherent,
> >> > dma_map/unmap_single and dma_map/unmap_sg) would solve the problem, as
> >> > Xen support in Linux provides an implementation of the dma API which
> >> > takes care of the additional address conversions. However using the dma
> >> > API would increase the complexity of the non-Xen case too. We would also
> >> > need to keep track of the physical or virtual address in addition to the
> >> > dma address for each vring_desc to be able to free the memory in
> >> > detach_buf (see patch #3).
> >> >
> >> > Instead this series adds few obvious checks to perform address
> >> > translations in a couple of key places, without changing non-Xen code
> >> > paths. You are welcome to suggest improvements or alternative
> >> > implementations.
> >>
> >> Andy Lutomirski also looked at this. Andy what happened to this work?
> >>
> >> David
> >
> > The approach there was to try and convert all virtio to use DMA
> > API unconditionally.
> > This is reasonable if there's a way for devices to request
> > 1:1 mappings individually.
> > As that is currently missing, that patchset can not be merged yet.
> >
>
> I still don't understand why *devices* need the ability to request
> anything in particular.
See below.
> In current kernels, devices that don't have
> an iommu work (and there's no choice about 1:1 or otherwise) and
> devices that have an iommu fail spectacularly. With the patches,
> devices that don't have an iommu continue to work as long as the DMA
> API and/or virtio correctly knows that there's no iommu. Devices that
> do have an iommu work fine, albeit slower than would be ideal. In my
> book, slower than would be ideal is strictly better than crashing.
>
> The real issue is *detecting* whether there's an iommu, and the string
> of bugs in that area (buggy QEMU for the Q35 thing and complete lack
> of a solution for PPC and SPARC is indeed a problem).
>
> I think that we could apply the series ending here:
>
> https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=virtio_dma&id=ad9d43052da44ce18363c02ea597dde01eeee11b
>
> and the only regression (performance or functionality) would be that
> the buggy Q35 iommu configuration would stop working until someone
> fixed it in QEMU. That should be okay -- it's explicitly
> experimental. (Xen works with that series applied.) (Actually,
> there might be a slight performance regression on PPC due to extra
> unused mappings being created. It would be straightforward to hack
> around that in one of several ways.)
>
> Am I missing something?
>
> --Andy
I think there's more to virtio than just QEMU.
I have no idea whether anyone implemented hypervisors with an IOMMU.
virtio bypassing iommu makes a lot of sense so it did this since
forever. I do not feel comfortable changing guest/hypervisor ABI and
waiting for people to complain.
But we do want to fix Xen.
Let's do this slowly, and whitelist the configurations that
require DMA API to work, so we know we are not breaking anything.
For example, test a device flag and use iommu if set.
Currently, set it if xen_pv_domain is enabled.
We'll add more as more platforms gain IOMMU support
for virtio and we find ways to identify them.
It would be kind of a mix of what you did and what Stefano did.
And alternative would be a quirk: make DMA API create 1:1 mappings for
virtio devices only. Then teach Xen pv to ignore this quirk. This is
what I referred to above.
For example, something like DMA_ATTR_IOMMU_BYPASS would do the trick
nicely. If there's a chance that's going to be upstream, we
could use that.
--
MST
^ permalink raw reply
* Re: [Xen-devel] [PATCH v2 0/3] Fix and cleanup for 32-bit PV sysexit
From: Andrew Cooper @ 2015-12-15 20:40 UTC (permalink / raw)
To: Andy Lutomirski, Boris Ostrovsky
Cc: linux-kernel@vger.kernel.org, Linux Virtualization, Ingo Molnar,
David Vrabel, Andrew Lutomirski, H. Peter Anvin,
xen-devel@lists.xenproject.org, Thomas Gleixner, Borislav Petkov
In-Reply-To: <CALCETrUeGoeB8vNgbRNkFXjH0A570=M+4L1sRsfh-7btym59nA@mail.gmail.com>
On 19/11/15 22:07, Andy Lutomirski wrote:
> On Thu, Nov 19, 2015 at 1:55 PM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>> The first patch fixes Xen PV regression introduced by 32-bit rewrite. Unlike the
>> earlier version it uses ALTERNATIVE instruction and avoids using xen_sysexit
>> (and sysret32 in compat mode) pv ops, as suggested by Andy.
>>
>> As result of this patch irq_enable_sysexit and usergs_sysret32 pv ops are not
>> used anymore by anyone and so can be removed.
> This whole series is:
>
> Acked-by: Andy Lutomirski <luto@kernel.org>
>
> Now I just have to sucker someone into getting rid of
> PARAVIRT_ADJUST_EXCEPTION_FRAME (by using stub entries) and the
> overcomplicated syscall entry stuff. :)
Looking at this, it should be quite easy now.
ALTERNATIVE "", "pop %rcx; %pop %11", X86_FEATURE_XENPV
(Completely untested)
> And whoever gets rid of
> PARAVIRT_ADJUST_EXCEPTION_FRAME gets to wonder why it doesn't crash
> and burn for NMIs on Xen, since I'm reasonably confident that it can't
> possibly be correct.
The Xen PV ABI only has a single kernel stack pointer which may be
registered. There is no equivalent of an IST, so if a second fault
occurs, it is delivered normally on the current stack.
By the looks of it, the other NMI handling is ambivalent to the fact
that it isn't really on an IST stack under Xen.
~Andrew
^ permalink raw reply
* Re: [Xen-devel] [PATCH RFC 0/3] Xen on Virtio
From: Michael S. Tsirkin @ 2015-12-15 20:44 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stefano Stabellini, Benjamin Herrenschmidt,
linux-kernel@vger.kernel.org, Linux Virtualization, David Vrabel,
xen-devel@lists.xenproject.org
In-Reply-To: <CALCETrX9g2Rsh5M+521Pv+OHO-HSUQLHTjGObU0y=qjrMdsgMQ@mail.gmail.com>
On Tue, Dec 15, 2015 at 08:45:32AM -0800, Andy Lutomirski wrote:
> On Tue, Dec 15, 2015 at 4:13 AM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 14 Dec 2015, Andy Lutomirski wrote:
> >> On Mon, Dec 14, 2015 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Mon, Dec 14, 2015 at 02:00:05PM +0000, David Vrabel wrote:
> >> >> On 07/12/15 16:19, Stefano Stabellini wrote:
> >> >> > Hi all,
> >> >> >
> >> >> > this patch series introduces support for running Linux on top of Xen
> >> >> > inside a virtual machine with virtio devices (nested virt scenario).
> >> >> > The problem is that Linux virtio drivers use virt_to_phys to get the
> >> >> > guest pseudo-physical addresses to pass to the backend, which doesn't
> >> >> > work as expected on Xen.
> >> >> >
> >> >> > Switching the virtio drivers to the dma APIs (dma_alloc_coherent,
> >> >> > dma_map/unmap_single and dma_map/unmap_sg) would solve the problem, as
> >> >> > Xen support in Linux provides an implementation of the dma API which
> >> >> > takes care of the additional address conversions. However using the dma
> >> >> > API would increase the complexity of the non-Xen case too. We would also
> >> >> > need to keep track of the physical or virtual address in addition to the
> >> >> > dma address for each vring_desc to be able to free the memory in
> >> >> > detach_buf (see patch #3).
> >> >> >
> >> >> > Instead this series adds few obvious checks to perform address
> >> >> > translations in a couple of key places, without changing non-Xen code
> >> >> > paths. You are welcome to suggest improvements or alternative
> >> >> > implementations.
> >> >>
> >> >> Andy Lutomirski also looked at this. Andy what happened to this work?
> >> >>
> >> >> David
> >> >
> >> > The approach there was to try and convert all virtio to use DMA
> >> > API unconditionally.
> >> > This is reasonable if there's a way for devices to request
> >> > 1:1 mappings individually.
> >> > As that is currently missing, that patchset can not be merged yet.
> >> >
> >>
> >> I still don't understand why *devices* need the ability to request
> >> anything in particular. In current kernels, devices that don't have
> >> an iommu work (and there's no choice about 1:1 or otherwise) and
> >> devices that have an iommu fail spectacularly. With the patches,
> >> devices that don't have an iommu continue to work as long as the DMA
> >> API and/or virtio correctly knows that there's no iommu. Devices that
> >> do have an iommu work fine, albeit slower than would be ideal. In my
> >> book, slower than would be ideal is strictly better than crashing.
> >>
> >> The real issue is *detecting* whether there's an iommu, and the string
> >> of bugs in that area (buggy QEMU for the Q35 thing and complete lack
> >> of a solution for PPC and SPARC is indeed a problem).
> >>
> >> I think that we could apply the series ending here:
> >>
> >> https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=virtio_dma&id=ad9d43052da44ce18363c02ea597dde01eeee11b
> >>
> >> and the only regression (performance or functionality) would be that
> >> the buggy Q35 iommu configuration would stop working until someone
> >> fixed it in QEMU. That should be okay -- it's explicitly
> >> experimental. (Xen works with that series applied.) (Actually,
> >> there might be a slight performance regression on PPC due to extra
> >> unused mappings being created. It would be straightforward to hack
> >> around that in one of several ways.)
> >>
> >> Am I missing something?
> >
> > Your changes look plausible and if they fix Xen on virtio I am happy
> > with them. I didn't choose the DMA API approach because, although it
> > looks cleaner, I acknowledge that is a bit invasive.
> >
> > I suggest that the virtio maintainers consider one of the two approaches
> > for inclusion because they fix a real issue.
> >
> > If you would rather avoid the DMA API, then I would be happy to work
> > with you to evolve my current series in a direction of your liking.
> > Please advise on how to proceed.
>
> I would rather use the DMA API, but we need Michael's buy-in for that.
>
> Michael, if I further modify the driver so that even the dma
> allocations don't happen if the in-driver quirk is set, and we set the
> quirk on sparc and ppc but clear it on x86 and presumably all the
> other architectures, would that be okay with you?
>
> --Andy
It's a bit hard to be sure without seeing the patches, but I think so.
Except let's be conservative and set it everywhere except on Xen. We
used physical addresses for many years, at this point we need to
whitelist systems not blacklist systems which do something different.
--
MST
^ permalink raw reply
* Re: [Xen-devel] [PATCH v2 0/3] Fix and cleanup for 32-bit PV sysexit
From: Andy Lutomirski @ 2015-12-15 21:27 UTC (permalink / raw)
To: Andrew Cooper
Cc: linux-kernel@vger.kernel.org, Linux Virtualization, Ingo Molnar,
David Vrabel, Andrew Lutomirski, H. Peter Anvin,
xen-devel@lists.xenproject.org, Boris Ostrovsky, Borislav Petkov,
Thomas Gleixner
In-Reply-To: <56707ACE.9060809@citrix.com>
On Tue, Dec 15, 2015 at 12:40 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 19/11/15 22:07, Andy Lutomirski wrote:
>> On Thu, Nov 19, 2015 at 1:55 PM, Boris Ostrovsky
>> <boris.ostrovsky@oracle.com> wrote:
>>> The first patch fixes Xen PV regression introduced by 32-bit rewrite. Unlike the
>>> earlier version it uses ALTERNATIVE instruction and avoids using xen_sysexit
>>> (and sysret32 in compat mode) pv ops, as suggested by Andy.
>>>
>>> As result of this patch irq_enable_sysexit and usergs_sysret32 pv ops are not
>>> used anymore by anyone and so can be removed.
>> This whole series is:
>>
>> Acked-by: Andy Lutomirski <luto@kernel.org>
>>
>> Now I just have to sucker someone into getting rid of
>> PARAVIRT_ADJUST_EXCEPTION_FRAME (by using stub entries) and the
>> overcomplicated syscall entry stuff. :)
>
> Looking at this, it should be quite easy now.
>
> ALTERNATIVE "", "pop %rcx; %pop %11", X86_FEATURE_XENPV
>
> (Completely untested)
Can't we do one better, though? Generate a pile of stubs that do the
pops and jump into the normal native asm path. Admittedly, that's a
lot more work, and I think that the ALTERNATIVE thing you're
suggesting would be a nice improvement.
>
>> And whoever gets rid of
>> PARAVIRT_ADJUST_EXCEPTION_FRAME gets to wonder why it doesn't crash
>> and burn for NMIs on Xen, since I'm reasonably confident that it can't
>> possibly be correct.
>
> The Xen PV ABI only has a single kernel stack pointer which may be
> registered. There is no equivalent of an IST, so if a second fault
> occurs, it is delivered normally on the current stack.
>
> By the looks of it, the other NMI handling is ambivalent to the fact
> that it isn't really on an IST stack under Xen.
I'll try to find some time to look at it.
--Andy
^ permalink raw reply
* Re: [PATCH] vhost: move is_le setup to the backend
From: Greg Kurz @ 2015-12-16 13:52 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20151112152819.78eb1b4d@bahia.local>
On Thu, 12 Nov 2015 15:28:19 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Thu, 12 Nov 2015 15:46:30 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Fri, Oct 30, 2015 at 12:42:35PM +0100, Greg Kurz wrote:
> > > The vq->is_le field is used to fix endianness when accessing the vring via
> > > the cpu_to_vhost16() and vhost16_to_cpu() helpers in the following cases:
> > >
> > > 1) host is big endian and device is modern virtio
> > >
> > > 2) host has cross-endian support and device is legacy virtio with a different
> > > endianness than the host
> > >
> > > Both cases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the
> > > VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le
> > > is only needed when the backend is active, it was decided to set it at
> > > backend start.
> > >
> > > This is currently done in vhost_init_used()->vhost_init_is_le() but it
> > > obfuscates the core vhost code. This patch moves the is_le setup to a
> > > dedicated function that is called from the backend code.
> > >
> > > Note vhost_net is the only backend that can pass vq->private_data == NULL to
> > > vhost_init_used(), hence the "if (sock)" branch.
> > >
> > > No behaviour change.
> > >
> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >
> > I plan to look at this next week, busy with QEMU 2.5 now.
> >
>
> I don't have any deadline for this since this is only a cleanup tentative.
>
> Thanks.
>
... but ping anyway since it was a month ago :)
Cheers.
--
Greg
> > > ---
> > > drivers/vhost/net.c | 6 ++++++
> > > drivers/vhost/scsi.c | 3 +++
> > > drivers/vhost/test.c | 2 ++
> > > drivers/vhost/vhost.c | 12 +++++++-----
> > > drivers/vhost/vhost.h | 1 +
> > > 5 files changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 9eda69e40678..d6319cb2664c 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -917,6 +917,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> > >
> > > vhost_net_disable_vq(n, vq);
> > > vq->private_data = sock;
> > > +
> > > + if (sock)
> > > + vhost_set_is_le(vq);
> > > + else
> > > + vq->is_le = virtio_legacy_is_little_endian();
> > > +
> > > r = vhost_init_used(vq);
> > > if (r)
> > > goto err_used;
> > > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > > index e25a23692822..e2644a301fa5 100644
> > > --- a/drivers/vhost/scsi.c
> > > +++ b/drivers/vhost/scsi.c
> > > @@ -1276,6 +1276,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
> > > vq = &vs->vqs[i].vq;
> > > mutex_lock(&vq->mutex);
> > > vq->private_data = vs_tpg;
> > > +
> > > + vhost_set_is_le(vq);
> > > +
> > > vhost_init_used(vq);
> > > mutex_unlock(&vq->mutex);
> > > }
> > > diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> > > index f2882ac98726..b1c7df502211 100644
> > > --- a/drivers/vhost/test.c
> > > +++ b/drivers/vhost/test.c
> > > @@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int test)
> > > oldpriv = vq->private_data;
> > > vq->private_data = priv;
> > >
> > > + vhost_set_is_le(vq);
> > > +
> > > r = vhost_init_used(&n->vqs[index]);
> > >
> > > mutex_unlock(&vq->mutex);
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index eec2f11809ff..6be863dcbd13 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -113,6 +113,12 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > > }
> > > #endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
> > >
> > > +void vhost_set_is_le(struct vhost_virtqueue *vq)
> > > +{
> > > + vhost_init_is_le(vq);
> > > +}
> > > +EXPORT_SYMBOL_GPL(vhost_set_is_le);
> > > +
> > > static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
> > > poll_table *pt)
> > > {
> > > @@ -1156,12 +1162,8 @@ int vhost_init_used(struct vhost_virtqueue *vq)
> > > {
> > > __virtio16 last_used_idx;
> > > int r;
> > > - if (!vq->private_data) {
> > > - vq->is_le = virtio_legacy_is_little_endian();
> > > + if (!vq->private_data)
> > > return 0;
> > > - }
> > > -
> > > - vhost_init_is_le(vq);
> > >
> > > r = vhost_update_used_flags(vq);
> > > if (r)
> > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > index 4772862b71a7..8a62041959fe 100644
> > > --- a/drivers/vhost/vhost.h
> > > +++ b/drivers/vhost/vhost.h
> > > @@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
> > >
> > > int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> > > unsigned int log_num, u64 len);
> > > +void vhost_set_is_le(struct vhost_virtqueue *vq);
> > >
> > > #define vq_err(vq, fmt, ...) do { \
> > > pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
> >
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
^ permalink raw reply
* [PATCH] virtio: make find_vqs() checkpatch.pl-friendly
From: Stefan Hajnoczi @ 2015-12-17 8:53 UTC (permalink / raw)
To: virtualization; +Cc: linux-kernel, Stefan Hajnoczi, Michael S. Tsirkin
checkpatch.pl wants arrays of strings declared as follows:
static const char * const names[] = { "vq-1", "vq-2", "vq-3" };
Currently the find_vqs() function takes a const char *names[] argument
so passing checkpatch.pl's const char * const names[] results in a
compiler error due to losing the second const.
This patch adjusts the find_vqs() prototype and updates all virtio
transports. This makes it possible for virtio_balloon.c, virtio_input.c,
virtgpu_kms.c, and virtio_rpmsg_bus.c to use the checkpatch.pl-friendly
type.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Compile-tested drivers/virtio/ but not the more exotic source files in this
patch.
drivers/gpu/drm/virtio/virtgpu_kms.c | 2 +-
drivers/misc/mic/card/mic_virtio.c | 2 +-
drivers/remoteproc/remoteproc_virtio.c | 2 +-
drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
drivers/s390/virtio/kvm_virtio.c | 2 +-
drivers/s390/virtio/virtio_ccw.c | 2 +-
drivers/virtio/virtio_balloon.c | 2 +-
drivers/virtio/virtio_input.c | 2 +-
drivers/virtio/virtio_mmio.c | 2 +-
drivers/virtio/virtio_pci_common.c | 4 ++--
drivers/virtio/virtio_pci_common.h | 2 +-
drivers/virtio/virtio_pci_modern.c | 2 +-
include/linux/virtio_config.h | 2 +-
13 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 06496a1..4150873 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -130,7 +130,7 @@ int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
static vq_callback_t *callbacks[] = {
virtio_gpu_ctrl_ack, virtio_gpu_cursor_ack
};
- static const char *names[] = { "control", "cursor" };
+ static const char * const names[] = { "control", "cursor" };
struct virtio_gpu_device *vgdev;
/* this will expand later */
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index e486a0c..f6ed57d 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -311,7 +311,7 @@ unmap:
static int mic_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[])
+ const char * const names[])
{
struct mic_vdev *mvdev = to_micvdev(vdev);
struct mic_device_ctrl __iomem *dc = mvdev->dc;
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index e1a1023..e44872f 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -147,7 +147,7 @@ static void rproc_virtio_del_vqs(struct virtio_device *vdev)
static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[])
+ const char * const names[])
{
struct rproc *rproc = vdev_to_rproc(vdev);
int i, ret;
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 73354ee..1fcd27c 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -945,7 +945,7 @@ static void rpmsg_ns_cb(struct rpmsg_channel *rpdev, void *data, int len,
static int rpmsg_probe(struct virtio_device *vdev)
{
vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
- const char *names[] = { "input", "output" };
+ static const char * const names[] = { "input", "output" };
struct virtqueue *vqs[2];
struct virtproc_info *vrp;
void *bufs_va;
diff --git a/drivers/s390/virtio/kvm_virtio.c b/drivers/s390/virtio/kvm_virtio.c
index 53fb975..1d060fd 100644
--- a/drivers/s390/virtio/kvm_virtio.c
+++ b/drivers/s390/virtio/kvm_virtio.c
@@ -255,7 +255,7 @@ static void kvm_del_vqs(struct virtio_device *vdev)
static int kvm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[])
+ const char * const names[])
{
struct kvm_device *kdev = to_kvmdev(vdev);
int i;
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index b2a1a81..5a433ae 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -635,7 +635,7 @@ out:
static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[])
+ const char * const names[])
{
struct virtio_ccw_device *vcdev = to_vc_device(vdev);
unsigned long *indicatorp = NULL;
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 7efc329..d0f0817c 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -388,7 +388,7 @@ static int init_vqs(struct virtio_balloon *vb)
{
struct virtqueue *vqs[3];
vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
- const char *names[] = { "inflate", "deflate", "stats" };
+ static const char * const names[] = { "inflate", "deflate", "stats" };
int err, nvqs;
/*
diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
index c96944b..350a2a5 100644
--- a/drivers/virtio/virtio_input.c
+++ b/drivers/virtio/virtio_input.c
@@ -170,7 +170,7 @@ static int virtinput_init_vqs(struct virtio_input *vi)
struct virtqueue *vqs[2];
vq_callback_t *cbs[] = { virtinput_recv_events,
virtinput_recv_status };
- static const char *names[] = { "events", "status" };
+ static const char * const names[] = { "events", "status" };
int err;
err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names);
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index f499d9d..745c6ee 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -482,7 +482,7 @@ error_available:
static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[])
+ const char * const names[])
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
unsigned int irq = platform_get_irq(vm_dev->pdev, 0);
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 78f804a..36205c2 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -296,7 +296,7 @@ void vp_del_vqs(struct virtio_device *vdev)
static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[],
+ const char * const names[],
bool use_msix,
bool per_vq_vectors)
{
@@ -376,7 +376,7 @@ error_find:
int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[])
+ const char * const names[])
{
int err;
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index b976d96..2cc2522 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -139,7 +139,7 @@ void vp_del_vqs(struct virtio_device *vdev);
int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[]);
+ const char * const names[]);
const char *vp_bus_name(struct virtio_device *vdev);
/* Setup the affinity for a virtqueue:
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 8e5cf19..c0c11fa 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -418,7 +418,7 @@ err_new_queue:
static int vp_modern_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[])
+ const char * const names[])
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
struct virtqueue *vq;
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index e5ce8ab..6e6cb0c 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -70,7 +70,7 @@ struct virtio_config_ops {
int (*find_vqs)(struct virtio_device *, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[]);
+ const char * const names[]);
void (*del_vqs)(struct virtio_device *);
u64 (*get_features)(struct virtio_device *vdev);
int (*finalize_features)(struct virtio_device *vdev);
--
2.5.0
^ permalink raw reply related
* [PATCH] virtio: use smp_load_acquire/smp_store_release
From: Michael S. Tsirkin @ 2015-12-17 10:29 UTC (permalink / raw)
To: linux-kernel; +Cc: qemu-devel, virtualization
virtio ring entries have exactly the acquire/release
semantics:
- reading used index acquires a ring entry from host
- updating the available index releases it to host
Thus when using weak barriers and building for SMP (as most people
do), smp_load_acquire and smp_store_release will do exactly
the right thing to synchronize with the host.
In fact, QEMU already uses __atomic_thread_fence(__ATOMIC_ACQUIRE) and
__atomic_thread_fence(__ATOMIC_RELEASE);
Documentation/circular-buffers.txt suggests smp_load_acquire and
smp_store_release for head and tail updates.
Since we have to worry about UP and strong barrier users,
let's add APIs to wrap these, and use in virtio_ring.c
It is tempting to use this in vhost as well,
this needs a bit more work to make acquire/release macros
work on __user pointers.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_ring.h | 30 ++++++++++++++++++++++++++++++
drivers/virtio/virtio_ring.c | 20 ++++++++++----------
2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 8e50888..0135c16 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -47,6 +47,36 @@ static inline void virtio_wmb(bool weak_barriers)
wmb();
}
+static inline __virtio16 virtio_load_acquire(bool weak_barriers, __virtio16 *p)
+{
+ if (!weak_barriers) {
+ rmb();
+ return READ_ONCE(*p);
+ }
+#ifdef CONFIG_SMP
+ return smp_load_acquire(p);
+#else
+ dma_rmb();
+ return READ_ONCE(*p);
+#endif
+}
+
+static inline void virtio_store_release(bool weak_barriers,
+ __virtio16 *p, __virtio16 v)
+{
+ if (!weak_barriers) {
+ wmb();
+ WRITE_ONCE(*p, v);
+ return;
+ }
+#ifdef CONFIG_SMP
+ smp_store_release(p, v);
+#else
+ dma_wmb();
+ WRITE_ONCE(*p, v);
+#endif
+}
+
struct virtio_device;
struct virtqueue;
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index ee663c4..f822cab 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -244,11 +244,11 @@ static inline int virtqueue_add(struct virtqueue *_vq,
avail = vq->avail_idx_shadow & (vq->vring.num - 1);
vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
+ vq->avail_idx_shadow++;
/* Descriptors and available array need to be set before we expose the
* new available array entries. */
- virtio_wmb(vq->weak_barriers);
- vq->avail_idx_shadow++;
- vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
+ virtio_store_release(vq->weak_barriers, &vq->vring.avail->idx,
+ cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow));
vq->num_added++;
pr_debug("Added buffer head %i to %p\n", head, vq);
@@ -453,9 +453,10 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
vq->vq.num_free++;
}
-static inline bool more_used(const struct vring_virtqueue *vq)
+static inline
+bool more_used(const struct vring_virtqueue *vq, __virtio16 used_idx)
{
- return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev, vq->vring.used->idx);
+ return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev, used_idx);
}
/**
@@ -488,15 +489,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
return NULL;
}
- if (!more_used(vq)) {
+ /* Only get used array entries after they have been exposed by host. */
+ if (!more_used(vq, virtio_load_acquire(vq->weak_barriers,
+ &vq->vring.used->idx))) {
pr_debug("No more buffers in queue\n");
END_USE(vq);
return NULL;
}
- /* Only get used array entries after they have been exposed by host. */
- virtio_rmb(vq->weak_barriers);
-
last_used = (vq->last_used_idx & (vq->vring.num - 1));
i = virtio32_to_cpu(_vq->vdev, vq->vring.used->ring[last_used].id);
*len = virtio32_to_cpu(_vq->vdev, vq->vring.used->ring[last_used].len);
@@ -704,7 +704,7 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- if (!more_used(vq)) {
+ if (!more_used(vq, vq->vring.used->idx)) {
pr_debug("virtqueue interrupt with no work for %p\n", vq);
return IRQ_NONE;
}
--
MST
^ permalink raw reply related
* [PATCH] virtio_ring: use smp_store_mb
From: Michael S. Tsirkin @ 2015-12-17 10:32 UTC (permalink / raw)
To: linux-kernel; +Cc: qemu-devel, virtualization
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.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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?
include/linux/virtio_ring.h | 14 ++++++++++++++
drivers/virtio/virtio_ring.c | 15 +++++++++------
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 0135c16..8912189 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -47,6 +47,20 @@ static inline void virtio_wmb(bool weak_barriers)
wmb();
}
+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
+#endif
+ {
+ WRITE_ONCE(*p, v);
+ mb();
+ }
+}
+
static inline __virtio16 virtio_load_acquire(bool weak_barriers, __virtio16 *p)
{
if (!weak_barriers) {
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index f822cab..b0aea67 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -517,10 +517,10 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
/* If we expect an interrupt for the next entry, tell host
* by writing event index and flush out the write before
* the read in the next get_buf call. */
- if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
- vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, vq->last_used_idx);
- virtio_mb(vq->weak_barriers);
- }
+ if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
+ virtio_store_mb(vq->weak_barriers,
+ &vring_used_event(&vq->vring),
+ cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
#ifdef DEBUG
vq->last_add_time_valid = false;
@@ -653,8 +653,11 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
}
/* TODO: tune this threshold */
bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
- vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, vq->last_used_idx + bufs);
- virtio_mb(vq->weak_barriers);
+
+ virtio_store_mb(vq->weak_barriers,
+ &vring_used_event(&vq->vring),
+ cpu_to_virtio16(_vq->vdev, vq->last_used_idx + bufs));
+
if (unlikely((u16)(virtio16_to_cpu(_vq->vdev, vq->vring.used->idx) - vq->last_used_idx) > bufs)) {
END_USE(vq);
return false;
--
MST
^ permalink raw reply related
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Peter Zijlstra @ 2015-12-17 10:52 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, virtualization
In-Reply-To: <1450347932-16325-1-git-send-email-mst@redhat.com>
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
> +#endif
> + {
> + WRITE_ONCE(*p, v);
> + mb();
> + }
> +}
This is a different barrier depending on SMP, that seems wrong.
smp_mb(), as (should be) used by smp_store_mb() does not provide a
barrier against IO. mb() otoh does.
Since this is virtIO I would expect you always want mb().
^ permalink raw reply
* Re: [PATCH] virtio: use smp_load_acquire/smp_store_release
From: Peter Zijlstra @ 2015-12-17 10:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, virtualization
In-Reply-To: <1450347930-16275-1-git-send-email-mst@redhat.com>
On Thu, Dec 17, 2015 at 12:29:03PM +0200, Michael S. Tsirkin wrote:
> +static inline __virtio16 virtio_load_acquire(bool weak_barriers, __virtio16 *p)
> +{
> + if (!weak_barriers) {
> + rmb();
> + return READ_ONCE(*p);
> + }
> +#ifdef CONFIG_SMP
> + return smp_load_acquire(p);
> +#else
> + dma_rmb();
> + return READ_ONCE(*p);
> +#endif
> +}
This too is wrong. Look for example at arm.
dma_rmb() is dmb(osh), while the smp_mb() used by smp_load_acquire() is
dmb(ish). They order completely different types of memory accesses.
Also, load_acquire() is first load, then barrier, and an ACQUIRE barrier
at that, not a READ barrier.
So your #else branch should look something like:
var = READ_ONCE(*p);
dma_mb();
return var;
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Peter Zijlstra @ 2015-12-17 11:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, virtualization
In-Reply-To: <1450347932-16325-1-git-send-email-mst@redhat.com>
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 recently established that xchg is indeed faster than mfence
on at least recent machines, see:
lkml.kernel.org/r/CA+55aFynbkeuUGs9s-q+fLY6MeRBA6MjEyWWbbe7A5AaqsAknw@mail.gmail.com
> +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
> +#endif
> + {
> + WRITE_ONCE(*p, v);
> + mb();
> + }
> +}
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().
As previously stated, smp_mb() does not cover the same memory domains as
dma_mb() would.
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Michael S. Tsirkin @ 2015-12-17 13:16 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: qemu-devel, linux-kernel, virtualization
In-Reply-To: <20151217105238.GA6375@twins.programming.kicks-ass.net>
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 (weak_barriers)
> > + smp_store_mb(*p, v);
> > + else
> > +#endif
> > + {
> > + WRITE_ONCE(*p, v);
> > + mb();
> > + }
> > +}
>
> This is a different barrier depending on SMP, that seems wrong.
Of course it's wrong in the sense that it's
suboptimal on UP. What we would really like is to
have, on UP, exactly the same barrier as on SMP.
This is because a UP guest can run on an SMP host.
But Linux doesn't provide this ability: if CONFIG_SMP is
not defined is optimizes most barriers out to a
compiler barrier.
Consider for example x86: what we want is xchg (NOT
mfence - see below for why) but if built without CONFIG_SMP
smp_store_mb does not include this.
> smp_mb(), as (should be) used by smp_store_mb() does not provide a
> barrier against IO. mb() otoh does.
>
> Since this is virtIO I would expect you always want mb().
No because it's VIRTio not real io :) It just switches to the hyprevisor
mode - kind of like a function call really.
The weak_barriers flag is cleared for when it's used
with real devices with real IO.
All this is explained in some detail at the top of
include/linux/virtio.h
--
MST
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Michael S. Tsirkin @ 2015-12-17 13:26 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: qemu-devel, linux-kernel, virtualization
In-Reply-To: <20151217112222.GC6375@twins.programming.kicks-ass.net>
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?
>
> Would help if you Cc people who would actually know this :-)
Good point. Glad you still saw this. Thanks!
> Yes, we've recently established that xchg is indeed faster than mfence
> on at least recent machines, see:
>
> lkml.kernel.org/r/CA+55aFynbkeuUGs9s-q+fLY6MeRBA6MjEyWWbbe7A5AaqsAknw@mail.gmail.com
>
> > +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
> > +#endif
> > + {
> > + WRITE_ONCE(*p, v);
> > + mb();
> > + }
> > +}
>
> 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 consistent.
To simplify things, you may consider things before
the optimization brought in by
commit 9e1a27ea42691429e31f158cce6fc61bc79bb2e9
Author: Alexander Duyck <alexander.h.duyck@redhat.com>
Date: Mon Apr 13 21:03:49 2015 +0930
virtio_ring: Update weak barriers to use dma_wmb/rmb
> As previously stated, smp_mb() does not cover the same memory domains as
> dma_mb() would.
I know. We used to consistently do the right thing on SMP,
but on UP Linux does not have good portable APIs for us
to use. So we hack around with what's available which is
typically stronger than what's really needed.
I guess no one cares about UP that much.
The Alexander came and tried to optimize UP using
dma_wmb/dma_rmb. I guess he did not find dma_mb so
left it as is.
Maybe we should make virtio depend on SMP, and be done with it,
but the amount of code to maintain !SMP is small enough
to not be worth the potential pain to users (if any).
--
MST
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Peter Zijlstra @ 2015-12-17 13:57 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, virtualization
In-Reply-To: <20151217131554-mutt-send-email-mst@redhat.com>
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,
> > > + __virtio16 *p, __virtio16 v)
> > > +{
> > > +#ifdef CONFIG_SMP
> > > + if (weak_barriers)
> > > + smp_store_mb(*p, v);
> > > + else
> > > +#endif
> > > + {
> > > + WRITE_ONCE(*p, v);
> > > + mb();
> > > + }
> > > +}
> >
> > This is a different barrier depending on SMP, that seems wrong.
>
> Of course it's wrong in the sense that it's
> suboptimal on UP. What we would really like is to
> have, on UP, exactly the same barrier as on SMP.
> This is because a UP guest can run on an SMP host.
>
> But Linux doesn't provide this ability: if CONFIG_SMP is
> not defined is optimizes most barriers out to a
> compiler barrier.
>
> Consider for example x86: what we want is xchg (NOT
> mfence - see below for why) but if built without CONFIG_SMP
> smp_store_mb does not include this.
You could of course go fix that instead of mutilating things into
sort-of functional state.
>
>
> > smp_mb(), as (should be) used by smp_store_mb() does not provide a
> > barrier against IO. mb() otoh does.
> >
> > Since this is virtIO I would expect you always want mb().
>
> No because it's VIRTio not real io :) It just switches to the hyprevisor
> mode - kind of like a function call really.
> The weak_barriers flag is cleared for when it's used
> with real devices with real IO.
>
>
> All this is explained in some detail at the top of
> include/linux/virtio.h
I did read that, it didn't make any sense wrt the code below it.
For instance it seems to imply weak_barriers is for smp like stuff while
!weak_barriers is for actual devices.
But then you go use dma_*mb() ops, which are specifially for devices
only for weak_barrier.
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Peter Zijlstra @ 2015-12-17 14:02 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, virtualization
In-Reply-To: <20151217151705-mutt-send-email-mst@redhat.com>
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 consistent.
>
> To simplify things, you may consider things before
> the optimization brought in by
> commit 9e1a27ea42691429e31f158cce6fc61bc79bb2e9
> Author: Alexander Duyck <alexander.h.duyck@redhat.com>
> Date: Mon Apr 13 21:03:49 2015 +0930
>
> virtio_ring: Update weak barriers to use dma_wmb/rmb
That commit doesn't make any sense. dma_*mb() explicitly does _NOT_
cover the smp_*mb() part.
Again, look at the ARM definitions, the smp_*mb() primitives use the
inner coherence stuff, while the dma_*mb() primitives use the outer
coherent stuff.
the *mb() primitives cover both.
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Michael S. Tsirkin @ 2015-12-17 14:33 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: qemu-devel, linux-kernel, Alexander Duyck, virtualization
In-Reply-To: <20151217135726.GA6344@twins.programming.kicks-ass.net>
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 void virtio_store_mb(bool weak_barriers,
> > > > + __virtio16 *p, __virtio16 v)
> > > > +{
> > > > +#ifdef CONFIG_SMP
> > > > + if (weak_barriers)
> > > > + smp_store_mb(*p, v);
> > > > + else
> > > > +#endif
> > > > + {
> > > > + WRITE_ONCE(*p, v);
> > > > + mb();
> > > > + }
> > > > +}
> > >
> > > This is a different barrier depending on SMP, that seems wrong.
> >
> > Of course it's wrong in the sense that it's
> > suboptimal on UP. What we would really like is to
> > have, on UP, exactly the same barrier as on SMP.
> > This is because a UP guest can run on an SMP host.
> >
> > But Linux doesn't provide this ability: if CONFIG_SMP is
> > not defined is optimizes most barriers out to a
> > compiler barrier.
> >
> > Consider for example x86: what we want is xchg (NOT
> > mfence - see below for why) but if built without CONFIG_SMP
> > smp_store_mb does not include this.
>
> 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?
> >
> >
> > > smp_mb(), as (should be) used by smp_store_mb() does not provide a
> > > barrier against IO. mb() otoh does.
> > >
> > > Since this is virtIO I would expect you always want mb().
> >
> > No because it's VIRTio not real io :) It just switches to the hyprevisor
> > mode - kind of like a function call really.
> > The weak_barriers flag is cleared for when it's used
> > with real devices with real IO.
> >
> >
> > All this is explained in some detail at the top of
> > include/linux/virtio.h
>
> I did read that, it didn't make any sense wrt the code below it.
>
> For instance it seems to imply weak_barriers is for smp like stuff while
> !weak_barriers is for actual devices.
>
> But then you go use dma_*mb() ops, which are specifially for devices
> only for weak_barrier.
Yes the dma_*mb thing is a kind of an interface misuse.
It's an optimization for UP introduced here:
commit 9e1a27ea42691429e31f158cce6fc61bc79bb2e9
Author: Alexander Duyck <alexander.h.duyck@redhat.com>
Date: Mon Apr 13 21:03:49 2015 +0930
virtio_ring: Update weak barriers to use dma_wmb/rmb
This change makes it so that instead of using smp_wmb/rmb which varies
depending on the kernel configuration we can can use dma_wmb/rmb which for
most architectures should be equal to or slightly more strict than
smp_wmb/rmb.
The advantage to this is that these barriers are available to uniprocessor
builds as well so the performance should improve under such a
configuration.
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
but given the confusion this causes, I'm inclined to revert
this, and later switch to for cleaner API if that
appears. Cc Alexander (at the new address).
--
MST
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Michael S. Tsirkin @ 2015-12-17 14:34 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: qemu-devel, linux-kernel, Alexander Duyck, virtualization
In-Reply-To: <20151217140212.GB6344@twins.programming.kicks-ass.net>
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().
> >
> > It's a hack really. I think I'll clean it up a bit to
> > make it more consistent.
> >
> > To simplify things, you may consider things before
> > the optimization brought in by
> > commit 9e1a27ea42691429e31f158cce6fc61bc79bb2e9
> > Author: Alexander Duyck <alexander.h.duyck@redhat.com>
> > Date: Mon Apr 13 21:03:49 2015 +0930
> >
> > virtio_ring: Update weak barriers to use dma_wmb/rmb
>
> That commit doesn't make any sense. dma_*mb() explicitly does _NOT_
> cover the smp_*mb() part.
>
> Again, look at the ARM definitions, the smp_*mb() primitives use the
> inner coherence stuff, while the dma_*mb() primitives use the outer
> coherent stuff.
Does outer coherent imply inner coherent?
> the *mb() primitives cover both.
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Peter Zijlstra @ 2015-12-17 14:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, linux-kernel, Alexander Duyck, virtualization
In-Reply-To: <20151217161124-mutt-send-email-mst@redhat.com>
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.
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.
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.
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Michael S. Tsirkin @ 2015-12-17 14:43 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: qemu-devel, linux-kernel, Alexander Duyck, virtualization
In-Reply-To: <20151217143910.GD6344@twins.programming.kicks-ass.net>
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.
>
> 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.
>
> 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.
Thanks, will take a stub at this.
^ permalink raw reply
* Re: [PATCH] virtio: use smp_load_acquire/smp_store_release
From: Michael S. Tsirkin @ 2015-12-17 14:47 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: qemu-devel, linux-kernel, virtualization
In-Reply-To: <20151217105827.GB6375@twins.programming.kicks-ass.net>
On Thu, Dec 17, 2015 at 11:58:27AM +0100, Peter Zijlstra wrote:
> On Thu, Dec 17, 2015 at 12:29:03PM +0200, Michael S. Tsirkin wrote:
> > +static inline __virtio16 virtio_load_acquire(bool weak_barriers, __virtio16 *p)
> > +{
> > + if (!weak_barriers) {
> > + rmb();
> > + return READ_ONCE(*p);
> > + }
> > +#ifdef CONFIG_SMP
> > + return smp_load_acquire(p);
> > +#else
> > + dma_rmb();
> > + return READ_ONCE(*p);
> > +#endif
> > +}
>
> This too is wrong. Look for example at arm.
>
> dma_rmb() is dmb(osh), while the smp_mb() used by smp_load_acquire() is
> dmb(ish). They order completely different types of memory accesses.
>
> Also, load_acquire() is first load, then barrier, and an ACQUIRE barrier
> at that, not a READ barrier.
Yes - it just so happens that READ barrier is enough for where I use it
for virtio.
I really just need virtio_load_acquire that fences reads,
I don't care what happens to writes.
Given the confusion, maybe virtio_load_acquire is a bad name though.
Donnu what a good name would be.
virtio_load_acquire_rmb and virtio_store_release_wmb?
> So your #else branch should look something like:
>
> var = READ_ONCE(*p);
> dma_mb();
> return var;
>
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Peter Zijlstra @ 2015-12-17 15:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Will Deacon, qemu-devel, Alexander Duyck, linux-kernel,
virtualization
In-Reply-To: <20151217163412-mutt-send-email-mst@redhat.com>
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 <alexander.h.duyck@redhat.com>
> > > Date: Mon Apr 13 21:03:49 2015 +0930
> > >
> > > virtio_ring: Update weak barriers to use dma_wmb/rmb
> >
> > That commit doesn't make any sense. dma_*mb() explicitly does _NOT_
> > cover the smp_*mb() part.
> >
> > Again, look at the ARM definitions, the smp_*mb() primitives use the
> > inner coherence stuff, while the dma_*mb() primitives use the outer
> > coherent stuff.
>
> Does outer coherent imply inner coherent?
>
> > the *mb() primitives cover both.
I do not think so, but lets add Will, he dreams this stuff.
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Will Deacon @ 2015-12-17 15:52 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael S. Tsirkin, linux-kernel, qemu-devel, Alexander Duyck,
virtualization
In-Reply-To: <20151217150917.GP6373@twins.programming.kicks-ass.net>
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: Alexander Duyck <alexander.h.duyck@redhat.com>
> > > > Date: Mon Apr 13 21:03:49 2015 +0930
> > > >
> > > > virtio_ring: Update weak barriers to use dma_wmb/rmb
> > >
> > > That commit doesn't make any sense. dma_*mb() explicitly does _NOT_
> > > cover the smp_*mb() part.
> > >
> > > Again, look at the ARM definitions, the smp_*mb() primitives use the
> > > inner coherence stuff, while the dma_*mb() primitives use the outer
> > > coherent stuff.
> >
> > Does outer coherent imply inner coherent?
> >
> > > the *mb() primitives cover both.
>
> I do not think so, but lets add Will, he dreams this stuff.
Right, and I don't sleep well these days.
Anyway, the outer-shareable domain (osh) is a superset of the
inner-shareable domain (ish). The inner-shareable domain contains the
CPUs and any peripherals that you and I would call "cache coherent". The
outer-shareable domain extends this to cover a strange set of "less cache
coherent" devices, which we would just call "not cache coherent" for the
purposes of Linux. Normal, non-cacheable memory (i.e. the memory type we
use for non-coherent DMA buffers) is outer-shareable.
Since the barrier macros don't know if the device is coherent or not, we
use the stronger semantics of outer-shareable.
I've not been following the thread, but I reckon we could add dma_mb()
(as dmb(osh) on arm), then use that to build dma_load_acquire and
dma_store_release accessors. On arm64, we could probably use the
acquire/release instructions directly, since they inherit the shareability
domain of the address (which has the nice property of being inner-shareable
for coherent devices).
The massive pain with adding new accessors is defining the semantics.
memory-barriers.txt is already lacking for the CPU side, and we're
struggling to express the kind of transitivity guarantees we provide
today, let alone with new primitives :(
Will
^ permalink raw reply
* Re: [PATCH] virtio_ring: use smp_store_mb
From: Michael S. Tsirkin @ 2015-12-17 19:21 UTC (permalink / raw)
To: Will Deacon
Cc: Peter Zijlstra, qemu-devel, Alexander Duyck, linux-kernel,
virtualization
In-Reply-To: <20151217155224.GA24108@arm.com>
On Thu, Dec 17, 2015 at 03:52:24PM +0000, 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 9e1a27ea42691429e31f158cce6fc61bc79bb2e9
> > > > > Author: Alexander Duyck <alexander.h.duyck@redhat.com>
> > > > > Date: Mon Apr 13 21:03:49 2015 +0930
> > > > >
> > > > > virtio_ring: Update weak barriers to use dma_wmb/rmb
> > > >
> > > > That commit doesn't make any sense. dma_*mb() explicitly does _NOT_
> > > > cover the smp_*mb() part.
> > > >
> > > > Again, look at the ARM definitions, the smp_*mb() primitives use the
> > > > inner coherence stuff, while the dma_*mb() primitives use the outer
> > > > coherent stuff.
> > >
> > > Does outer coherent imply inner coherent?
> > >
> > > > the *mb() primitives cover both.
> >
> > I do not think so, but lets add Will, he dreams this stuff.
>
> Right, and I don't sleep well these days.
>
> Anyway, the outer-shareable domain (osh) is a superset of the
> inner-shareable domain (ish). The inner-shareable domain contains the
> CPUs and any peripherals that you and I would call "cache coherent". The
> outer-shareable domain extends this to cover a strange set of "less cache
> coherent" devices, which we would just call "not cache coherent" for the
> purposes of Linux. Normal, non-cacheable memory (i.e. the memory type we
> use for non-coherent DMA buffers) is outer-shareable.
>
> Since the barrier macros don't know if the device is coherent or not, we
> use the stronger semantics of outer-shareable.
>
> I've not been following the thread, but I reckon we could add dma_mb()
> (as dmb(osh) on arm), then use that to build dma_load_acquire and
> dma_store_release accessors. On arm64, we could probably use the
> acquire/release instructions directly, since they inherit the shareability
> domain of the address (which has the nice property of being inner-shareable
> for coherent devices).
>
> The massive pain with adding new accessors is defining the semantics.
> memory-barriers.txt is already lacking for the CPU side, and we're
> struggling to express the kind of transitivity guarantees we provide
> today, let alone with new primitives :(
>
> Will
Well virtio (might) have wanted dma_mb in the past.
But if are adding barrier stuff anyway, we really want
pv_ counterparts to smp_ that do the same on CONFIG_SMP
but don't change when CONFIG_SMP is disabled.
--
MST
^ permalink raw reply
* new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
From: Michael S. Tsirkin @ 2015-12-20 9:25 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Konrad Rzeszutek Wilk, qemu-devel, Alexander Duyck, linux-kernel,
David Vrabel, xen-devel, Boris Ostrovsky, virtualization
In-Reply-To: <20151217143910.GD6344@twins.programming.kicks-ass.net>
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 <mst@redhat.com>
--
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 <linux/interrupt.h>
#include <linux/sched.h>
#include <linux/err.h>
+#include <linux/virtio_ring.h>
#include <xen/xenbus.h>
#include <asm/xen/hypervisor.h>
#include <xen/events.h>
@@ -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
^ permalink raw reply related
* Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
From: Andrew Cooper @ 2015-12-20 17:07 UTC (permalink / raw)
To: Michael S. Tsirkin, Peter Zijlstra
Cc: qemu-devel, Alexander Duyck, linux-kernel, David Vrabel,
xen-devel, Boris Ostrovsky, virtualization
In-Reply-To: <20151220105146-mutt-send-email-mst@redhat.com>
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
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox