[PATCH] virtio-net: add schedule check to napi_enable call in refill_work

[PATCH] virtio-net: add schedule check to napi_enable call in refill_work

[Ken Stailey]

Justification:

Impact: Under heavy network I/O load virtio-net driver crashes making VM guest unusable.

Testcases:

1) Sergey Svishchev reports that servers that run java webapps with high java heap usage (especially when heap size is close to physical memory size) helps trigger one of aforementioned bugs. Unfortunately, I don't have a simple test case.

2) Peter Lieven reports that his binary NNTP newsfeed test servers crash without this patch.

3) Bruce Rogers of Novell has asked for this patch to be integrated but the request was mysteriously ignored.  Is is purported that this patch is being distributed with SLES.

4) I can crash 2.6.32 and 2.6.38-rc4 by simply running "scp -r /nfs/read-only/1 otherhost:/target/1" and "scp -r /nfs/read-only/2 otherhost:/target/2" concurrently with a mix of small to medium files for a few hours usually.  I've never seen more than 200 GB copied before the crash occurs.  Both 2.6.32 and 2.6.38-rc4 with this patch will copy more than 200GB unfailingly this way.

See
https://bugs.launchpad.net/bugs/579276
for more details.



--- drivers/net/virtio_net.c.orig	2011-02-08 14:34:51.444099190 -0500
+++ drivers/net/virtio_net.c	2011-02-08 14:18:00.484400134 -0500
@@ -446,6 +446,20 @@
 	}
 }
 
+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)) {
+		virtqueue_disable_cb(vi->rvq);
+		__napi_schedule(&vi->napi);
+	}
+}
+
 static void refill_work(struct work_struct *work)
 {
 	struct virtnet_info *vi;
@@ -454,7 +468,7 @@
 	vi = container_of(work, struct virtnet_info, refill.work);
 	napi_disable(&vi->napi);
 	still_empty = !try_fill_recv(vi, GFP_KERNEL);
-	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. */
@@ -638,16 +652,7 @@
 {
 	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)) {
-		virtqueue_disable_cb(vi->rvq);
-		__napi_schedule(&vi->napi);
-	}
+	virtnet_napi_enable(vi);
 	return 0;
 }

Re: [PATCH] virtio-net: add schedule check to napi_enable call in refill_work

[Rusty Russell]

On Thu, 10 Feb 2011 06:59:25 am Ken Stailey wrote:
> Justification:
> 
> Impact: Under heavy network I/O load virtio-net driver crashes making VM guest unusable.

Hmm, this went badly wrong.  I acked this patch, and it was mailed to
netdev six months ago.

Bruce's patch used spaces instead of tabs, but that should not have caused
it to be dropped.  I've taken that and ported it forwards, will repost now.

Thanks for picking this up off the floor!
Rusty.

Re: [PATCH] virtio-net: add schedule check to napi_enable call in refill_work

[Rusty Russell]

On Thu, 10 Feb 2011 06:59:25 am Ken Stailey wrote:
> Justification:
> 
> Impact: Under heavy network I/O load virtio-net driver crashes making VM guest unusable.

Hmm, this went badly wrong.  I acked this patch, and it was mailed to
netdev six months ago.

Bruce's patch used spaces instead of tabs, but that should not have caused
it to be dropped.  I've taken that and ported it forwards, will repost now.

Thanks for picking this up off the floor!
Rusty.

Re: [PATCH] virtio-net: add schedule check to napi_enable call in refill_work

[Bruce Rogers]

 >>> On 2/9/2011 at 06:31 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: 
> On Thu, 10 Feb 2011 06:59:25 am Ken Stailey wrote:
>> Justification:
>> 
>> Impact: Under heavy network I/O load virtio-net driver crashes making VM 
> guest unusable.
> 
> Hmm, this went badly wrong.  I acked this patch, and it was mailed to
> netdev six months ago.
> 
> Bruce's patch used spaces instead of tabs, but that should not have caused
> it to be dropped.  I've taken that and ported it forwards, will repost now.
> 
> Thanks for picking this up off the floor!
> Rusty.

Thanks for taking care of that!

Bruce

Re: [PATCH] virtio-net: add schedule check to napi_enable call in refill_work

[Bruce Rogers]

 >>> On 2/9/2011 at 06:31 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: 
> On Thu, 10 Feb 2011 06:59:25 am Ken Stailey wrote:
>> Justification:
>> 
>> Impact: Under heavy network I/O load virtio-net driver crashes making VM 
> guest unusable.
> 
> Hmm, this went badly wrong.  I acked this patch, and it was mailed to
> netdev six months ago.
> 
> Bruce's patch used spaces instead of tabs, but that should not have caused
> it to be dropped.  I've taken that and ported it forwards, will repost now.
> 
> Thanks for picking this up off the floor!
> Rusty.

Thanks for taking care of that!

Bruce