* [PATCH 3/3][STABLE] KVM: add schedule check to napi_enable call
From: Bruce Rogers @ 2010-06-03 22:29 UTC (permalink / raw)
To: rusty; +Cc: virtualization
Please consider this patch for the 2.6.32, 2.6.33, and 2.6.34 stable trees as well as current development trees. (I've only tested on 2.6.32 however)
virtio_net: Add schedule check to napi_enable call
Under harsh testing conditions, including low memory, the guest would
stop receiving packets. With this patch applied we no longer see any
problems in the driver while performing these tests for extended periods
of time.
Make sure napi is scheduled subsequent to each napi_enable.
Signed-off-by: Bruce Rogers <brogers@novell.com>
Signed-off-by: Olaf Kirch <okir@suse.de>
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -388,6 +388,20 @@ static void skb_recv_done(struct virtque
}
}
+static void virtnet_napi_enable(struct virtnet_info *vi)
+{
+ napi_enable(&vi->napi);
+
+ /* If all buffers were filled by other side before we napi_enabled, we
+ * won't get another interrupt, so process any outstanding packets
+ * now. virtnet_poll wants re-enable the queue, so we disable here.
+ * We synchronize against interrupts via NAPI_STATE_SCHED */
+ if (napi_schedule_prep(&vi->napi)) {
+ vi->rvq->vq_ops->disable_cb(vi->rvq);
+ __napi_schedule(&vi->napi);
+ }
+}
+
static void refill_work(struct work_struct *work)
{
struct virtnet_info *vi;
@@ -397,7 +411,7 @@ static void refill_work(struct work_stru
napi_disable(&vi->napi);
try_fill_recv(vi, GFP_KERNEL);
still_empty = (vi->num == 0);
- napi_enable(&vi->napi);
+ virtnet_napi_enable(vi);
/* In theory, this can happen: if we don't get any buffers in
* we will *never* try to fill again. */
@@ -589,16 +603,7 @@ static int virtnet_open(struct net_devic
{
struct virtnet_info *vi = netdev_priv(dev);
- napi_enable(&vi->napi);
-
- /* If all buffers were filled by other side before we napi_enabled, we
- * won't get another interrupt, so process any outstanding packets
- * now. virtnet_poll wants re-enable the queue, so we disable here.
- * We synchronize against interrupts via NAPI_STATE_SCHED */
- if (napi_schedule_prep(&vi->napi)) {
- vi->rvq->vq_ops->disable_cb(vi->rvq);
- __napi_schedule(&vi->napi);
- }
+ virtnet_napi_enable(vi);
return 0;
}
^ permalink raw reply
* [PATCH 2/3][STABLE] KVM: indicate oom if add_buf fails
From: Bruce Rogers @ 2010-06-03 22:27 UTC (permalink / raw)
To: rusty; +Cc: virtualization
This patch is a subset of an already upstream patch, but this portion is useful in earlier releases.
Please consider for the 2.6.32 and 2.6.33 stable trees.
If the add_buf operation fails, indicate failure to the caller.
Signed-off-by: Bruce Rogers <brogers@novell.com>
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -318,6 +318,7 @@ static bool try_fill_recv_maxbufs(struct
skb_unlink(skb, &vi->recv);
trim_pages(vi, skb);
kfree_skb(skb);
+ oom = true;
break;
}
vi->num++;
@@ -368,6 +369,7 @@ static bool try_fill_recv(struct virtnet
if (err < 0) {
skb_unlink(skb, &vi->recv);
kfree_skb(skb);
+ oom = true;
break;
}
vi->num++;
^ permalink raw reply
* [PATCH 1/3][STABLE] KVM: fix delayed refill checking
From: Bruce Rogers @ 2010-06-03 22:26 UTC (permalink / raw)
To: rusty; +Cc: virtualization
Please consider this for the 2.6.32 stable tree:
commit 39d321577405e8e269fd238b278aaf2425fa788a
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon Jan 25 15:51:01 2010 -0800
virtio_net: Make delayed refill more reliable
I have seen RX stalls on a machine that experienced a suspected
OOM. After the stall, the RX buffer is empty on the guest side
and there are exactly 16 entries available on the host side. As
the number of entries is less than that required by a maximal
skb, the host cannot proceed.
The guest did not have a refill job scheduled.
My diagnosis is that an OOM had occured, with the delayed refill
job scheduled. The job was able to allocate at least one skb, but
not enough to overcome the minimum required by the host to proceed.
As the refill job would only reschedule itself if it failed completely
to allocate any skbs, this would lead to an RX stall.
The following patch removes this stall possibility by always
rescheduling the refill job until the ring is totally refilled.
Testing has shown that the RX stall no longer occurs whereas
previously it would occur within a day.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index c708ecc..9ead30b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -395,8 +395,7 @@ static void refill_work(struct work_struct *work)
vi = container_of(work, struct virtnet_info, refill.work);
napi_disable(&vi->napi);
- try_fill_recv(vi, GFP_KERNEL);
- still_empty = (vi->num == 0);
+ still_empty = !try_fill_recv(vi, GFP_KERNEL);
napi_enable(&vi->napi);
/* In theory, this can happen: if we don't get any buffers in
^ permalink raw reply related
* [PATCH 0/3][STABLE] KVM: Various issues in virtio_net
From: Bruce Rogers @ 2010-06-03 22:24 UTC (permalink / raw)
To: rusty; +Cc: virtualization
These are patches which we have found useful for our 2.6.32 based SLES 11 SP1 release.
The first patch is already upstream, but should be included in 2.6.32 stable tree.
The second patch is a subset of another upstream patch. Again, stable material, applicable to 2.6.32 and 2.6.33.
The third patch solves the last remaining issue we saw when testing kvm configurations with the SUSE certification test suite. Under heavy load, we observed rx stalls (first two patches applied), and this third patch was crafted to address the issue. Please apply to 2.6.32, 2.6.33, and 2.6.34 stable trees.
(I assume this last problem also exists in more recent kernels than 2.6.32, but I haven't validated that.)
With these 3 patches applied we no longer see any issues with virito networking using our certification test suite.
Signed-off-by: Bruce Rogers <brogers@novell.com>
^ permalink raw reply
* [PULL] virtio fixes
From: Rusty Russell @ 2010-06-03 13:13 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Amit Shah, Christoph Hellwig, virtualization
The following changes since commit aef4b9aaae1decc775778903922bd0075cce7a88:
Linus Torvalds (1):
Merge branch 'next' of git://git.kernel.org/.../benh/powerpc
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.git virtio
Amit Shah (2):
virtio: console: Fix crash when hot-unplugging a port and read is blocked
virtio: console: Fix crash when port is unplugged and blocked for write
Christoph Hellwig (1):
virtio-blk: fix minimum number of S/G elements
drivers/block/virtio_blk.c | 4 +++-
drivers/char/virtio_console.c | 14 +++++++++++---
2 files changed, 14 insertions(+), 4 deletions(-)
commit a5b365a652206ca300256974ed9301a7d241a6ed
Author: Christoph Hellwig <hch@lst.de>
Date: Tue May 25 14:17:54 2010 +0200
virtio-blk: fix minimum number of S/G elements
We need at least one S/G element to operate properly, as does the block
layer which increments it to one anyway. We hit this due to a qemu
bug which advertises a sg_elements of 0 under some circumstances.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (tweaked logic)
drivers/block/virtio_blk.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
commit 0047634d3daebca9e99a22eb89167bf77f35cdfa
Author: Amit Shah <amit.shah@redhat.com>
Date: Thu May 27 13:24:39 2010 +0530
virtio: console: Fix crash when hot-unplugging a port and read is blocked
When removing a port we don't check if a program was blocked for read.
This leads to a crash when SIGTERM is sent to the program after
hot-unplugging the port.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
drivers/char/virtio_console.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
commit 60e5e0b84045ce0f6ab07a02c7fcd6627b53d2d3
Author: Amit Shah <amit.shah@redhat.com>
Date: Thu May 27 13:24:40 2010 +0530
virtio: console: Fix crash when port is unplugged and blocked for write
When a program that has a virtio port opened and blocked for a write
operation, a port hot-unplug event will later led to a crash when
SIGTERM was sent to the program. Fix that.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
drivers/char/virtio_console.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
^ permalink raw reply
* Re: [Qemu-devel] Re: [PATCHv2-RFC 0/2] virtio: put last seen used index into ring itself
From: Jes Sorensen @ 2010-06-02 20:39 UTC (permalink / raw)
To: Rusty Russell
Cc: linux-kernel, virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <201005311716.43573.rusty@rustcorp.com.au>
On 05/31/10 09:46, Rusty Russell wrote:
> On Thu, 27 May 2010 05:20:35 am Michael S. Tsirkin wrote:
>> Here's a rewrite of the original patch with a new layout.
>> I haven't tested it yet so no idea how this performs, but
>> I think this addresses the cache bounce issue raised by Avi.
>> Posting for early flames/comments.
>
> Sorry, not without some evidence that it'll actually reduce cacheline
> bouncing. I *think* it will, but it's not obvious: the host may keep
> looking at avail_idx as we're updating last_seen. Or does qemu always
> look at both together anyway?
>
> Can someone convince me this is a win?
> Rusty.
Hi Rusty,
I ran some tests using the vring index publish patch with virtio_blk.
The numbers are based on running IOZone on a ramdisk passed to the guest
via virtio. While I didn't see any throughput improvements, I saw a
20-30% reduction in the VMExit count for the full run. This was measured
grabbing the VMExit count prior and after the run and calculating the
difference.
I have the numbers in a PDF, so I will email it to you in private as I
don't like sending PDFs to the mailing list. However if anybody else
wants the numbers feel free to ping me off list and I'll forward them.
Cheers,
Jes
^ permalink raw reply
* Re: [PATCHv3 0/2] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-06-01 15:12 UTC (permalink / raw)
To: Rusty Russell, linux-kernel, virtualization, kvm, qemu-devel
In-Reply-To: <cover.1275403477.git.mst@redhat.com>
On Tue, Jun 01, 2010 at 05:47:08PM +0300, Michael S. Tsirkin wrote:
> Changes from v2: added padding between avail idx and flags,
> and changed virtio to only publish used index when callbacks
> are enabled.
Here's the updated spec patch.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
--
diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index ed35893..a2dcd76 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -1803,6 +1803,36 @@ next
\emph default
descriptor entry (modulo the ring size).
This starts at 0, and increases.
+\change_inserted 0 1274966643
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274968378
+When PUBLISH_USED feature flag has
+\emph on
+not
+\emph default
+ been negotiated, the ring follows the
+\begin_inset Quotes eld
+\end_inset
+
+flags
+\begin_inset Quotes erd
+\end_inset
+
+ and the
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ fields:
+\change_unchanged
+
\end_layout
\begin_layout Standard
@@ -1845,7 +1875,143 @@ struct vring_avail {
\end_layout
+\begin_layout Standard
+
+\change_inserted 0 1274968432
+\begin_inset CommandInset label
+LatexCommand label
+name "PUBLISH_USED-feature"
+
+\end_inset
+
+When PUBLISH_USED feature flag has been negotiated, the control structure
+ including the
+\begin_inset Quotes eld
+\end_inset
+
+flags and the
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ fields follows the ring.
+ This leaves the room for the
+\begin_inset Quotes eld
+\end_inset
+
+last_seen_used_idx
+\begin_inset Quotes erd
+\end_inset
+
+ field, which indicates the most recent
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ value observed by guest in the used ring (see
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "sub:Used-Ring"
+
+\end_inset
+
+ below):
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274967396
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967404
+
+struct vring_avail {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1275404889
+
+ u16 ring[qsz]; /* qsz is the Queue Size field read from device */
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1275404891
+
+ u16 idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1275404921
+
+ u8 pad[254] /* Padding to avoid sharing cache line between flags and
+ idx fields.
+ */
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967533
+
+#define VRING_AVAIL_F_NO_INTERRUPT 1
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967533
+
+ u16 flags;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274968345
+
+ u16 last_seen_used_idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967396
+
+};
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274967715
+If the ring is large enough, the second layout maintains the control and
+ ring structures on separate cache lines.
+\end_layout
+
\begin_layout Subsection
+
+\change_inserted 0 1274968415
+\begin_inset CommandInset label
+LatexCommand label
+name "sub:Used-Ring"
+
+\end_inset
+
+
+\change_unchanged
Used Ring
\end_layout
@@ -2391,12 +2557,20 @@ status open
\begin_layout Plain Layout
-while (vq->last_seen_used != vring->used.idx) {
+while (vq->last_seen_used
+\change_inserted 0 1274968316
+_idx
+\change_unchanged
+ != vring->used.idx) {
\end_layout
\begin_layout Plain Layout
- struct vring_used_elem *e = vring.used->ring[vq->last_seen_used%vsz];
+ struct vring_used_elem *e = vring.used->ring[vq->last_seen_used
+\change_inserted 0 1274968326
+_idx
+\change_unchanged
+%vsz];
\end_layout
\begin_layout Plain Layout
@@ -2406,7 +2580,11 @@ while (vq->last_seen_used != vring->used.idx) {
\begin_layout Plain Layout
- vq->last_seen_used++;
+ vq->last_seen_used
+\change_inserted 0 1274968321
+_idx
+\change_unchanged
+++;
\end_layout
\begin_layout Plain Layout
@@ -2419,6 +2597,15 @@ while (vq->last_seen_used != vring->used.idx) {
\end_layout
+\begin_layout Standard
+
+\change_inserted 0 1275405042
+If PUBLISH_USED feature is negotiated, last_seen_used value should be published
+ to the device in the avail ring.
+ This value is used by the host for interrupt mitigation, so it only need
+ to be updated when interrupts are enabled.
+\end_layout
+
\begin_layout Subsection
Dealing With Configuration Changes
\end_layout
@@ -2986,6 +3173,47 @@ struct vring_avail {
\begin_layout Plain Layout
};
+\change_inserted 0 1274966477
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966484
+
+struct vring_avail_ctrl {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966489
+
+ __u16 flags;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966494
+
+ __u16 idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966499
+
+ __u16 last_used_idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966474
+
+};
\end_layout
\begin_layout Plain Layout
@@ -3349,6 +3577,28 @@ reference "sub:Indirect-Descriptors"
\end_inset
.
+\change_inserted 0 1274967762
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 0 1274967926
+VIRTIO_F_RING_PUBLISH_USED
+\begin_inset space ~
+\end_inset
+
+(29) Negotiating this feature indicates that the avail ring layout includes
+ the used index observed by driver, see
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "PUBLISH_USED-feature"
+
+\end_inset
+
+.
+\change_unchanged
+
\end_layout
\begin_layout Description
^ permalink raw reply related
* [PATCHv3 2/2] virtio: publish used idx
From: Michael S. Tsirkin @ 2010-06-01 14:47 UTC (permalink / raw)
To: Rusty Russell, linux-kernel, virtualization, kvm, qemu-devel
In-Reply-To: <cover.1275403477.git.mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/virtio_net.c | 2 ++
drivers/virtio/virtio_ring.c | 15 +++++++++++++--
include/linux/virtio_ring.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 78eb319..30e7483 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -23,6 +23,7 @@
#include <linux/module.h>
#include <linux/virtio.h>
#include <linux/virtio_net.h>
+#include <linux/virtio_ring.h>
#include <linux/scatterlist.h>
#include <linux/if_vlan.h>
#include <linux/slab.h>
@@ -1056,6 +1057,7 @@ static unsigned int features[] = {
VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
+ VIRTIO_RING_F_PUBLISH_USED,
};
static struct virtio_driver virtio_net_driver = {
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 0f684b4..5a8c711 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -92,6 +92,9 @@ struct vring_virtqueue
/* Last used index we've seen. */
u16 last_used_idx;
+ /* Publish last used index we've seen at this location. */
+ u16 *publish_last_used_idx;
+
/* How to notify other side. FIXME: commonalize hcalls! */
void (*notify)(struct virtqueue *vq);
@@ -325,7 +328,7 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
/* detach_buf clears data, so grab it now. */
ret = vq->data[i];
detach_buf(vq, i);
- vq->last_used_idx++;
+ *vq->publish_last_used_idx = ++vq->last_used_idx;
END_USE(vq);
return ret;
}
@@ -425,13 +428,19 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
if (!vq)
return NULL;
- vring_init(&vq->vring, num, pages, vring_align, false);
+ vring_init(&vq->vring, num, pages, vring_align,
+ virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_USED));
+ if (virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_USED))
+ vq->publish_last_used_idx = &vq->vring.avail->last_used_idx;
+ else
+ vq->publish_last_used_idx = &vq->last_used_idx;
vq->vq.callback = callback;
vq->vq.vdev = vdev;
vq->vq.name = name;
vq->notify = notify;
vq->broken = false;
vq->last_used_idx = 0;
+ *vq->publish_last_used_idx = 0;
vq->num_added = 0;
list_add_tail(&vq->vq.list, &vdev->vqs);
#ifdef DEBUG
@@ -473,6 +482,8 @@ void vring_transport_features(struct virtio_device *vdev)
switch (i) {
case VIRTIO_RING_F_INDIRECT_DESC:
break;
+ case VIRTIO_RING_F_PUBLISH_USED:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev->features);
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 458ce41..87f8fd3 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -29,6 +29,9 @@
/* We support indirect buffer descriptors */
#define VIRTIO_RING_F_INDIRECT_DESC 28
+/* The Guest publishes last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_USED 29
+
/* Virtio ring descriptors: 16 bytes. These can chain together via "next". */
struct vring_desc {
/* Address (guest-physical). */
@@ -51,6 +54,7 @@ struct vring_avail_ctrl {
__u16 idx;
__u8 pad[254];
__u16 flags;
+ __u16 last_used_idx;
};
/* u32 is used here for ids for padding reasons. */
@@ -75,6 +79,7 @@ struct vring {
__u16 *avail_idx;
__u16 *avail_flags;
__u16 *avail_ring;
+ __u16 *last_used_idx;
struct vring_used *used;
};
@@ -100,6 +105,7 @@ struct vring {
* __u16 avail_idx;
* __u8 pad[254]; // Padding to align flags at cache line boundary.
* __u16 avail_flags;
+ * __u16 last_used_idx;
* // Padding to the next align boundary.
* char pad[];
*
@@ -120,14 +126,18 @@ static inline void vring_init(struct vring *vr, unsigned int num, void *p,
vr->avail_ring = (void*)avail;
vr->avail_idx = &ctrl->idx;
vr->avail_flags = &ctrl->flags;
+ vr->last_used_idx = &ctrl->last_used_idx;
} else {
vr->avail_idx = &avail->idx;
vr->avail_flags = &avail->flags;
+ vr->last_used_idx = NULL;
}
vr->used = (void *)ALIGN((unsigned long)&avail->ring[num], align);
/* Verify that avail fits before used. */
BUG_ON((unsigned long)(vr->avail_flags + 1) > (unsigned long)vr->used);
BUG_ON((unsigned long)(vr->avail_idx + 1) > (unsigned long)vr->used);
+ BUG_ON(vr->last_used_idx && (unsigned long)(vr->last_used_idx + 1) >
+ (unsigned long)vr->used);
BUG_ON((unsigned long)(&vr->avail_ring[num]) > (unsigned long)vr->used);
}
--
1.7.1.12.g42b7f
^ permalink raw reply related
* [PATCHv3 1/2] virtio: support layout with avail ring before idx
From: Michael S. Tsirkin @ 2010-06-01 14:47 UTC (permalink / raw)
To: Rusty Russell, linux-kernel, virtualization, kvm, qemu-devel
In-Reply-To: <cover.1275403477.git.mst@redhat.com>
This adds an (unused) option to put available ring before control (avail
index, flags), and adds padding between index and flags. This avoids
cache line sharing between control and ring, and also makes it possible
to extend avail control without incurring extra cache misses.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_ring.c | 12 ++++++------
include/linux/virtio_ring.h | 40 ++++++++++++++++++++++++++++++++++------
2 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 1ca8890..0f684b4 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -223,8 +223,8 @@ add_head:
/* Put entry in available array (but don't update avail->idx until they
* do sync). FIXME: avoid modulus here? */
- avail = (vq->vring.avail->idx + vq->num_added++) % vq->vring.num;
- vq->vring.avail->ring[avail] = head;
+ avail = (*vq->vring.avail_idx + vq->num_added++) % vq->vring.num;
+ vq->vring.avail_ring[avail] = head;
pr_debug("Added buffer head %i to %p\n", head, vq);
END_USE(vq);
@@ -244,7 +244,7 @@ void virtqueue_kick(struct virtqueue *_vq)
* new available array entries. */
virtio_wmb();
- vq->vring.avail->idx += vq->num_added;
+ *vq->vring.avail_idx += vq->num_added;
vq->num_added = 0;
/* Need to update avail index before checking if we should notify */
@@ -335,7 +335,7 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
+ *vq->vring.avail_flags |= VRING_AVAIL_F_NO_INTERRUPT;
}
EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
@@ -347,7 +347,7 @@ bool virtqueue_enable_cb(struct virtqueue *_vq)
/* We optimistically turn back on interrupts, then check if there was
* more to do. */
- vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
+ vq->vring.avail_flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
virtio_mb();
if (unlikely(more_used(vq))) {
END_USE(vq);
@@ -425,7 +425,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
if (!vq)
return NULL;
- vring_init(&vq->vring, num, pages, vring_align);
+ vring_init(&vq->vring, num, pages, vring_align, false);
vq->vq.callback = callback;
vq->vq.vdev = vdev;
vq->vq.name = name;
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index e4d144b..458ce41 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -47,6 +47,12 @@ struct vring_avail {
__u16 ring[];
};
+struct vring_avail_ctrl {
+ __u16 idx;
+ __u8 pad[254];
+ __u16 flags;
+};
+
/* u32 is used here for ids for padding reasons. */
struct vring_used_elem {
/* Index of start of used descriptor chain. */
@@ -66,7 +72,9 @@ struct vring {
struct vring_desc *desc;
- struct vring_avail *avail;
+ __u16 *avail_idx;
+ __u16 *avail_flags;
+ __u16 *avail_ring;
struct vring_used *used;
};
@@ -79,11 +87,19 @@ struct vring {
* // The actual descriptors (16 bytes each)
* struct vring_desc desc[num];
*
- * // A ring of available descriptor heads with free-running index.
+ * // A ring of available descriptor heads with a control structure
+ * // including a free-running index.
+ * // The ring can come either after (legacy) or before the control.
* __u16 avail_flags;
* __u16 avail_idx;
* __u16 available[num];
*
+ * or
+ *
+ * __u16 available[num];
+ * __u16 avail_idx;
+ * __u8 pad[254]; // Padding to align flags at cache line boundary.
+ * __u16 avail_flags;
* // Padding to the next align boundary.
* char pad[];
*
@@ -94,13 +110,25 @@ struct vring {
* };
*/
static inline void vring_init(struct vring *vr, unsigned int num, void *p,
- unsigned long align)
+ unsigned long align, bool avail_ring_first)
{
+ struct vring_avail *avail = p + num * sizeof(struct vring_desc);
vr->num = num;
vr->desc = p;
- vr->avail = p + num*sizeof(struct vring_desc);
- vr->used = (void *)(((unsigned long)&vr->avail->ring[num] + align-1)
- & ~(align - 1));
+ if (avail_ring_first) {
+ struct vring_avail_ctrl *ctrl = (void *)&vr->avail_ring[num];
+ vr->avail_ring = (void*)avail;
+ vr->avail_idx = &ctrl->idx;
+ vr->avail_flags = &ctrl->flags;
+ } else {
+ vr->avail_idx = &avail->idx;
+ vr->avail_flags = &avail->flags;
+ }
+ vr->used = (void *)ALIGN((unsigned long)&avail->ring[num], align);
+ /* Verify that avail fits before used. */
+ BUG_ON((unsigned long)(vr->avail_flags + 1) > (unsigned long)vr->used);
+ BUG_ON((unsigned long)(vr->avail_idx + 1) > (unsigned long)vr->used);
+ BUG_ON((unsigned long)(&vr->avail_ring[num]) > (unsigned long)vr->used);
}
static inline unsigned vring_size(unsigned int num, unsigned long align)
--
1.7.1.12.g42b7f
^ permalink raw reply related
* [PATCHv3 0/2] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-06-01 14:47 UTC (permalink / raw)
To: Rusty Russell, linux-kernel, virtualization, kvm, qemu-devel
Changes from v2: added padding between avail idx and flags,
and changed virtio to only publish used index when callbacks
are enabled.
Here's a rewrite of the original patch with a new layout.
I haven't tested it yet so no idea how this performs, but
I think this addresses the cache bounce issue raised by Avi.
Posting for early flames/comments.
Generally, the Host end of the virtio ring doesn't need to see where
Guest is up to in consuming the ring. However, to completely understand
what's going on from the outside, this information must be exposed.
For example, host can reduce the number of interrupts by detecting
that the guest is currently handling previous buffers.
We add a feature bit so the guest can tell the host that it's writing
out the current value there, if it wants to use that.
This differs from original approach in that the used index
is put after avail index (they are typically written out together).
To avoid cache bounces on descriptor access,
and make future extensions easier, we put the ring itself at start of
page, and move the control after it.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Michael S. Tsirkin (2):
virtio: support layout with avail ring before idx
virtio: publish used idx
drivers/net/virtio_net.c | 2 +
drivers/virtio/virtio_ring.c | 25 +++++++++++++++------
include/linux/virtio_ring.h | 50 ++++++++++++++++++++++++++++++++++++-----
3 files changed, 64 insertions(+), 13 deletions(-)
^ permalink raw reply
* Re: [PATCHv2-RFC 0/2] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-31 13:29 UTC (permalink / raw)
To: Jes Sorensen; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <4C0366E0.6040203@redhat.com>
On Mon, May 31, 2010 at 09:36:00AM +0200, Jes Sorensen wrote:
> On 05/30/10 13:22, Michael S. Tsirkin wrote:
> > On Fri, May 28, 2010 at 11:56:54AM +0200, Jes Sorensen wrote:
> >> It looks pretty good to me, however one thing I have been thinking of
> >> while reading through it:
> >>
> >> Rather than storing a pointer within the ring struct, pointing into a
> >> position within the same struct. How about storing a byte offset instead
> >> and using a cast to get to the pointer position? That would avoid the
> >> pointer dereference, which is less effective cache wise and harder for
> >> the CPU to predict.
> >>
> >> Not sure whether it really matters performance wise, just a thought.
> >
> > I think this won't work: when PUBLUSH_USED_IDX is negotiated,
> > the pointer is to within the ring.
>
> Hmmm shame, it would be a nice optimization.
>
> Maybe it's time to introduce the v2 ring format, rather than having
> adding more kludges to the existing one?
>
> Cheers,
> Jes
There has been discussion about a ring format that does not
use indexes at all. My guess is that would be a good point
for v2 ring format. But making that a product
and tuning might take a while. So definitely something to
keep in mind but I would not want that to block this optimization.
--
MST
^ permalink raw reply
* Re: [PATCHv2-RFC 0/2] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-31 12:20 UTC (permalink / raw)
To: Rusty Russell; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <201005311716.43573.rusty@rustcorp.com.au>
On Mon, May 31, 2010 at 05:16:42PM +0930, Rusty Russell wrote:
> On Thu, 27 May 2010 05:20:35 am Michael S. Tsirkin wrote:
> > Here's a rewrite of the original patch with a new layout.
> > I haven't tested it yet so no idea how this performs, but
> > I think this addresses the cache bounce issue raised by Avi.
> > Posting for early flames/comments.
>
> Sorry, not without some evidence that it'll actually reduce cacheline
> bouncing. I *think* it will, but it's not obvious: the host may keep
> looking at avail_idx as we're updating last_seen. Or does qemu always
> look at both together anyway?
> Can someone convince me this is a win?
> Rusty.
What really happens is host looks at flags and last_seen together.
And flags happens to be in the same cache line with avail idx.
So to get an obvious win, we should put flags and last_seen
in a separate cache line from avail, which us easy - just add some padding.
And I'll relax the requirement from guest to only require it to update
last_seen when interrupts are enabled. This way flags and
last_seen are written together and read together.
Makes sense?
--
MST
^ permalink raw reply
* Re: [PATCH 2/2] virtio: console: Fix crash when port is unplugged and blocked for write
From: Amit Shah @ 2010-05-31 7:58 UTC (permalink / raw)
To: Rusty Russell; +Cc: Virtualization List
In-Reply-To: <201005311725.36271.rusty@rustcorp.com.au>
On (Mon) May 31 2010 [17:25:35], Rusty Russell wrote:
> On Thu, 27 May 2010 05:24:40 pm Amit Shah wrote:
> > When a program that has a virtio port opened and blocked for a write
> > operation, a port hot-unplug event will later led to a crash when
> > SIGTERM was sent to the program. Fix that.
> >
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
>
> Thanks, both applied.
Please push for 2.6.35 as well. Thanks.
Amit
^ permalink raw reply
* Re: [PATCH 2/2] virtio: console: Fix crash when port is unplugged and blocked for write
From: Rusty Russell @ 2010-05-31 7:55 UTC (permalink / raw)
To: Amit Shah; +Cc: Virtualization List
In-Reply-To: <db3f30b6211d5a0b4d4e0acaddc6cedf9018e749.1274946878.git.amit.shah@redhat.com>
On Thu, 27 May 2010 05:24:40 pm Amit Shah wrote:
> When a program that has a virtio port opened and blocked for a write
> operation, a port hot-unplug event will later led to a crash when
> SIGTERM was sent to the program. Fix that.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
Thanks, both applied.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCHv2-RFC 0/2] virtio: put last seen used index into ring itself
From: Rusty Russell @ 2010-05-31 7:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <cover.1274903328.git.mst@redhat.com>
On Thu, 27 May 2010 05:20:35 am Michael S. Tsirkin wrote:
> Here's a rewrite of the original patch with a new layout.
> I haven't tested it yet so no idea how this performs, but
> I think this addresses the cache bounce issue raised by Avi.
> Posting for early flames/comments.
Sorry, not without some evidence that it'll actually reduce cacheline
bouncing. I *think* it will, but it's not obvious: the host may keep
looking at avail_idx as we're updating last_seen. Or does qemu always
look at both together anyway?
Can someone convince me this is a win?
Rusty.
^ permalink raw reply
* Re: [PATCHv2-RFC 0/2] virtio: put last seen used index into ring itself
From: Jes Sorensen @ 2010-05-31 7:36 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <20100530112220.GA27611@redhat.com>
On 05/30/10 13:22, Michael S. Tsirkin wrote:
> On Fri, May 28, 2010 at 11:56:54AM +0200, Jes Sorensen wrote:
>> It looks pretty good to me, however one thing I have been thinking of
>> while reading through it:
>>
>> Rather than storing a pointer within the ring struct, pointing into a
>> position within the same struct. How about storing a byte offset instead
>> and using a cast to get to the pointer position? That would avoid the
>> pointer dereference, which is less effective cache wise and harder for
>> the CPU to predict.
>>
>> Not sure whether it really matters performance wise, just a thought.
>
> I think this won't work: when PUBLUSH_USED_IDX is negotiated,
> the pointer is to within the ring.
Hmmm shame, it would be a nice optimization.
Maybe it's time to introduce the v2 ring format, rather than having
adding more kludges to the existing one?
Cheers,
Jes
^ permalink raw reply
* Re: [PATCHv2-RFC 0/2] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-30 11:22 UTC (permalink / raw)
To: Jes Sorensen; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <4BFF9366.5090103@redhat.com>
On Fri, May 28, 2010 at 11:56:54AM +0200, Jes Sorensen wrote:
> On 05/26/10 21:50, Michael S. Tsirkin wrote:
> > Here's a rewrite of the original patch with a new layout.
> > I haven't tested it yet so no idea how this performs, but
> > I think this addresses the cache bounce issue raised by Avi.
> > Posting for early flames/comments.
> >
> > Generally, the Host end of the virtio ring doesn't need to see where
> > Guest is up to in consuming the ring. However, to completely understand
> > what's going on from the outside, this information must be exposed.
> > For example, host can reduce the number of interrupts by detecting
> > that the guest is currently handling previous buffers.
> >
> > We add a feature bit so the guest can tell the host that it's writing
> > out the current value there, if it wants to use that.
> >
> > This differs from original approach in that the used index
> > is put after avail index (they are typically written out together).
> > To avoid cache bounces on descriptor access,
> > and make future extensions easier, we put the ring itself at start of
> > page, and move the control after it.
>
> Hi Michael,
>
> It looks pretty good to me, however one thing I have been thinking of
> while reading through it:
>
> Rather than storing a pointer within the ring struct, pointing into a
> position within the same struct. How about storing a byte offset instead
> and using a cast to get to the pointer position? That would avoid the
> pointer dereference, which is less effective cache wise and harder for
> the CPU to predict.
>
> Not sure whether it really matters performance wise, just a thought.
>
> Cheers,
> Jes
I think this won't work: when PUBLUSH_USED_IDX is negotiated,
the pointer is to within the ring.
--
MST
^ permalink raw reply
* [PATCH 1/1] staging: hv: Fix race condition on vmbus channel initialization
From: Haiyang Zhang @ 2010-05-28 23:22 UTC (permalink / raw)
To: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
"'virtualization@lists.osdl.org'" <virtualiz>
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: [PATCH] staging: hv: Fix race condition on vmbus channel initialization
There is a possible race condition when hv_utils starts to load immediately
after hv_vmbus is loading - null pointer error could happen.
This patch added wait/completion to ensure all channels are ready before
vmbus loading completes. So another module won't have any uninitialized channel.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/channel_mgmt.c | 41 +++++++++++++++++++++++++++----------
drivers/staging/hv/vmbus.h | 2 +
drivers/staging/hv/vmbus_drv.c | 3 ++
3 files changed, 35 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..12db555 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -23,6 +23,7 @@
#include <linux/slab.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/completion.h>
#include "osd.h"
#include "logging.h"
#include "vmbus_private.h"
@@ -293,6 +294,25 @@ void FreeVmbusChannel(struct vmbus_channel *Channel)
Channel);
}
+
+DECLARE_COMPLETION(hv_channel_ready);
+
+/*
+ * Count initialized channels, and ensure all channels are ready when hv_vmbus
+ * module loading completes.
+ */
+static void count_hv_channel(void)
+{
+ static int counter;
+ unsigned long flags;
+
+ spin_lock_irqsave(&gVmbusConnection.channel_lock, flags);
+ if (++counter == MAX_MSG_TYPES)
+ complete(&hv_channel_ready);
+ spin_unlock_irqrestore(&gVmbusConnection.channel_lock, flags);
+}
+
+
/*
* VmbusChannelProcessOffer - Process the offer by creating a channel/device
* associated with this offer
@@ -373,22 +393,21 @@ static void VmbusChannelProcessOffer(void *context)
* can cleanup properly
*/
newChannel->State = CHANNEL_OPEN_STATE;
- cnt = 0;
- while (cnt != MAX_MSG_TYPES) {
+ /* Open IC channels */
+ for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
&hv_cb_utils[cnt].data,
- sizeof(struct hv_guid)) == 0) {
+ sizeof(struct hv_guid)) == 0 &&
+ VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
+ 2 * PAGE_SIZE, NULL, 0,
+ hv_cb_utils[cnt].callback,
+ newChannel) == 0) {
+ hv_cb_utils[cnt].channel = newChannel;
DPRINT_INFO(VMBUS, "%s",
- hv_cb_utils[cnt].log_msg);
-
- if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
- 2 * PAGE_SIZE, NULL, 0,
- hv_cb_utils[cnt].callback,
- newChannel) == 0)
- hv_cb_utils[cnt].channel = newChannel;
+ hv_cb_utils[cnt].log_msg);
+ count_hv_channel();
}
- cnt++;
}
}
DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/vmbus.h b/drivers/staging/hv/vmbus.h
index 0c6ee0f..3c14b29 100644
--- a/drivers/staging/hv/vmbus.h
+++ b/drivers/staging/hv/vmbus.h
@@ -74,4 +74,6 @@ int vmbus_child_driver_register(struct driver_context *driver_ctx);
void vmbus_child_driver_unregister(struct driver_context *driver_ctx);
void vmbus_get_interface(struct vmbus_channel_interface *interface);
+extern struct completion hv_channel_ready;
+
#endif /* _VMBUS_H_ */
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index c21731a..22c80ec 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -27,6 +27,7 @@
#include <linux/pci.h>
#include <linux/dmi.h>
#include <linux/slab.h>
+#include <linux/completion.h>
#include "version_info.h"
#include "osd.h"
#include "logging.h"
@@ -356,6 +357,8 @@ static int vmbus_bus_init(int (*drv_init)(struct hv_driver *drv))
vmbus_drv_obj->GetChannelOffers();
+ wait_for_completion(&hv_channel_ready);
+
cleanup:
DPRINT_EXIT(VMBUS_DRV);
--
1.6.3.2
^ permalink raw reply related
* [Reminder] KVM Forum 2010: Early Bird Registration
From: KVM Forum 2010 Program Committee @ 2010-05-28 19:20 UTC (permalink / raw)
To: kvm, qemu-devel, virtualization, libvir-list
Just a reminder...The early bird registration period ends on May 30th.
It's shaping up to be an excellent KVM Forum, look forward to seeing you
there.
Registration link is here:
http://events.linuxfoundation.org/component/registrationpro/?func=details&did=34
thanks,
-KVM Forum 2010 Program Commitee
^ permalink raw reply
* Re: [PATCHv2-RFC 0/2] virtio: put last seen used index into ring itself
From: Jes Sorensen @ 2010-05-28 9:56 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <cover.1274903328.git.mst@redhat.com>
On 05/26/10 21:50, Michael S. Tsirkin wrote:
> Here's a rewrite of the original patch with a new layout.
> I haven't tested it yet so no idea how this performs, but
> I think this addresses the cache bounce issue raised by Avi.
> Posting for early flames/comments.
>
> Generally, the Host end of the virtio ring doesn't need to see where
> Guest is up to in consuming the ring. However, to completely understand
> what's going on from the outside, this information must be exposed.
> For example, host can reduce the number of interrupts by detecting
> that the guest is currently handling previous buffers.
>
> We add a feature bit so the guest can tell the host that it's writing
> out the current value there, if it wants to use that.
>
> This differs from original approach in that the used index
> is put after avail index (they are typically written out together).
> To avoid cache bounces on descriptor access,
> and make future extensions easier, we put the ring itself at start of
> page, and move the control after it.
Hi Michael,
It looks pretty good to me, however one thing I have been thinking of
while reading through it:
Rather than storing a pointer within the ring struct, pointing into a
position within the same struct. How about storing a byte offset instead
and using a cast to get to the pointer position? That would avoid the
pointer dereference, which is less effective cache wise and harder for
the CPU to predict.
Not sure whether it really matters performance wise, just a thought.
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)
From: Greg KH @ 2010-05-27 23:22 UTC (permalink / raw)
To: Haiyang Zhang
Cc: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <1FB5E1D5CA062146B38059374562DF7266B8C7C4@TK5EX14MBXC128.redmond.corp.microsoft.com>
On Wed, May 26, 2010 at 10:52:36PM +0000, Haiyang Zhang wrote:
> > From: Greg KH [mailto:gregkh@suse.de]
> > How about a lock!
> >
> > What's so scary about a pretty little semaphore? They are all cute and
> > cuddley and don't bite anyone. You should not be afraid to use them,
> > they are here to do your bidding.
>
> No problem, we will add a lock here.
>
> > > The VmbusChannelProcessOffer() is called from interrupt context, and
> > > initialize the channels, wake up vmbus_init when all channels are
> > > ready. If using local variable only, how to pass the channel ready
> > > info to vmbus_init() which is in a different context?
> >
> > No, I mean move the logic you added here, into the vmbus_init() call.
>
> Do you mean:
> Move the event creat/wait/free, which is currently in vmbus_init(),
> into vmbus_bus_init() function.
Yes.
> hv_channle_ready will still be a global variable.
That's fine.
> And, the wakeup call -- osd_WaitEventSet() --remains in
> VmbusChannelProcessOffer() ?
Yes.
And as Jiri pointed out, this should be a simple completion.
thanks,
greg k-h
^ permalink raw reply
* Re: [GIT PULL net-2.6] vhost-net: error handling fixes
From: David Miller @ 2010-05-27 23:13 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization, netdev, linux-kernel, krkumar2
In-Reply-To: <20100527115714.GA7899@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 27 May 2010 14:57:14 +0300
> David,
> The following tree includes fixes dealing with error handling
> in vhost-net. It is on top of net-2.6.
> Please merge it for 2.6.35.
> Thanks!
>
> The following changes since commit 8a74ad60a546b13bd1096b2a61a7a5c6fd9ae17c:
>
> net: fix lock_sock_bh/unlock_sock_bh (2010-05-27 00:30:53 -0700)
>
> are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net
Pulled, thanks Michael.
^ permalink raw reply
* Re: [PATCHv2-RFC 0/2] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-27 13:59 UTC (permalink / raw)
To: Avi Kivity; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <4BFE6098.9010000@redhat.com>
On Thu, May 27, 2010 at 03:07:52PM +0300, Avi Kivity wrote:
> I missed the spec patch, can you repost it?
Still work in progress, but here it is.
Note I am still debating with myself whether we should split
avail idx and flags into separate cache lines.
diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index ed35893..150e5a8 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -1803,6 +1803,36 @@ next
\emph default
descriptor entry (modulo the ring size).
This starts at 0, and increases.
+\change_inserted 0 1274966643
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274968378
+When PUBLISH_USED feature flag has
+\emph on
+not
+\emph default
+ been negotiated, the ring follows the
+\begin_inset Quotes eld
+\end_inset
+
+flags
+\begin_inset Quotes erd
+\end_inset
+
+ and the
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ fields:
+\change_unchanged
+
\end_layout
\begin_layout Standard
@@ -1845,7 +1875,134 @@ struct vring_avail {
\end_layout
+\begin_layout Standard
+
+\change_inserted 0 1274968432
+\begin_inset CommandInset label
+LatexCommand label
+name "PUBLISH_USED-feature"
+
+\end_inset
+
+When PUBLISH_USED feature flag has been negotiated, the control structure
+ including the
+\begin_inset Quotes eld
+\end_inset
+
+flags and the
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ fields follows the ring.
+ This leaves the room for the
+\begin_inset Quotes eld
+\end_inset
+
+last_seen_used_idx
+\begin_inset Quotes erd
+\end_inset
+
+ field, which indicates the most recent
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ value observed by guest in the used ring (see
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "sub:Used-Ring"
+
+\end_inset
+
+ below):
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274967396
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967404
+
+struct vring_avail {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967405
+
+ u16 ring[qsz]; /* qsz is the Queue Size field read from device */
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967533
+
+#define VRING_AVAIL_F_NO_INTERRUPT 1
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967533
+
+ u16 flags;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967533
+
+ u16 idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274968345
+
+ u16 last_seen_used_idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967396
+
+};
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274967715
+If the ring is large enough, the second layout maintains the control and
+ ring structures on separate cache lines.
+\end_layout
+
\begin_layout Subsection
+
+\change_inserted 0 1274968415
+\begin_inset CommandInset label
+LatexCommand label
+name "sub:Used-Ring"
+
+\end_inset
+
+
+\change_unchanged
Used Ring
\end_layout
@@ -2391,12 +2548,20 @@ status open
\begin_layout Plain Layout
-while (vq->last_seen_used != vring->used.idx) {
+while (vq->last_seen_used
+\change_inserted 0 1274968316
+_idx
+\change_unchanged
+ != vring->used.idx) {
\end_layout
\begin_layout Plain Layout
- struct vring_used_elem *e = vring.used->ring[vq->last_seen_used%vsz];
+ struct vring_used_elem *e = vring.used->ring[vq->last_seen_used
+\change_inserted 0 1274968326
+_idx
+\change_unchanged
+%vsz];
\end_layout
\begin_layout Plain Layout
@@ -2406,7 +2571,11 @@ while (vq->last_seen_used != vring->used.idx) {
\begin_layout Plain Layout
- vq->last_seen_used++;
+ vq->last_seen_used
+\change_inserted 0 1274968321
+_idx
+\change_unchanged
+++;
\end_layout
\begin_layout Plain Layout
@@ -2419,6 +2588,13 @@ while (vq->last_seen_used != vring->used.idx) {
\end_layout
+\begin_layout Standard
+
+\change_inserted 0 1274968252
+If PUBLISH_USED feature is negotiated, last_seen_used value should be published
+ to the device in the avail ring.
+\end_layout
+
\begin_layout Subsection
Dealing With Configuration Changes
\end_layout
@@ -2986,6 +3162,47 @@ struct vring_avail {
\begin_layout Plain Layout
};
+\change_inserted 0 1274966477
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966484
+
+struct vring_avail_ctrl {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966489
+
+ __u16 flags;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966494
+
+ __u16 idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966499
+
+ __u16 last_used_idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966474
+
+};
\end_layout
\begin_layout Plain Layout
@@ -3349,6 +3566,28 @@ reference "sub:Indirect-Descriptors"
\end_inset
.
+\change_inserted 0 1274967762
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 0 1274967926
+VIRTIO_F_RING_PUBLISH_USED
+\begin_inset space ~
+\end_inset
+
+(29) Negotiating this feature indicates that the avail ring layout includes
+ the used index observed by driver, see
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "PUBLISH_USED-feature"
+
+\end_inset
+
+.
+\change_unchanged
+
\end_layout
\begin_layout Description
^ permalink raw reply related
* Re: [PATCHv2-RFC 0/2] virtio: put last seen used index into ring itself
From: Avi Kivity @ 2010-05-27 12:07 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <cover.1274903328.git.mst@redhat.com>
On 05/26/2010 10:50 PM, Michael S. Tsirkin wrote:
> Here's a rewrite of the original patch with a new layout.
> I haven't tested it yet so no idea how this performs, but
> I think this addresses the cache bounce issue raised by Avi.
> Posting for early flames/comments.
>
> Generally, the Host end of the virtio ring doesn't need to see where
> Guest is up to in consuming the ring. However, to completely understand
> what's going on from the outside, this information must be exposed.
> For example, host can reduce the number of interrupts by detecting
> that the guest is currently handling previous buffers.
>
> We add a feature bit so the guest can tell the host that it's writing
> out the current value there, if it wants to use that.
>
> This differs from original approach in that the used index
> is put after avail index (they are typically written out together).
> To avoid cache bounces on descriptor access,
> and make future extensions easier, we put the ring itself at start of
> page, and move the control after it.
>
I missed the spec patch, can you repost it?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* [GIT PULL net-2.6] vhost-net: error handling fixes
From: Michael S. Tsirkin @ 2010-05-27 11:57 UTC (permalink / raw)
To: David Miller; +Cc: kvm, virtualization, netdev, linux-kernel, krkumar2
David,
The following tree includes fixes dealing with error handling
in vhost-net. It is on top of net-2.6.
Please merge it for 2.6.35.
Thanks!
The following changes since commit 8a74ad60a546b13bd1096b2a61a7a5c6fd9ae17c:
net: fix lock_sock_bh/unlock_sock_bh (2010-05-27 00:30:53 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net
Krishna Kumar (1):
vhost: Fix host panic if ioctl called with wrong index
Takuya Yoshikawa (3):
vhost: fix to check the return value of copy_to/from_user() correctly
vhost-net: fix to check the return value of copy_to/from_user() correctly
vhost: fix the memory leak which will happen when memory_access_ok fails
drivers/vhost/net.c | 14 ++++++------
drivers/vhost/vhost.c | 57 +++++++++++++++++++++++++++---------------------
2 files changed, 39 insertions(+), 32 deletions(-)
--
MST
^ 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