Re: Merge of virtio_net: fix race in RX VQ processing for linux-3.2.48
On Sat, Jul 27, 2013 at 03:42:53PM +0100, Ben Hutchings wrote:
On Fri, 2013-07-12 at 23:13 +0200, Wolfram Gloger wrote:
Hi,
Today I merged M. Tsirkin's patch v2 virtio_net: fix race in RX VQ
processing:
http://lkml.indiana.edu/hypermail/linux/kernel/1307.1/00503.html
into 3.2.48 and lightly tested the result (vhost-net, virtio-disk
and 9pfs using virtio).
This sounds like it could be suitable for stable, but that doesn't seem
to have been requested by the author.
It was in the cover letter:
Please review, and consider for 3.11 and stable.
I'm cc'ing those involved so they
can make a decision whether this should be included in 3.2.y or other
stable branches.
Merge is not completely trivial, so might save you some time, if only
for a comparison that you arrive at the same result.
Thanks, but these are not in quite the right patch format. The
filenames should always have a leading directory name which will be
stripped (patch -p1). The original patch header must be preserved and a
reference to the upstream commit inserted e.g.
'commit 0123456789abcdef0123456789abcdef012345678 upstream.'
(Attachments copied for reference.)
Ben.
--
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.
--- drivers/virtio/virtio_ring.c.orig 2013-06-30 11:35:44.0 +0200
+++ drivers/virtio/virtio_ring.c 2013-07-12 15:18:48.0 +0200
@@ -360,9 +360,22 @@ void virtqueue_disable_cb(struct virtque
}
EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
-bool virtqueue_enable_cb(struct virtqueue *_vq)
+/**
+ * virtqueue_enable_cb_prepare - restart callbacks after disable_cb
+ * @vq: the struct virtqueue we're talking about.
+ *
+ * This re-enables callbacks; it returns current queue state
+ * in an opaque unsigned value. This value should be later tested by
+ * virtqueue_poll, to detect a possible race between the driver checking for
+ * more work, and enabling callbacks.
+ *
+ * Caller must ensure we don't call this with other virtqueue
+ * operations at the same time (except where noted).
+ */
+unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ u16 last_used_idx;
START_USE(vq);
@@ -372,15 +385,45 @@ bool virtqueue_enable_cb(struct virtqueu
* 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;
+ vring_used_event(vq-vring) = last_used_idx = vq-last_used_idx;
+ END_USE(vq);
+ return last_used_idx;
+}
+EXPORT_SYMBOL_GPL(virtqueue_enable_cb_prepare);
+
+/**
+ * virtqueue_poll - query pending used buffers
+ * @vq: the struct virtqueue we're talking about.
+ * @last_used_idx: virtqueue state (from call to
virtqueue_enable_cb_prepare).
+ *
+ * Returns true if there are pending used buffers in the queue.
+ *
+ * This does not need to be serialized.
+ */
+bool virtqueue_poll(struct virtqueue *_vq, unsigned last_used_idx)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+
virtio_mb();
- if (unlikely(more_used(vq))) {
- END_USE(vq);
- return false;
- }
+ return (u16)last_used_idx != vq-vring.used-idx;
+}
+EXPORT_SYMBOL_GPL(virtqueue_poll);
- END_USE(vq);
- return true;
+/**
+ * virtqueue_enable_cb - restart callbacks after disable_cb.
+ * @vq: the struct virtqueue we're talking about.
+ *
+ * This re-enables callbacks; it returns false if there are pending
+ * buffers in the queue, to detect a possible race between the driver
+ * checking for more work, and enabling callbacks.
+ *
+ * Caller must ensure we don't call this with other virtqueue
+ * operations at the same time (except where noted).
+ */
+bool virtqueue_enable_cb(struct virtqueue *_vq)
+{
+ unsigned last_used_idx = virtqueue_enable_cb_prepare(_vq);
+ return !virtqueue_poll(_vq, last_used_idx);
}
EXPORT_SYMBOL_GPL(virtqueue_enable_cb);
--- include/linux/virtio.h.orig 2012-01-05 00:55:44.0 +0100
+++ include/linux/virtio.h2013-07-12 14:42:26.0 +0200
@@ -96,6 +96,10 @@ void virtqueue_disable_cb(struct virtque
bool virtqueue_enable_cb(struct virtqueue *vq);
+unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq);
+
+bool virtqueue_poll(struct virtqueue *vq, unsigned);
+
bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
void *virtqueue_detach_unused_buf(struct virtqueue *vq);
--- drivers/net/virtio_net.c.orig 2012-01-05 00:55:44.0 +0100
+++ drivers/net/virtio_net.c 2013-07-12 15:39:06.0 +0200
@@ -508,7 +508,7 @@ static int virtnet_poll(struct napi_stru
{
struct virtnet_info *vi = container_of(napi, struct virtnet_info, napi);
void *buf;
- unsigned int len, received = 0;
+
Re: Merge of virtio_net: fix race in RX VQ processing for linux-3.2.48
On Fri, 2013-07-12 at 23:13 +0200, Wolfram Gloger wrote:
Hi,
Today I merged M. Tsirkin's patch v2 virtio_net: fix race in RX VQ
processing:
http://lkml.indiana.edu/hypermail/linux/kernel/1307.1/00503.html
into 3.2.48 and lightly tested the result (vhost-net, virtio-disk
and 9pfs using virtio).
This sounds like it could be suitable for stable, but that doesn't seem
to have been requested by the author. I'm cc'ing those involved so they
can make a decision whether this should be included in 3.2.y or other
stable branches.
Merge is not completely trivial, so might save you some time, if only
for a comparison that you arrive at the same result.
Thanks, but these are not in quite the right patch format. The
filenames should always have a leading directory name which will be
stripped (patch -p1). The original patch header must be preserved and a
reference to the upstream commit inserted e.g.
'commit 0123456789abcdef0123456789abcdef012345678 upstream.'
(Attachments copied for reference.)
Ben.
--
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.
--- drivers/virtio/virtio_ring.c.orig 2013-06-30 11:35:44.0 +0200
+++ drivers/virtio/virtio_ring.c 2013-07-12 15:18:48.0 +0200
@@ -360,9 +360,22 @@ void virtqueue_disable_cb(struct virtque
}
EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
-bool virtqueue_enable_cb(struct virtqueue *_vq)
+/**
+ * virtqueue_enable_cb_prepare - restart callbacks after disable_cb
+ * @vq: the struct virtqueue we're talking about.
+ *
+ * This re-enables callbacks; it returns current queue state
+ * in an opaque unsigned value. This value should be later tested by
+ * virtqueue_poll, to detect a possible race between the driver checking for
+ * more work, and enabling callbacks.
+ *
+ * Caller must ensure we don't call this with other virtqueue
+ * operations at the same time (except where noted).
+ */
+unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ u16 last_used_idx;
START_USE(vq);
@@ -372,15 +385,45 @@ bool virtqueue_enable_cb(struct virtqueu
* 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;
+ vring_used_event(vq-vring) = last_used_idx = vq-last_used_idx;
+ END_USE(vq);
+ return last_used_idx;
+}
+EXPORT_SYMBOL_GPL(virtqueue_enable_cb_prepare);
+
+/**
+ * virtqueue_poll - query pending used buffers
+ * @vq: the struct virtqueue we're talking about.
+ * @last_used_idx: virtqueue state (from call to virtqueue_enable_cb_prepare).
+ *
+ * Returns true if there are pending used buffers in the queue.
+ *
+ * This does not need to be serialized.
+ */
+bool virtqueue_poll(struct virtqueue *_vq, unsigned last_used_idx)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+
virtio_mb();
- if (unlikely(more_used(vq))) {
- END_USE(vq);
- return false;
- }
+ return (u16)last_used_idx != vq-vring.used-idx;
+}
+EXPORT_SYMBOL_GPL(virtqueue_poll);
- END_USE(vq);
- return true;
+/**
+ * virtqueue_enable_cb - restart callbacks after disable_cb.
+ * @vq: the struct virtqueue we're talking about.
+ *
+ * This re-enables callbacks; it returns false if there are pending
+ * buffers in the queue, to detect a possible race between the driver
+ * checking for more work, and enabling callbacks.
+ *
+ * Caller must ensure we don't call this with other virtqueue
+ * operations at the same time (except where noted).
+ */
+bool virtqueue_enable_cb(struct virtqueue *_vq)
+{
+ unsigned last_used_idx = virtqueue_enable_cb_prepare(_vq);
+ return !virtqueue_poll(_vq, last_used_idx);
}
EXPORT_SYMBOL_GPL(virtqueue_enable_cb);
--- include/linux/virtio.h.orig 2012-01-05 00:55:44.0 +0100
+++ include/linux/virtio.h 2013-07-12 14:42:26.0 +0200
@@ -96,6 +96,10 @@ void virtqueue_disable_cb(struct virtque
bool virtqueue_enable_cb(struct virtqueue *vq);
+unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq);
+
+bool virtqueue_poll(struct virtqueue *vq, unsigned);
+
bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
void *virtqueue_detach_unused_buf(struct virtqueue *vq);
--- drivers/net/virtio_net.c.orig 2012-01-05 00:55:44.0 +0100
+++ drivers/net/virtio_net.c 2013-07-12 15:39:06.0 +0200
@@ -508,7 +508,7 @@ static int virtnet_poll(struct napi_stru
{
struct virtnet_info *vi = container_of(napi, struct virtnet_info, napi);
void *buf;
- unsigned int len, received = 0;
+ unsigned int r, len, received = 0;
again:
while (received budget
@@ -525,8 +525,9 @@ again:
/* Out of packets? */
if (received budget) {
+ r = virtqueue_enable_cb_prepare(vi-rvq);
napi_complete(napi);
- if (unlikely(!virtqueue_enable_cb(vi-rvq))
+ if (unlikely(virtqueue_poll(vi-rvq, r))
napi_schedule_prep(napi)) {
virtqueue_disable_cb(vi-rvq);
__napi_schedule(napi);
