[Qemu-devel] e1000, virtio_net: Check link status in can_receive

[Qemu-devel] e1000, virtio_net: Check link status in can_receive

[Yan Vugenfirer]

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]

Hello,

Fixing the bug of 100% cpu usage by qemu after using "set_link <NIC> down"
monitor command. The fix is for virtio_net and for e1000 emulations.


Best regards,
Yan Vugenfirer.


[-- Attachment #2: 0000-cover-letter.patch --]
[-- Type: application/octet-stream, Size: 675 bytes --]

>From d5d7ca2cb68e76cd89a17768f6e854b38e7f1efa Mon Sep 17 00:00:00 2001
From: Yan Vugenfirer <yvugenfi@redhat.com>
Date: Sun, 5 Apr 2009 10:21:29 -0400
Subject: [PATCH 0/1] e1000, virtio_net: Check link status in 
can_receive

Fixing the bug of 100% cpu usage by qemu after using "set_link <NIC> 
down"
monitor command. The fix is for virtio_net and for e1000 emulations. 

Yan Vugenfirer (1):
  Fixing the bug of 100% cpu usage by qemu after using "set_link <NIC>
    down" monitor command. The fix is for virtio_net and for e1000
    emulations.

 hw/e1000.c      |    3 ++-
 hw/virtio-net.c |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)


[-- Attachment #3: 0001-Fixing-the-bug-of-100-cpu-usage-by-qemu-after-using.patch --]
[-- Type: application/octet-stream, Size: 1436 bytes --]

>From d5d7ca2cb68e76cd89a17768f6e854b38e7f1efa Mon Sep 17 00:00:00 2001
From: Yan Vugenfirer <yvugenfi@redhat.com>
Date: Sun, 5 Apr 2009 10:20:53 -0400
Subject: [PATCH 1/1] e1000, virtio_net: Check link status in can_receive

Fixing the bug of 100% cpu usage by qemu after using "set_link <NIC> down" 
monitor command. The fix is for virtio_net and for e1000 emulations.

---
 hw/e1000.c      |    3 ++-
 hw/virtio-net.c |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 1644201..36878d9 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -590,7 +590,8 @@ e1000_can_receive(void *opaque)
 {
     E1000State *s = opaque;
 
-    return (s->mac_reg[RCTL] & E1000_RCTL_EN);
+    return (s->mac_reg[RCTL] & E1000_RCTL_EN) &&
+           (s->mac_reg[STATUS] & E1000_STATUS_LU);
 }
 
 static void
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index ad55bb7..dcd18c1 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -259,7 +259,8 @@ static void virtio_net_handle_rx(VirtIODevice *vdev, VirtQueue *vq)
 static int do_virtio_net_can_receive(VirtIONet *n, int bufsize)
 {
     if (!virtio_queue_ready(n->rx_vq) ||
-        !(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
+        !(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
+        !(n->status & VIRTIO_NET_S_LINK_UP))
         return 0;
 
     if (virtio_queue_empty(n->rx_vq) ||
-- 
1.5.5.6

Re: [Qemu-devel] e1000, virtio_net: Check link status in can_receive

[Mark McLoughlin]

On Sun, 2009-04-12 at 05:17 -0400, Yan Vugenfirer wrote:

> From: Yan Vugenfirer <yvugenfi@redhat.com>
> Date: Sun, 5 Apr 2009 10:20:53 -0400
> Subject: [PATCH 1/1] e1000, virtio_net: Check link status in can_receive
> 
> Fixing the bug of 100% cpu usage by qemu after using "set_link <NIC> down" 
> monitor command. The fix is for virtio_net and for e1000 emulations.
> 
> ---
>  hw/e1000.c      |    3 ++-
>  hw/virtio-net.c |    3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 1644201..36878d9 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -590,7 +590,8 @@ e1000_can_receive(void *opaque)
>  {
>      E1000State *s = opaque;
>  
> -    return (s->mac_reg[RCTL] & E1000_RCTL_EN);
> +    return (s->mac_reg[RCTL] & E1000_RCTL_EN) &&
> +           (s->mac_reg[STATUS] & E1000_STATUS_LU);
>  }
>  
>  static void
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index ad55bb7..dcd18c1 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -259,7 +259,8 @@ static void virtio_net_handle_rx(VirtIODevice *vdev, VirtQueue *vq)
>  static int do_virtio_net_can_receive(VirtIONet *n, int bufsize)
>  {
>      if (!virtio_queue_ready(n->rx_vq) ||
> -        !(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
> +        !(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
> +        !(n->status & VIRTIO_NET_S_LINK_UP))
>          return 0;
>  
>      if (virtio_queue_empty(n->rx_vq) ||

Firstly, I think this "100% CPU" bug is specific to kvm - we have some
code in kvm's tree to buffer tap packets which causes this, I think. I
have some qemu patches in the works to remove this difference between
the trees.

Your patch does make some sense, but I think what we really want is for
qemu_send_packet() to drop the tap packet when virtio/e1000 is down.
This is the behaviour we have in the other direction. Does the patch
below fix the problem for you?

Thanks,
Mark.

diff --git a/qemu/net.c b/qemu/net.c
index 4d07905..95fd808 100644
--- a/qemu/net.c
+++ b/qemu/net.c
@@ -409,8 +409,10 @@ int qemu_send_packet(VLANClientState *vc1, const uint8_t *buf, int size)
     hex_dump(stdout, buf, size);
 #endif
     for(vc = vlan->first_client; vc != NULL; vc = vc->next) {
-        if (vc != vc1 && !vc->link_down) {
-            if (!vc->fd_can_read || vc->fd_can_read(vc->opaque)) {
+        if (vc != vc1) {
+            if (vc->link_down)
+                ret = 0;
+            else if (!vc->fd_can_read || vc->fd_can_read(vc->opaque)) {
                 vc->fd_read(vc->opaque, buf, size);
                 ret = 0;
             }

RE: [Qemu-devel] e1000, virtio_net: Check link status in can_receive

[Yan Vugenfirer]

Hi Mark,

The patch for qemu_send_packet() solves the problem too. 
It looks like a better place for a fix.

Thanks,
Yan.

-----Original Message-----
From: qemu-devel-bounces+yvugenfi=redhat.com@nongnu.org
[mailto:qemu-devel-bounces+yvugenfi=redhat.com@nongnu.org] On Behalf Of
Mark McLoughlin
Sent: Tuesday, April 14, 2009 1:18 AM
To: qemu-devel@nongnu.org
Cc: dlaor@redhat.com
Subject: Re: [Qemu-devel] e1000, virtio_net: Check link status in
can_receive

On Sun, 2009-04-12 at 05:17 -0400, Yan Vugenfirer wrote:

> From: Yan Vugenfirer <yvugenfi@redhat.com>
> Date: Sun, 5 Apr 2009 10:20:53 -0400
> Subject: [PATCH 1/1] e1000, virtio_net: Check link status in can_receive
> 
> Fixing the bug of 100% cpu usage by qemu after using "set_link <NIC>
down" 
> monitor command. The fix is for virtio_net and for e1000 emulations.
> 
> ---
>  hw/e1000.c      |    3 ++-
>  hw/virtio-net.c |    3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 1644201..36878d9 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -590,7 +590,8 @@ e1000_can_receive(void *opaque)
>  {
>      E1000State *s = opaque;
>  
> -    return (s->mac_reg[RCTL] & E1000_RCTL_EN);
> +    return (s->mac_reg[RCTL] & E1000_RCTL_EN) &&
> +           (s->mac_reg[STATUS] & E1000_STATUS_LU);
>  }
>  
>  static void
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index ad55bb7..dcd18c1 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -259,7 +259,8 @@ static void virtio_net_handle_rx(VirtIODevice *vdev,
VirtQueue *vq)
>  static int do_virtio_net_can_receive(VirtIONet *n, int bufsize)
>  {
>      if (!virtio_queue_ready(n->rx_vq) ||
> -        !(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
> +        !(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
> +        !(n->status & VIRTIO_NET_S_LINK_UP))
>          return 0;
>  
>      if (virtio_queue_empty(n->rx_vq) ||

Firstly, I think this "100% CPU" bug is specific to kvm - we have some
code in kvm's tree to buffer tap packets which causes this, I think. I
have some qemu patches in the works to remove this difference between
the trees.

Your patch does make some sense, but I think what we really want is for
qemu_send_packet() to drop the tap packet when virtio/e1000 is down.
This is the behaviour we have in the other direction. Does the patch
below fix the problem for you?

Thanks,
Mark.

diff --git a/qemu/net.c b/qemu/net.c
index 4d07905..95fd808 100644
--- a/qemu/net.c
+++ b/qemu/net.c
@@ -409,8 +409,10 @@ int qemu_send_packet(VLANClientState *vc1, const
uint8_t *buf, int size)
     hex_dump(stdout, buf, size);
 #endif
     for(vc = vlan->first_client; vc != NULL; vc = vc->next) {
-        if (vc != vc1 && !vc->link_down) {
-            if (!vc->fd_can_read || vc->fd_can_read(vc->opaque)) {
+        if (vc != vc1) {
+            if (vc->link_down)
+                ret = 0;
+            else if (!vc->fd_can_read || vc->fd_can_read(vc->opaque)) {
                 vc->fd_read(vc->opaque, buf, size);
                 ret = 0;
             }

Re: [Qemu-devel] e1000, virtio_net: Check link status in can_receive

[Anthony Liguori]

Yan Vugenfirer wrote:
> Hello,
>
> Fixing the bug of 100% cpu usage by qemu after using "set_link <NIC> down"
> monitor command. The fix is for virtio_net and for e1000 emulations.
>
>
> Best regards,
> Yan Vugenfirer.
>
>   
Can you please send this as a plain/text attachment, with [PATCH] in the 
subject, etc.

-- 
Regards,

Anthony Liguori