* [PATCH 1/2] Connector: Set the CN_NETLINK_USERS correctly
From: K. Y. Srinivasan @ 2011-05-17 22:25 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: K. Y. Srinivasan
The CN_NETLINK_USERS must be set to the highest valid index +1.
Thanks to Evgeniy for pointing this out.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
include/linux/connector.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/connector.h b/include/linux/connector.h
index 7c60d09..f696bcc 100644
--- a/include/linux/connector.h
+++ b/include/linux/connector.h
@@ -44,7 +44,7 @@
#define CN_VAL_DRBD 0x1
#define CN_KVP_IDX 0x9 /* HyperV KVP */
-#define CN_NETLINK_USERS 9
+#define CN_NETLINK_USERS 10 /* Highest index + 1 */
/*
* Maximum connector's message size.
--
1.7.4.1
^ permalink raw reply related
* [PATCH 2/2] Connector: Correctly set the error code in case of success when dispatching receive callbacks
From: K. Y. Srinivasan @ 2011-05-17 22:25 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: K. Y. Srinivasan
In-Reply-To: <1305671138-19443-1-git-send-email-kys@microsoft.com>
The recent changes to the connector code introduced this bug where even
when a callback was invoked, we would return an error resulting in
double freeing of the skb. This patch fixes this bug.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/connector/connector.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index d770058..c36fbe1 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -139,6 +139,7 @@ static int cn_call_callback(struct sk_buff *skb)
spin_unlock_bh(&dev->cbdev->queue_lock);
if (cbq != NULL) {
+ err = 0;
cbq->callback(msg, nsp);
kfree_skb(skb);
cn_queue_release_callback(cbq);
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 2/2] Connector: Correctly set the error code in case of success when dispatching receive callbacks
From: Greg KH @ 2011-05-17 22:31 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: gregkh, linux-kernel, devel, virtualization
In-Reply-To: <1305671138-19443-2-git-send-email-kys@microsoft.com>
On Tue, May 17, 2011 at 03:25:38PM -0700, K. Y. Srinivasan wrote:
> The recent changes to the connector code introduced this bug where even
> when a callback was invoked, we would return an error resulting in
> double freeing of the skb. This patch fixes this bug.
By "recent" what do you mean? Is this something that is in the .39
kernel? Do you have a git commit id that caused this?
Or is this due to changes that are queued up for the .40 merge window?
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 2/2] Connector: Correctly set the error code in case of success when dispatching receive callbacks
From: KY Srinivasan @ 2011-05-17 22:53 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110517223155.GC15741@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, May 17, 2011 6:32 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH 2/2] Connector: Correctly set the error code in case of
> success when dispatching receive callbacks
>
> On Tue, May 17, 2011 at 03:25:38PM -0700, K. Y. Srinivasan wrote:
> > The recent changes to the connector code introduced this bug where even
> > when a callback was invoked, we would return an error resulting in
> > double freeing of the skb. This patch fixes this bug.
>
> By "recent" what do you mean? Is this something that is in the .39
> kernel? Do you have a git commit id that caused this?
>
> Or is this due to changes that are queued up for the .40 merge window?
I just happened to test KVP functionality yesterday on latest bits from your tree (NEXT)
and discovered that whenever something was sent to the kernel from userland on a
connector channel, the system would hang. The following commit appears to be where
this bug was introduced:
commit 04f482faf50535229a5a5c8d629cf963899f857c
Author: Patrick McHardy <kaber@trash.net>
Date: Mon Mar 28 08:39:36 2011 +0000
Since prior to this commit I don't see this problem. In any event the current code is clearly
broken.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 2/2] Connector: Correctly set the error code in case of success when dispatching receive callbacks
From: Greg KH @ 2011-05-17 23:05 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481FB74C@TK5EX14MBXC122.redmond.corp.microsoft.com>
On Tue, May 17, 2011 at 10:53:18PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, May 17, 2011 6:32 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > Subject: Re: [PATCH 2/2] Connector: Correctly set the error code in case of
> > success when dispatching receive callbacks
> >
> > On Tue, May 17, 2011 at 03:25:38PM -0700, K. Y. Srinivasan wrote:
> > > The recent changes to the connector code introduced this bug where even
> > > when a callback was invoked, we would return an error resulting in
> > > double freeing of the skb. This patch fixes this bug.
> >
> > By "recent" what do you mean? Is this something that is in the .39
> > kernel? Do you have a git commit id that caused this?
> >
> > Or is this due to changes that are queued up for the .40 merge window?
>
> I just happened to test KVP functionality yesterday on latest bits from your tree (NEXT)
> and discovered that whenever something was sent to the kernel from userland on a
> connector channel, the system would hang. The following commit appears to be where
> this bug was introduced:
>
> commit 04f482faf50535229a5a5c8d629cf963899f857c
> Author: Patrick McHardy <kaber@trash.net>
> Date: Mon Mar 28 08:39:36 2011 +0000
>
> Since prior to this commit I don't see this problem. In any event the current code is clearly
> broken.
Ok, since this was released in the 2.6.39-rc2 kernel, this needs to get
backported to the .39-stable kernel release as well, I'll mark it as
such when I apply it to the tree.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 06/18] virtio_ring: avail event index interface
From: Rusty Russell @ 2011-05-18 0:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <20110517060052.GB26989@redhat.com>
On Tue, 17 May 2011 09:00:52 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, May 16, 2011 at 03:53:19PM +0930, Rusty Russell wrote:
> > On Sun, 15 May 2011 15:47:27 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Mon, May 09, 2011 at 01:43:15PM +0930, Rusty Russell wrote:
> > > > On Wed, 4 May 2011 23:51:19 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > #define VIRTIO_RING_F_USED_EVENT_IDX 29
> > > > > +/* The Host publishes the avail index for which it expects a kick
> > > > > + * at the end of the used ring. Guest should ignore the used->flags field. */
> > > > > +#define VIRTIO_RING_F_AVAIL_EVENT_IDX 32
> > > >
> > > > Are you really sure we want to separate the two? Seems a little simpler
> > > > to have one bit to mean "we're publishing our threshold". For someone
> > > > implementing this from scratch, it's a little simpler.
> > > >
> > > > Or are there cases where the old style makes more sense?
> > > >
> > > > Thanks,
> > > > Rusty.
> > >
> > > Hmm, it makes debugging easier as each side can disable
> > > publishing separately - I used it all the time when I saw
> > > e.g. networking stuck to guess whether I need to investigate the
> > > interrupt or the exit handling.
> > >
> > > But I'm not hung up on this.
> > >
> > > Let me know pls.
> >
> > If we combine them into one, then these patches no longer depend on
> > the feature bit expansion, which is worthwhile (though I'll take both).
> >
> > Thanks,
> > Rusty.
>
> Yes, I know. But if we do expand feature bits anyway, for debugging
> and profiling if nothing else it's useful to have them separate ...
> If you take both why does the order matter?
Damage control. Then if something breaks, it doesn't break everything.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 09/18] virtio: use avail_event index
From: Rusty Russell @ 2011-05-18 0:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <20110517061031.GC26989@redhat.com>
On Tue, 17 May 2011 09:10:31 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Well one can imagine a driver doing:
>
> while (virtqueue_get_buf()) {
> virtqueue_add_buf()
> }
> virtqueue_kick()
>
> which looks sensible (batch kicks) but might
> process any number of bufs between kicks.
No, we currently only expose the buffers in the kick, so it can only
fill the ring doing that.
We could change that (and maybe that's worth looking at)...
> If we look at drivers closely enough, I think none
> of them do the equivalent of the above, but not 100% sure.
I'm pretty sure we don't have this kind of 'echo' driver yet. Drivers
tend to take OS requests and queue them. The only one which does
anything even partially sophisticated is the net driver...
Thanks,
Rusty.
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 3/3] powerpc-virtio: virtio support introduced (block, network, serial, balloon, 9p-fs), both fullemu and power-kvm
From: David Gibson @ 2011-05-18 3:27 UTC (permalink / raw)
To: Alexander Graf
Cc: Michael S. Tsirkin, Alexey Kardashevskiy,
qemu-devel@nongnu.org Developers, virtualization, Paul Mackerras
In-Reply-To: <CC7C7633-F1F6-4CCC-BC48-F7DC057CB63E@suse.de>
On Tue, May 17, 2011 at 09:06:41AM +0200, Alexander Graf wrote:
> On 17.05.2011, at 08:47, David Gibson wrote:
> > From: Alexey Kardashevskiy <aik@ozlabs.ru>
> >
> > The recently added pseries machine does not currently support PCI
> > emulation. For the (upcoming) kvm case, this is quite difficult to do
> > because the preferred HV mode for the host kernel does not allow MMIO
> > emulation (a hardware limitation).
> >
> > Therefore, to support virtio devices, we implement a new virtio setup
> > protocol for PAPR guests. This is based loosely on the s390 and lguest
> > methods, using the PAPR hcalls for the virtio primitive operations,
> > and the PAPR device tree to advertise the virtio device resources to the
> > guest.
> >
> > This patch includes support for the virtio block, network, serial, and
> > balloon devices, and the 9p filesystem.
[snip]
>
> Before including such a patch, we should really discuss the desired
> interface for virtio on sPAPR. I personally would prefer if we could
> have a generic MMIO hypercall that the guest issues, so that we can
> simply use virtio-pci on sPAPR (and all the other PCI hardware).
Hrm. I'm not thrilled at that idea. We would still need to advertise
in the device tree to use the MMIO hypercall, instead of going direct
- since we will be going direct for things like passthrough PCI
devices under KVM.
> But at the end of the day, the steps should be as follows:
>
> 1) Discuss this on the virtualization ML
Ok, which list do you mean here?
> 2) Send patches for the virtio documentation so the protocol has a spec (which we're lacking for s390 still)
> 3) Implement Linux side, upstream it
I don't see why the Linux guest side needs to go before the qemu
side. You can't use one without the other, so there's no clear order.
> 4) Upstream Qemu side
>
> Since I haven't seen 1-3, I'd like to defer this patch until the
> other points are good :)
Hmm. I'll see what I can do.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH 09/18] virtio: use avail_event index
From: Michael S. Tsirkin @ 2011-05-18 5:43 UTC (permalink / raw)
To: Rusty Russell
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <87tycsn9lt.fsf@rustcorp.com.au>
On Wed, May 18, 2011 at 09:49:42AM +0930, Rusty Russell wrote:
> On Tue, 17 May 2011 09:10:31 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > Well one can imagine a driver doing:
> >
> > while (virtqueue_get_buf()) {
> > virtqueue_add_buf()
> > }
> > virtqueue_kick()
> >
> > which looks sensible (batch kicks) but might
> > process any number of bufs between kicks.
>
> No, we currently only expose the buffers in the kick, so it can only
> fill the ring doing that.
>
> We could change that (and maybe that's worth looking at)...
Yes, I think we should - this way host and guest can process
data in parallel without a kick.
My patchset included that simply because it's one index
less to be confused about.
> > If we look at drivers closely enough, I think none
> > of them do the equivalent of the above, but not 100% sure.
>
> I'm pretty sure we don't have this kind of 'echo' driver yet. Drivers
> tend to take OS requests and queue them. The only one which does
> anything even partially sophisticated is the net driver...
>
> Thanks,
> Rusty.
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 3/3] powerpc-virtio: virtio support introduced (block, network, serial, balloon, 9p-fs), both fullemu and power-kvm
From: Michael S. Tsirkin @ 2011-05-18 8:58 UTC (permalink / raw)
To: Alexander Graf, Alexey Kardashevskiy, Rusty Russell,
qemu-devel@nongnu.org Developers
In-Reply-To: <20110518032745.GB3015@yookeroo.fritz.box>
On Wed, May 18, 2011 at 01:27:45PM +1000, David Gibson wrote:
> On Tue, May 17, 2011 at 09:06:41AM +0200, Alexander Graf wrote:
> > On 17.05.2011, at 08:47, David Gibson wrote:
> > > From: Alexey Kardashevskiy <aik@ozlabs.ru>
> > >
> > > The recently added pseries machine does not currently support PCI
> > > emulation. For the (upcoming) kvm case, this is quite difficult to do
> > > because the preferred HV mode for the host kernel does not allow MMIO
> > > emulation (a hardware limitation).
> > >
> > > Therefore, to support virtio devices, we implement a new virtio setup
> > > protocol for PAPR guests. This is based loosely on the s390 and lguest
> > > methods, using the PAPR hcalls for the virtio primitive operations,
> > > and the PAPR device tree to advertise the virtio device resources to the
> > > guest.
> > >
> > > This patch includes support for the virtio block, network, serial, and
> > > balloon devices, and the 9p filesystem.
> [snip]
> >
> > Before including such a patch, we should really discuss the desired
> > interface for virtio on sPAPR. I personally would prefer if we could
> > have a generic MMIO hypercall that the guest issues, so that we can
> > simply use virtio-pci on sPAPR (and all the other PCI hardware).
>
> Hrm. I'm not thrilled at that idea. We would still need to advertise
> in the device tree to use the MMIO hypercall, instead of going direct
> - since we will be going direct for things like passthrough PCI
> devices under KVM.
>
> > But at the end of the day, the steps should be as follows:
> >
> > 1) Discuss this on the virtualization ML
>
> Ok, which list do you mean here?
>
> > 2) Send patches for the virtio documentation so the protocol has a spec (which we're lacking for s390 still)
> > 3) Implement Linux side, upstream it
>
> I don't see why the Linux guest side needs to go before the qemu
> side. You can't use one without the other, so there's no clear order.
Agree here. I would further argue qemu should go in first,
this is what happens with real devices: first hardware, then
driver. Linux side just needs to be ready and available in the repository
somewhere.
> > 4) Upstream Qemu side
> >
> > Since I haven't seen 1-3, I'd like to defer this patch until the
> > other points are good :)
>
> Hmm. I'll see what I can do.
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 3/3] powerpc-virtio: virtio support introduced (block, network, serial, balloon, 9p-fs), both fullemu and power-kvm
From: Alexander Graf @ 2011-05-18 9:35 UTC (permalink / raw)
To: David Gibson
Cc: Michael S. Tsirkin, Alexey Kardashevskiy,
qemu-devel@nongnu.org Developers, virtualization, Paul Mackerras
In-Reply-To: <20110518032745.GB3015@yookeroo.fritz.box>
On 18.05.2011, at 05:27, David Gibson wrote:
> On Tue, May 17, 2011 at 09:06:41AM +0200, Alexander Graf wrote:
>> On 17.05.2011, at 08:47, David Gibson wrote:
>>> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>
>>> The recently added pseries machine does not currently support PCI
>>> emulation. For the (upcoming) kvm case, this is quite difficult to do
>>> because the preferred HV mode for the host kernel does not allow MMIO
>>> emulation (a hardware limitation).
>>>
>>> Therefore, to support virtio devices, we implement a new virtio setup
>>> protocol for PAPR guests. This is based loosely on the s390 and lguest
>>> methods, using the PAPR hcalls for the virtio primitive operations,
>>> and the PAPR device tree to advertise the virtio device resources to the
>>> guest.
>>>
>>> This patch includes support for the virtio block, network, serial, and
>>> balloon devices, and the 9p filesystem.
> [snip]
>>
>> Before including such a patch, we should really discuss the desired
>> interface for virtio on sPAPR. I personally would prefer if we could
>> have a generic MMIO hypercall that the guest issues, so that we can
>> simply use virtio-pci on sPAPR (and all the other PCI hardware).
>
> Hrm. I'm not thrilled at that idea. We would still need to advertise
> in the device tree to use the MMIO hypercall, instead of going direct
> - since we will be going direct for things like passthrough PCI
> devices under KVM.
Yes, it's not an easy thing to solve. But not having the chance of emulating MMIO robs you off quite some possibilities and reimplementing yet another host abstraction layer for virtio will be more headaches than you can imagine :).
>
>> But at the end of the day, the steps should be as follows:
>>
>> 1) Discuss this on the virtualization ML
>
> Ok, which list do you mean here?
The one I CC'ed :)
>
>> 2) Send patches for the virtio documentation so the protocol has a spec (which we're lacking for s390 still)
>> 3) Implement Linux side, upstream it
>
> I don't see why the Linux guest side needs to go before the qemu
> side. You can't use one without the other, so there's no clear order.
Well, ok. I'd at least like to see the Linux side on the mailing list then before pulling the qemu side :).
>
>> 4) Upstream Qemu side
>>
>> Since I haven't seen 1-3, I'd like to defer this patch until the
>> other points are good :)
>
> Hmm. I'll see what I can do.
Thanks!
Alex
^ permalink raw reply
* [PATCH RFC] vhost: fix enable notify: write out last avail value we saw
From: Michael S. Tsirkin @ 2011-05-18 21:13 UTC (permalink / raw)
To: virtualization
Cc: Krishna Kumar2, habanero, kvm, steved, netdev, Shirley Ma,
linux-kernel, borntraeger, Tom Lendacky, avi
With RX ring and mergeable buffers, vhost-net sometimes
enables notifications when ring is not empty - just
doesn't have enough entries for the incoming packet.
To get event when entries are added in this case,
we should always write out the last index value
we saw into event index field, which is ahead
of the index we have consumed.
Otherwise we get a kick, see that there are not enough
entries in the ring, reenable notifications
but since last used index was not updated
we don't get any more events.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
This bugfix is on top of my patchset, I'm working on addressing
Rusty's comments for that now.
Will push to my git tree event idx branch for everyone's testing pleasure
shortly.
drivers/vhost/vhost.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 2aea4cb..1267a3d 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1438,7 +1438,7 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
return false;
}
} else {
- r = put_user(vq->last_avail_idx, vhost_avail_event(vq));
+ r = put_user(vq->avail_idx, vhost_avail_event(vq));
if (r) {
vq_err(vq, "Failed to update avail event index at %p: %d\n",
vhost_avail_event(vq), r);
--
1.7.5.53.gc233e
^ permalink raw reply related
* [PATCH RFC] virtio_net: fix patch: virtio_net: limit xmit polling
From: Michael S. Tsirkin @ 2011-05-18 22:01 UTC (permalink / raw)
To: rusty, habanero, Shirley Ma, Krishna Kumar2, kvm, steved
Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
The patch virtio_net: limit xmit polling
got the logic reversed: it polled while we had
capacity not while ring was empty.
Fix it up and clean up a bit by using a for loop.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
OK, turns out that patch was borken. Here's
a fix that survived stress test on my box.
Pushed on my branch, I'll send a rebased series
with Rusty's comments addressed ASAP.
drivers/net/virtio_net.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9982bd7..c8cd22d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -514,12 +514,14 @@ static bool free_old_xmit_skbs(struct virtnet_info *vi, int capacity)
struct sk_buff *skb;
unsigned int len;
bool c;
+ int n;
+
/* We try to free up at least 2 skbs per one sent, so that we'll get
* all of the memory back if they are used fast enough. */
- int n = 2;
-
- while ((c = virtqueue_get_capacity(vi->svq) >= capacity) && --n > 0 &&
- (skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
+ for (n = 0;
+ ((c = virtqueue_get_capacity(vi->svq)) < capacity || n < 2) &&
+ ((skb = virtqueue_get_buf(vi->svq, &len)));
+ ++n) {
pr_debug("Sent skb %p\n", skb);
vi->dev->stats.tx_bytes += skb->len;
vi->dev->stats.tx_packets++;
--
1.7.5.53.gc233e
^ permalink raw reply related
* Re: [PATCH 14/18] virtio: add api for delayed callbacks
From: Michael S. Tsirkin @ 2011-05-19 7:24 UTC (permalink / raw)
To: Rusty Russell
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <87boz3dsoe.fsf@rustcorp.com.au>
On Mon, May 16, 2011 at 04:43:21PM +0930, Rusty Russell wrote:
> On Sun, 15 May 2011 15:48:18 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, May 09, 2011 at 03:27:33PM +0930, Rusty Russell wrote:
> > > On Wed, 4 May 2011 23:52:33 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > Add an API that tells the other side that callbacks
> > > > should be delayed until a lot of work has been done.
> > > > Implement using the new used_event feature.
> > >
> > > Since you're going to add a capacity query anyway, why not add the
> > > threshold argument here?
> >
> > I thought that if we keep the API kind of generic
> > there might be more of a chance that future transports
> > will be able to implement it. For example, with an
> > old host we can't commit to a specific index.
>
> No, it's always a hint anyway: you can be notified before the threshold
> is reached. But best make it explicit I think.
>
> Cheers,
> Rusty.
I tried doing that and remembered the real reason I went for this API:
capacity is limited by descriptor table space, not
used ring space: each entry in the used ring frees up multiple entries
in the descriptor ring. Thus the ring can't provide
callback after capacity is N: capacity is only available
after we get bufs.
We could try and make the API pass in the number of freed bufs, however:
- this is not really what virtio-net cares about (it cares about
capacity)
- if the driver passes a number > number of outstanding bufs, it will
never get a callback. So to stay correct the driver will need to
track number of outstanding requests. The simpler API avoids that.
APIs are easy to change so I'm guessing it's not a major blocker:
we can change later when e.g. block tries to
pass in some kind of extra hint: we'll be smarter
about how this API can change then.
Right?
--
MST
^ permalink raw reply
* Re: [PATCH 09/18] virtio: use avail_event index
From: Michael S. Tsirkin @ 2011-05-19 7:27 UTC (permalink / raw)
To: Rusty Russell
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <87tycsn9lt.fsf@rustcorp.com.au>
On Wed, May 18, 2011 at 09:49:42AM +0930, Rusty Russell wrote:
> On Tue, 17 May 2011 09:10:31 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > Well one can imagine a driver doing:
> >
> > while (virtqueue_get_buf()) {
> > virtqueue_add_buf()
> > }
> > virtqueue_kick()
> >
> > which looks sensible (batch kicks) but might
> > process any number of bufs between kicks.
>
> No, we currently only expose the buffers in the kick, so it can only
> fill the ring doing that.
>
> We could change that (and maybe that's worth looking at)...
That's actually what one of the early patches in the series did.
I guess I can try and reorder the patches, I do believe
it makes sense to publish immediately as this way
host can work in parallel with the guest.
> > If we look at drivers closely enough, I think none
> > of them do the equivalent of the above, but not 100% sure.
>
> I'm pretty sure we don't have this kind of 'echo' driver yet. Drivers
> tend to take OS requests and queue them. The only one which does
> anything even partially sophisticated is the net driver...
>
> Thanks,
> Rusty.
I guess I'll just need to do the legwork and check then.
--
MST
^ permalink raw reply
* Re: [PATCH RFC] virtio_net: fix patch: virtio_net: limit xmit polling
From: Rusty Russell @ 2011-05-19 7:30 UTC (permalink / raw)
To: habanero, Shirley Ma, Krishna Kumar2, kvm, steved
Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <20110518220125.GA26835@redhat.com>
On Thu, 19 May 2011 01:01:25 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> The patch virtio_net: limit xmit polling
> got the logic reversed: it polled while we had
> capacity not while ring was empty.
>
> Fix it up and clean up a bit by using a for loop.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> OK, turns out that patch was borken. Here's
> a fix that survived stress test on my box.
> Pushed on my branch, I'll send a rebased series
> with Rusty's comments addressed ASAP.
Normally you would have missed the merge window by now, but I'd really
like this stuff in, so I'm holding it open for this. I want these patches
in linux-next for at least a few days before I push them.
If you think we're not close enough, please tell me and I'll push
the rest of the virtio patches to Linus now.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] arch/tile: add /proc/tile, /proc/sys/tile, and a sysfs cpu attribute
From: Arnd Bergmann @ 2011-05-19 15:22 UTC (permalink / raw)
To: Chris Metcalf, virtualization; +Cc: linux-kernel
In-Reply-To: <4DD5334D.4060800@tilera.com>
(adding virtualization mailing list)
On Thursday 19 May 2011, Chris Metcalf wrote:
> On 5/19/2011 9:41 AM, Arnd Bergmann wrote:
> >> /proc/tile/hvconfig
> >> Detailed configuration description of the hypervisor config
>
> I'm concerned about moving this one out of /proc, since it's just (copious)
> free text. An "hvconfig" (hypervisor config) file describes hypervisor
> driver "dedicated tiles" that run things like network packet or PCIe
> ingress/egress processing, etc. In addition it lists hypervisor driver
> options, boot flags for the kernel, etc, all kinds of things -- and you
> can't really guarantee that it will fit on a 4KB page, though in practice
> it usually does. The hypervisor reads this file from the boot stream when
> it boots, and then makes it available to Linux not for Linux's use, or even
> for programmatic userspace use, but just for end users to be able to review
> and verify that the configuration they think they booted is really what
> they got, for customer remote debugging, etc. The "remote debugging"
> aspect makes truncation to page size a particularly worrisome idea.
Since it's not the kernel that is imposing the format here, you could
make it a binary sysfs attribute, which works in the same way as
a proc file and does not have the size limitations.
> >> /proc/tile/board
> >> Information on part numbers, serial numbers, etc., of the
> >> hardware that the kernel is executing on
> >>
> >> /proc/tile/switch
> >> The type of control path for the onboard network switch, if any.
>
> These two report information about the hardware, not the hypervisor. For
> example:
>
> # cat /proc/tile/board
> board_part: 402-00002-05
> board_serial: NBS-5002-00012
> chip_serial: P62338.01.110
> chip_revision: A0
> board_revision: 2.2
> board_description: Tilera TILExpressPro-64, TILEPro64 processor (866 MHz-capable), 1 10GbE, 6 1GbE
> # cat /proc/tile/switch
> control: mdio gbe/0
I think it's ok to have it below /sys/hypervisor, because the information
is provided through a hypervisor ABI, even though it describes something
else. This is more like /sys/firmware, but the boundaries between that
and /sys/hypervisor are not clearly defined when running virtualized anyway.
> The chip_serial and chip_revision can certainly hang off
> /sys/devices/system/cpu along with chip_height and chip_width (I've made
> this change now) but I don't know where the remaining "board" level
> description could go. Note that (as you can see in the source) certain
> boards will also include four lines of output with the "mezzanine board"
> part number, serial number, revision, and description; this particular
> example doesn't have a mezzanine board. The "switch" info is broken out
> into a separate file just to make it easier to script some /etc/rc code
> that launches a configurator for the Marvell switch on some of our boards,
> but is conceptually part of the board info.
>
> >> /proc/tile/hardwall
> >> Information on the set of currently active hardwalls (note that
> >> the implementation is already present in arch/tile/kernel/hardwall.c;
> >> this change just enables it)
>
> This one is not a hypervisor-related file. It just lists information about
> the set of Linux hardwalls currently active. Again, it's not primarily
> intended for programmatic use, but as a diagnostic tool.
same here, I'd still put it into the hypervisor structure.
Arnd
^ permalink raw reply
* [PATCH RFC] virtio_ring: fix delayed enable cb implementation
From: Michael S. Tsirkin @ 2011-05-19 17:12 UTC (permalink / raw)
To: rusty, habanero, Shirley Ma, Krishna Kumar2, kvm, steved
Cc: netdev, linux-kernel, virtualization
Fix some signed/assigned mistakes in virtqueue_enable_cb_delayed
by using u16 math all over.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
I'll put this on my v1 branch as well
@@ -398,7 +397,7 @@ EXPORT_SYMBOL_GPL(virtqueue_enable_cb);
bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- int bufs;
+ u16 bufs;
START_USE(vq);
@@ -412,7 +411,7 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
bufs = (vq->vring.avail->idx - vq->last_used_idx) * 3 / 4;
vring_used_event(&vq->vring) = vq->last_used_idx + bufs;
virtio_mb();
- if (unlikely(vq->vring.used->idx - vq->last_used_idx > bufs)) {
+ if (unlikely((u16)(vq->vring.used->idx - vq->last_used_idx) > bufs)) {
END_USE(vq);
return false;
}
--
1.7.5.53.gc233e
^ permalink raw reply
* vmbus driver
From: K. Y. Srinivasan @ 2011-05-19 22:06 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization
Greg,
A few days ago you applied all the outstanding patches for the Hyper-V
drivers. With these patches, I have addressed all of the known review
comments for the vmbus driver (and a lot of comments/issues in other
drivers as well). I am still hoping I can address
whatever other issues/comments there might be with the intention to
get the vmbus driver out of staging in the current window. What is your
sense in terms of how feasible this is. From my side, I can assure you
that I will address all legitimate issues in a very timely manner and this
will not be dependent upon the location of the drivers (staging or
outside staging). Looking forward to hearing from you.
Regards,
K. Y
^ permalink raw reply
* [PATCHv2 00/14] virtio and vhost-net performance enhancements
From: Michael S. Tsirkin @ 2011-05-19 23:10 UTC (permalink / raw)
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
OK, here is the large patchset that implements the virtio spec update
that I sent earlier (the spec itself needs a minor update, will send
that out too next week, but I think we are on the same page here
already). It supercedes the PUBLISH_USED_IDX patches I sent
out earlier.
What will follow will be a patchset that actually includes 4 sets of
patches. I note below their status. Please consider for 2.6.40, at
least partially. Rusty, do you think it's feasible?
List of patches and what they do:
I) With the first patchset, we change virtio ring notification
hand-off to work like the one in Xen -
each side publishes an event index, the other one
notifies when it reaches that value -
With the one difference that event index starts at 0,
same as request index (in xen event index starts at 1).
These are the patches in this set:
virtio: event index interface
virtio ring: inline function to check for events
virtio_ring: support event idx feature
vhost: support event index
virtio_test: support event index
Changes in this part of the patchset from v1 - address comments by Rusty et al.
I tested this a lot with virtio net block and with the simulator and esp
with the simulator it's easy to see drastic performance improvement
here:
[virtio]# time ./virtio_test
spurious wakeus: 0x7
real 0m0.169s
user 0m0.140s
sys 0m0.019s
[virtio]# time ./virtio_test --no-event-idx
spurious wakeus: 0x11
real 0m0.649s
user 0m0.295s
sys 0m0.335s
And these patches are mostly unchanged from the very first version,
changes being almost exclusively code cleanups. So I consider this part
the most stable, I strongly think these patches should go into 2.6.40.
One extra reason besides performance is that maintaining
them out of tree is very painful as guest/host ABI is affected.
II) Second set of patches: new apis and use in virtio_net
With the indexes in place it becomes possibile to request an event after
many requests (and not just on the next one as done now). This shall fix
the TX queue overrun which currently triggers a storm of interrupts.
Another issue I tried to fix is capacity checks in virtio-net,
there's a new API for that, and on top of that,
I implemented a patch improving real-time characteristics
of virtio_net
Thus we get the second patchset:
virtio: add api for delayed callbacks
virtio_net: delay TX callbacks
virtio_ring: Add capacity check API
virtio_net: fix TX capacity checks using new API
virtio_net: limit xmit polling
This has some fixes that I posted previously applied,
but otherwise ideantical to v1. I tried to change API
for enable_cb_delayed as Rusty suggested but failed to do this.
I think it's not possible to define cleanly.
These work fine for me, I think they can be merged for 2.6.40
too but would be nice to hear back from Shirley, Tom, Krishna.
III) There's also a patch that adds a tweak to virtio ring
virtio: don't delay avail index update
This seems to help small message sizes where we are constantly draining
the RX VQ.
I'll need to benchmark this to be able to give any numbers
with confidence, but I don't see how it can hurt anything.
Thoughts?
IV) Last part is a set of patches to extend feature bits
to 64 bit. I tested this by using feature bit 32.
vhost: fix 64 bit features
virtio_test: update for 64 bit features
virtio: 64 bit features
It's nice to have as set I used up the last free bit.
But not a must now that a single bit controls
use of event index on both sides.
The patchset is on top of net-next which at the time
I last rebased was 15ecd03 - so roughly 2.6.39-rc2.
For testing I usually do merge v2.6.39 on top.
qemu patch is also ready. Code can be pulled from here:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net-next-event-idx-v3
git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu-kvm.git virtio-net-event-idx-v3
Rusty, I think it will be easier to merge vhost and virtio bits in one
go. Can it all go in through your tree (Dave in the past acked
sending a very similar patch through you so should not be a problem)?
--
1.7.5.53.gc233e
Michael S. Tsirkin (13):
virtio: event index interface
virtio ring: inline function to check for events
virtio_ring: support event idx feature
vhost: support event index
virtio_test: support event index
virtio: add api for delayed callbacks
virtio_net: delay TX callbacks
virtio_net: fix TX capacity checks using new API
virtio_net: limit xmit polling
virtio: don't delay avail index update
virtio: 64 bit features
virtio_test: update for 64 bit features
vhost: fix 64 bit features
Shirley Ma (1):
virtio_ring: Add capacity check API
drivers/lguest/lguest_device.c | 8 +-
drivers/net/virtio_net.c | 27 +++++---
drivers/s390/kvm/kvm_virtio.c | 8 +-
drivers/vhost/net.c | 12 ++--
drivers/vhost/test.c | 6 +-
drivers/vhost/vhost.c | 138 ++++++++++++++++++++++++++++++----------
drivers/vhost/vhost.h | 29 +++++---
drivers/virtio/virtio.c | 8 +-
drivers/virtio/virtio_pci.c | 34 ++++++++--
drivers/virtio/virtio_ring.c | 87 ++++++++++++++++++++++---
include/linux/virtio.h | 16 ++++-
include/linux/virtio_config.h | 15 +++--
include/linux/virtio_pci.h | 9 ++-
include/linux/virtio_ring.h | 29 ++++++++-
tools/virtio/virtio_test.c | 27 +++++++-
15 files changed, 348 insertions(+), 105 deletions(-)
--
1.7.5.53.gc233e
^ permalink raw reply
* [PATCHv2 01/14] virtio: event index interface
From: Michael S. Tsirkin @ 2011-05-19 23:10 UTC (permalink / raw)
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <cover.1305846412.git.mst@redhat.com>
Define a new feature bit for the guest and host to utilize
an event index (like Xen) instead if a flag bit to enable/disable
interrupts and kicks.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_ring.h | 15 ++++++++++++++-
1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index e4d144b..70b0b39 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -29,6 +29,12 @@
/* We support indirect buffer descriptors */
#define VIRTIO_RING_F_INDIRECT_DESC 28
+/* The Guest publishes the used index for which it expects an interrupt
+ * at the end of the avail ring. Host should ignore the avail->flags field. */
+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_EVENT_IDX 29
+
/* Virtio ring descriptors: 16 bytes. These can chain together via "next". */
struct vring_desc {
/* Address (guest-physical). */
@@ -83,6 +89,7 @@ struct vring {
* __u16 avail_flags;
* __u16 avail_idx;
* __u16 available[num];
+ * __u16 used_event_idx;
*
* // Padding to the next align boundary.
* char pad[];
@@ -91,8 +98,14 @@ struct vring {
* __u16 used_flags;
* __u16 used_idx;
* struct vring_used_elem used[num];
+ * __u16 avail_event_idx;
* };
*/
+/* We publish the used event index at the end of the available ring, and vice
+ * versa. They are at the end for backwards compatibility. */
+#define vring_used_event(vr) ((vr)->avail->ring[(vr)->num])
+#define vring_avail_event(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
+
static inline void vring_init(struct vring *vr, unsigned int num, void *p,
unsigned long align)
{
@@ -107,7 +120,7 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
{
return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
+ align - 1) & ~(align - 1))
- + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
+ + sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num;
}
#ifdef __KERNEL__
--
1.7.5.53.gc233e
^ permalink raw reply related
* [PATCHv2 02/14] virtio ring: inline function to check for events
From: Michael S. Tsirkin @ 2011-05-19 23:10 UTC (permalink / raw)
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <cover.1305846412.git.mst@redhat.com>
With the new used_event and avail_event and features, both
host and guest need similar logic to check whether events are
enabled, so it helps to put the common code in the header.
Note that Xen has similar logic for notification hold-off
in include/xen/interface/io/ring.h with req_event and req_prod
corresponding to event_idx + 1 and new_idx respectively.
+1 comes from the fact that req_event and req_prod in Xen start at 1,
while event index in virtio starts at 0.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_ring.h | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 70b0b39..cf020e3 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -123,6 +123,20 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
+ sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num;
}
+/* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
+/* Assuming a given event_idx value from the other size, if
+ * we have just incremented index from old to new_idx,
+ * should we trigger an event? */
+static inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
+{
+ /* Note: Xen has similar logic for notification hold-off
+ * in include/xen/interface/io/ring.h with req_event and req_prod
+ * corresponding to event_idx + 1 and new_idx respectively.
+ * Note also that req_event and req_prod in Xen start at 1,
+ * event indexes in virtio start at 0. */
+ return (__u16)(new_idx - event_idx - 1) < (__u16)(new_idx - old);
+}
+
#ifdef __KERNEL__
#include <linux/irqreturn.h>
struct virtio_device;
--
1.7.5.53.gc233e
^ permalink raw reply related
* [PATCHv2 03/14] virtio_ring: support event idx feature
From: Michael S. Tsirkin @ 2011-05-19 23:10 UTC (permalink / raw)
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <cover.1305846412.git.mst@redhat.com>
Support for the new event idx feature:
1. When enabling interrupts, publish the current avail index
value to the host to get interrupts on the next update.
2. Use the new avail_event feature to reduce the number
of exits from the guest.
Simple test with the simulator:
[virtio]# time ./virtio_test
spurious wakeus: 0x7
real 0m0.169s
user 0m0.140s
sys 0m0.019s
[virtio]# time ./virtio_test --no-event-idx
spurious wakeus: 0x11
real 0m0.649s
user 0m0.295s
sys 0m0.335s
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_ring.c | 26 ++++++++++++++++++++++++--
1 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index cc2f73e..1d0f9be 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -82,6 +82,9 @@ struct vring_virtqueue
/* Host supports indirect buffers */
bool indirect;
+ /* Host publishes avail event idx */
+ bool event;
+
/* Number of free buffers */
unsigned int num_free;
/* Head of free buffer list. */
@@ -237,18 +240,22 @@ EXPORT_SYMBOL_GPL(virtqueue_add_buf_gfp);
void virtqueue_kick(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ u16 new, old;
START_USE(vq);
/* Descriptors and available array need to be set before we expose the
* new available array entries. */
virtio_wmb();
- vq->vring.avail->idx += vq->num_added;
+ old = vq->vring.avail->idx;
+ new = vq->vring.avail->idx = old + vq->num_added;
vq->num_added = 0;
/* Need to update avail index before checking if we should notify */
virtio_mb();
- if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
+ if (vq->event ?
+ vring_need_event(vring_avail_event(&vq->vring), new, old) :
+ !(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
/* Prod other side to tell it about changes. */
vq->notify(&vq->vq);
@@ -324,6 +331,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
ret = vq->data[i];
detach_buf(vq, i);
vq->last_used_idx++;
+ /* 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->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
+ vring_used_event(&vq->vring) = vq->last_used_idx;
+ virtio_mb();
+ }
+
END_USE(vq);
return ret;
}
@@ -345,7 +360,11 @@ bool virtqueue_enable_cb(struct virtqueue *_vq)
/* We optimistically turn back on interrupts, then check if there was
* more to do. */
+ /* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
+ * either clear the flags bit or point the event index at the next
+ * entry. Always do both to keep code simple. */
vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
+ vring_used_event(&vq->vring) = vq->last_used_idx;
virtio_mb();
if (unlikely(more_used(vq))) {
END_USE(vq);
@@ -437,6 +456,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
#endif
vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+ vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
/* No callback? Tell other side not to bother us. */
if (!callback)
@@ -471,6 +491,8 @@ void vring_transport_features(struct virtio_device *vdev)
switch (i) {
case VIRTIO_RING_F_INDIRECT_DESC:
break;
+ case VIRTIO_RING_F_EVENT_IDX:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev->features);
--
1.7.5.53.gc233e
^ permalink raw reply related
* [PATCHv2 04/14] vhost: support event index
From: Michael S. Tsirkin @ 2011-05-19 23:10 UTC (permalink / raw)
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <cover.1305846412.git.mst@redhat.com>
Support the new event index feature. When acked,
utilize it to reduce the # of interrupts sent to the guest.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 12 ++--
drivers/vhost/test.c | 6 +-
drivers/vhost/vhost.c | 138 +++++++++++++++++++++++++++++++++++++------------
drivers/vhost/vhost.h | 21 +++++---
4 files changed, 127 insertions(+), 50 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 2f7c76a..e224a92 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -144,7 +144,7 @@ static void handle_tx(struct vhost_net *net)
}
mutex_lock(&vq->mutex);
- vhost_disable_notify(vq);
+ vhost_disable_notify(&net->dev, vq);
if (wmem < sock->sk->sk_sndbuf / 2)
tx_poll_stop(net);
@@ -166,8 +166,8 @@ static void handle_tx(struct vhost_net *net)
set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
break;
}
- if (unlikely(vhost_enable_notify(vq))) {
- vhost_disable_notify(vq);
+ if (unlikely(vhost_enable_notify(&net->dev, vq))) {
+ vhost_disable_notify(&net->dev, vq);
continue;
}
break;
@@ -315,7 +315,7 @@ static void handle_rx(struct vhost_net *net)
return;
mutex_lock(&vq->mutex);
- vhost_disable_notify(vq);
+ vhost_disable_notify(&net->dev, vq);
vhost_hlen = vq->vhost_hlen;
sock_hlen = vq->sock_hlen;
@@ -334,10 +334,10 @@ static void handle_rx(struct vhost_net *net)
break;
/* OK, now we need to know about added descriptors. */
if (!headcount) {
- if (unlikely(vhost_enable_notify(vq))) {
+ if (unlikely(vhost_enable_notify(&net->dev, vq))) {
/* They have slipped one in as we were
* doing that: check again. */
- vhost_disable_notify(vq);
+ vhost_disable_notify(&net->dev, vq);
continue;
}
/* Nothing new? Wait for eventfd to tell us
diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
index 099f302..734e1d7 100644
--- a/drivers/vhost/test.c
+++ b/drivers/vhost/test.c
@@ -49,7 +49,7 @@ static void handle_vq(struct vhost_test *n)
return;
mutex_lock(&vq->mutex);
- vhost_disable_notify(vq);
+ vhost_disable_notify(&n->dev, vq);
for (;;) {
head = vhost_get_vq_desc(&n->dev, vq, vq->iov,
@@ -61,8 +61,8 @@ static void handle_vq(struct vhost_test *n)
break;
/* Nothing new? Wait for eventfd to tell us they refilled. */
if (head == vq->num) {
- if (unlikely(vhost_enable_notify(vq))) {
- vhost_disable_notify(vq);
+ if (unlikely(vhost_enable_notify(&n->dev, vq))) {
+ vhost_disable_notify(&n->dev, vq);
continue;
}
break;
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 2ab2912..2a10786 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -37,6 +37,9 @@ enum {
VHOST_MEMORY_F_LOG = 0x1,
};
+#define vhost_used_event(vq) ((u16 __user *)&vq->avail->ring[vq->num])
+#define vhost_avail_event(vq) ((u16 __user *)&vq->used->ring[vq->num])
+
static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
poll_table *pt)
{
@@ -161,6 +164,8 @@ static void vhost_vq_reset(struct vhost_dev *dev,
vq->last_avail_idx = 0;
vq->avail_idx = 0;
vq->last_used_idx = 0;
+ vq->signalled_used = 0;
+ vq->signalled_used_valid = false;
vq->used_flags = 0;
vq->log_used = false;
vq->log_addr = -1ull;
@@ -489,16 +494,17 @@ static int memory_access_ok(struct vhost_dev *d, struct vhost_memory *mem,
return 1;
}
-static int vq_access_ok(unsigned int num,
+static int vq_access_ok(struct vhost_dev *d, unsigned int num,
struct vring_desc __user *desc,
struct vring_avail __user *avail,
struct vring_used __user *used)
{
+ size_t s = vhost_has_feature(d, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
return access_ok(VERIFY_READ, desc, num * sizeof *desc) &&
access_ok(VERIFY_READ, avail,
- sizeof *avail + num * sizeof *avail->ring) &&
+ sizeof *avail + num * sizeof *avail->ring + s) &&
access_ok(VERIFY_WRITE, used,
- sizeof *used + num * sizeof *used->ring);
+ sizeof *used + num * sizeof *used->ring + s);
}
/* Can we log writes? */
@@ -514,9 +520,11 @@ int vhost_log_access_ok(struct vhost_dev *dev)
/* Verify access for write logging. */
/* Caller should have vq mutex and device mutex */
-static int vq_log_access_ok(struct vhost_virtqueue *vq, void __user *log_base)
+static int vq_log_access_ok(struct vhost_dev *d, struct vhost_virtqueue *vq,
+ void __user *log_base)
{
struct vhost_memory *mp;
+ size_t s = vhost_has_feature(d, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
mp = rcu_dereference_protected(vq->dev->memory,
lockdep_is_held(&vq->mutex));
@@ -524,15 +532,15 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq, void __user *log_base)
vhost_has_feature(vq->dev, VHOST_F_LOG_ALL)) &&
(!vq->log_used || log_access_ok(log_base, vq->log_addr,
sizeof *vq->used +
- vq->num * sizeof *vq->used->ring));
+ vq->num * sizeof *vq->used->ring + s));
}
/* Can we start vq? */
/* Caller should have vq mutex and device mutex */
int vhost_vq_access_ok(struct vhost_virtqueue *vq)
{
- return vq_access_ok(vq->num, vq->desc, vq->avail, vq->used) &&
- vq_log_access_ok(vq, vq->log_base);
+ return vq_access_ok(vq->dev, vq->num, vq->desc, vq->avail, vq->used) &&
+ vq_log_access_ok(vq->dev, vq, vq->log_base);
}
static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
@@ -577,6 +585,7 @@ static int init_used(struct vhost_virtqueue *vq,
if (r)
return r;
+ vq->signalled_used_valid = false;
return get_user(vq->last_used_idx, &used->idx);
}
@@ -674,7 +683,7 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
* If it is not, we don't as size might not have been setup.
* We will verify when backend is configured. */
if (vq->private_data) {
- if (!vq_access_ok(vq->num,
+ if (!vq_access_ok(d, vq->num,
(void __user *)(unsigned long)a.desc_user_addr,
(void __user *)(unsigned long)a.avail_user_addr,
(void __user *)(unsigned long)a.used_user_addr)) {
@@ -818,7 +827,7 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
vq = d->vqs + i;
mutex_lock(&vq->mutex);
/* If ring is inactive, will check when it's enabled. */
- if (vq->private_data && !vq_log_access_ok(vq, base))
+ if (vq->private_data && !vq_log_access_ok(d, vq, base))
r = -EFAULT;
else
vq->log_base = base;
@@ -1219,6 +1228,10 @@ int vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
/* On success, increment avail index. */
vq->last_avail_idx++;
+
+ /* Assume notifications from guest are disabled at this point,
+ * if they aren't we would need to update avail_event index. */
+ BUG_ON(!(vq->used_flags & VRING_USED_F_NO_NOTIFY));
return head;
}
@@ -1267,6 +1280,12 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
eventfd_signal(vq->log_ctx, 1);
}
vq->last_used_idx++;
+ /* If the driver never bothers to signal in a very long while,
+ * used index might wrap around. If that happens, invalidate
+ * signalled_used index we stored. TODO: make sure driver
+ * signals at least once in 2^16 and remove this. */
+ if (unlikely(vq->last_used_idx == vq->signalled_used))
+ vq->signalled_used_valid = false;
return 0;
}
@@ -1275,6 +1294,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
unsigned count)
{
struct vring_used_elem __user *used;
+ u16 old, new;
int start;
start = vq->last_used_idx % vq->num;
@@ -1292,7 +1312,14 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
((void __user *)used - (void __user *)vq->used),
count * sizeof *used);
}
- vq->last_used_idx += count;
+ old = vq->last_used_idx;
+ new = (vq->last_used_idx += count);
+ /* If the driver never bothers to signal in a very long while,
+ * used index might wrap around. If that happens, invalidate
+ * signalled_used index we stored. TODO: make sure driver
+ * signals at least once in 2^16 and remove this. */
+ if (unlikely((u16)(new - vq->signalled_used) < (u16)(new - old)))
+ vq->signalled_used_valid = false;
return 0;
}
@@ -1331,29 +1358,47 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
return r;
}
-/* This actually signals the guest, using eventfd. */
-void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
{
- __u16 flags;
-
+ __u16 old, new, event;
+ bool v;
/* Flush out used index updates. This is paired
* with the barrier that the Guest executes when enabling
* interrupts. */
smp_mb();
- if (__get_user(flags, &vq->avail->flags)) {
- vq_err(vq, "Failed to get flags");
- return;
+ if (vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY) &&
+ unlikely(vq->avail_idx == vq->last_avail_idx))
+ return true;
+
+ if (!vhost_has_feature(dev, VIRTIO_RING_F_EVENT_IDX)) {
+ __u16 flags;
+ if (__get_user(flags, &vq->avail->flags)) {
+ vq_err(vq, "Failed to get flags");
+ return true;
+ }
+ return !(flags & VRING_AVAIL_F_NO_INTERRUPT);
}
+ old = vq->signalled_used;
+ v = vq->signalled_used_valid;
+ new = vq->signalled_used = vq->last_used_idx;
+ vq->signalled_used_valid = true;
- /* If they don't want an interrupt, don't signal, unless empty. */
- if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
- (vq->avail_idx != vq->last_avail_idx ||
- !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
- return;
+ if (unlikely(!v))
+ return true;
+ if (get_user(event, vhost_used_event(vq))) {
+ vq_err(vq, "Failed to get used event idx");
+ return true;
+ }
+ return vring_need_event(event, new, old);
+}
+
+/* This actually signals the guest, using eventfd. */
+void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+{
/* Signal the Guest tell them we used something up. */
- if (vq->call_ctx)
+ if (vq->call_ctx && vhost_notify(dev, vq))
eventfd_signal(vq->call_ctx, 1);
}
@@ -1376,7 +1421,7 @@ void vhost_add_used_and_signal_n(struct vhost_dev *dev,
}
/* OK, now we need to know about added descriptors. */
-bool vhost_enable_notify(struct vhost_virtqueue *vq)
+bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
{
u16 avail_idx;
int r;
@@ -1384,11 +1429,34 @@ bool vhost_enable_notify(struct vhost_virtqueue *vq)
if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY))
return false;
vq->used_flags &= ~VRING_USED_F_NO_NOTIFY;
- r = put_user(vq->used_flags, &vq->used->flags);
- if (r) {
- vq_err(vq, "Failed to enable notification at %p: %d\n",
- &vq->used->flags, r);
- return false;
+ if (!vhost_has_feature(dev, VIRTIO_RING_F_EVENT_IDX)) {
+ r = put_user(vq->used_flags, &vq->used->flags);
+ if (r) {
+ vq_err(vq, "Failed to enable notification at %p: %d\n",
+ &vq->used->flags, r);
+ return false;
+ }
+ } else {
+ r = put_user(vq->avail_idx, vhost_avail_event(vq));
+ if (r) {
+ vq_err(vq, "Failed to update avail event index at %p: %d\n",
+ vhost_avail_event(vq), r);
+ return false;
+ }
+ }
+ if (unlikely(vq->log_used)) {
+ void __user *used;
+ /* Make sure data is seen before log. */
+ smp_wmb();
+ used = vhost_has_feature(dev, VIRTIO_RING_F_EVENT_IDX) ?
+ &vq->used->flags : vhost_avail_event(vq);
+ /* Log used flags or event index entry write. Both are 16 bit
+ * fields. */
+ log_write(vq->log_base, vq->log_addr +
+ (used - (void __user *)vq->used),
+ sizeof(u16));
+ if (vq->log_ctx)
+ eventfd_signal(vq->log_ctx, 1);
}
/* They could have slipped one in as we were doing that: make
* sure it's written, then check again. */
@@ -1404,15 +1472,17 @@ bool vhost_enable_notify(struct vhost_virtqueue *vq)
}
/* We don't need to be notified again. */
-void vhost_disable_notify(struct vhost_virtqueue *vq)
+void vhost_disable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
{
int r;
if (vq->used_flags & VRING_USED_F_NO_NOTIFY)
return;
vq->used_flags |= VRING_USED_F_NO_NOTIFY;
- r = put_user(vq->used_flags, &vq->used->flags);
- if (r)
- vq_err(vq, "Failed to enable notification at %p: %d\n",
- &vq->used->flags, r);
+ if (!vhost_has_feature(dev, VIRTIO_RING_F_EVENT_IDX)) {
+ r = put_user(vq->used_flags, &vq->used->flags);
+ if (r)
+ vq_err(vq, "Failed to enable notification at %p: %d\n",
+ &vq->used->flags, r);
+ }
}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index b3363ae..8e03379 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -84,6 +84,12 @@ struct vhost_virtqueue {
/* Used flags */
u16 used_flags;
+ /* Last used index value we have signalled on */
+ u16 signalled_used;
+
+ /* Last used index value we have signalled on */
+ bool signalled_used_valid;
+
/* Log writes to used structure. */
bool log_used;
u64 log_addr;
@@ -149,8 +155,8 @@ void vhost_add_used_and_signal(struct vhost_dev *, struct vhost_virtqueue *,
void vhost_add_used_and_signal_n(struct vhost_dev *, struct vhost_virtqueue *,
struct vring_used_elem *heads, unsigned count);
void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
-void vhost_disable_notify(struct vhost_virtqueue *);
-bool vhost_enable_notify(struct vhost_virtqueue *);
+void vhost_disable_notify(struct vhost_dev *, struct vhost_virtqueue *);
+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);
@@ -162,11 +168,12 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
} while (0)
enum {
- VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
- (1 << VIRTIO_RING_F_INDIRECT_DESC) |
- (1 << VHOST_F_LOG_ALL) |
- (1 << VHOST_NET_F_VIRTIO_NET_HDR) |
- (1 << VIRTIO_NET_F_MRG_RXBUF),
+ VHOST_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
+ (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
+ (1ULL << VIRTIO_RING_F_EVENT_IDX) |
+ (1ULL << VHOST_F_LOG_ALL) |
+ (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
+ (1ULL << VIRTIO_NET_F_MRG_RXBUF),
};
static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
--
1.7.5.53.gc233e
^ permalink raw reply related
* [PATCHv2 05/14] virtio_test: support event index
From: Michael S. Tsirkin @ 2011-05-19 23:11 UTC (permalink / raw)
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <cover.1305846412.git.mst@redhat.com>
Add ability to test the new event idx feature,
enable by default.
---
tools/virtio/virtio_test.c | 19 +++++++++++++++++--
1 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
index df0c6d2..74d3331 100644
--- a/tools/virtio/virtio_test.c
+++ b/tools/virtio/virtio_test.c
@@ -198,6 +198,14 @@ const struct option longopts[] = {
.val = 'h',
},
{
+ .name = "event-idx",
+ .val = 'E',
+ },
+ {
+ .name = "no-event-idx",
+ .val = 'e',
+ },
+ {
.name = "indirect",
.val = 'I',
},
@@ -211,13 +219,17 @@ const struct option longopts[] = {
static void help()
{
- fprintf(stderr, "Usage: virtio_test [--help] [--no-indirect]\n");
+ fprintf(stderr, "Usage: virtio_test [--help]"
+ " [--no-indirect]"
+ " [--no-event-idx]"
+ "\n");
}
int main(int argc, char **argv)
{
struct vdev_info dev;
- unsigned long long features = 1ULL << VIRTIO_RING_F_INDIRECT_DESC;
+ unsigned long long features = (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
+ (1ULL << VIRTIO_RING_F_EVENT_IDX);
int o;
for (;;) {
@@ -228,6 +240,9 @@ int main(int argc, char **argv)
case '?':
help();
exit(2);
+ case 'e':
+ features &= ~(1ULL << VIRTIO_RING_F_EVENT_IDX);
+ break;
case 'h':
help();
goto done;
--
1.7.5.53.gc233e
^ permalink raw reply related
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