[Qemu-devel] [PATCH 1/3] net: invoke callback when purging queue

[Qemu-devel] [PATCH 1/3] net: invoke callback when purging queue

[Michael S. Tsirkin]

devices rely on packet callbacks eventually running,
but we violate this rule whenever we purge the queue.
To fix, invoke callbacks on all packets on purge.
Set length to 0, this way callers can detect that
this happened and re-queue if necessary.

Cc: qemu-stable@nongnu.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 net/queue.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/queue.c b/net/queue.c
index 859d02a..f948318 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -233,6 +233,9 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from)
         if (packet->sender == from) {
             QTAILQ_REMOVE(&queue->packets, packet, entry);
             queue->nq_count--;
+            if (packet->sent_cb) {
+                packet->sent_cb(packet->sender, 0);
+            }
             g_free(packet);
         }
     }
-- 
MST

[Qemu-devel] [PATCH 2/3] net: complete all queued packets on VM stop

[Michael S. Tsirkin]

This completes all packets, ensuring that callbacks
will not run when VM is stopped.

Cc: qemu-stable@nongnu.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 net/net.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index 6d930ea..25fdb07 100644
--- a/net/net.c
+++ b/net/net.c
@@ -47,6 +47,7 @@
 # define CONFIG_NET_BRIDGE
 #endif
 
+static VMChangeStateEntry *net_change_state_entry;
 static QTAILQ_HEAD(, NetClientState) net_clients;
 
 const char *host_net_devices[] = {
@@ -504,7 +505,8 @@ void qemu_purge_queued_packets(NetClientState *nc)
     qemu_net_queue_purge(nc->peer->incoming_queue, nc);
 }
 
-void qemu_flush_queued_packets(NetClientState *nc)
+static
+void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge)
 {
     nc->receive_disabled = 0;
 
@@ -518,9 +520,17 @@ void qemu_flush_queued_packets(NetClientState *nc)
          * the file descriptor (for tap, for example).
          */
         qemu_notify_event();
+    } else if (purge) {
+        /* Unable to empty the queue, purge remaining packets */
+        qemu_net_queue_purge(nc->incoming_queue, nc);
     }
 }
 
+void qemu_flush_queued_packets(NetClientState *nc)
+{
+    qemu_flush_or_purge_queued_packets(nc, false);
+}
+
 static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
                                                  unsigned flags,
                                                  const uint8_t *buf, int size,
@@ -1168,6 +1178,22 @@ void qmp_set_link(const char *name, bool up, Error **errp)
     }
 }
 
+static void net_vm_change_state_handler(void *opaque, int running,
+                                        RunState state)
+{
+    /* Complete all queued packets, to guarantee we don't modify
+     * state later when VM is not running.
+     */
+    if (!running) {
+        NetClientState *nc;
+        NetClientState *tmp;
+
+        QTAILQ_FOREACH_SAFE(nc, &net_clients, next, tmp) {
+            qemu_flush_or_purge_queued_packets(nc, true);
+        }
+    }
+}
+
 void net_cleanup(void)
 {
     NetClientState *nc;
@@ -1183,6 +1209,8 @@ void net_cleanup(void)
             qemu_del_net_client(nc);
         }
     }
+
+    qemu_del_vm_change_state_handler(net_change_state_entry);
 }
 
 void net_check_clients(void)
@@ -1268,6 +1296,9 @@ int net_init_clients(void)
 #endif
     }
 
+    net_change_state_entry =
+        qemu_add_vm_change_state_handler(net_vm_change_state_handler, NULL);
+
     QTAILQ_INIT(&net_clients);
 
     if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1)
-- 
MST

Re: [Qemu-devel] [PATCH 2/3] net: complete all queued packets on VM stop

[Jason Wang]

On 09/04/2014 04:39 PM, Michael S. Tsirkin wrote:
> This completes all packets, ensuring that callbacks
> will not run when VM is stopped.
>
> Cc: qemu-stable@nongnu.org
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  net/net.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/net/net.c b/net/net.c
> index 6d930ea..25fdb07 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -47,6 +47,7 @@
>  # define CONFIG_NET_BRIDGE
>  #endif
>  
> +static VMChangeStateEntry *net_change_state_entry;
>  static QTAILQ_HEAD(, NetClientState) net_clients;
>  
>  const char *host_net_devices[] = {
> @@ -504,7 +505,8 @@ void qemu_purge_queued_packets(NetClientState *nc)
>      qemu_net_queue_purge(nc->peer->incoming_queue, nc);
>  }
>  
> -void qemu_flush_queued_packets(NetClientState *nc)
> +static
> +void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge)
>  {
>      nc->receive_disabled = 0;
>  
> @@ -518,9 +520,17 @@ void qemu_flush_queued_packets(NetClientState *nc)
>           * the file descriptor (for tap, for example).
>           */
>          qemu_notify_event();
> +    } else if (purge) {
> +        /* Unable to empty the queue, purge remaining packets */
> +        qemu_net_queue_purge(nc->incoming_queue, nc);
>      }
>  }
>  
> +void qemu_flush_queued_packets(NetClientState *nc)
> +{
> +    qemu_flush_or_purge_queued_packets(nc, false);
> +}
> +
>  static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
>                                                   unsigned flags,
>                                                   const uint8_t *buf, int size,
> @@ -1168,6 +1178,22 @@ void qmp_set_link(const char *name, bool up, Error **errp)
>      }
>  }
>  
> +static void net_vm_change_state_handler(void *opaque, int running,
> +                                        RunState state)
> +{
> +    /* Complete all queued packets, to guarantee we don't modify
> +     * state later when VM is not running.
> +     */
> +    if (!running) {
> +        NetClientState *nc;
> +        NetClientState *tmp;
> +
> +        QTAILQ_FOREACH_SAFE(nc, &net_clients, next, tmp) {
> +            qemu_flush_or_purge_queued_packets(nc, true);
> +        }
> +    }
> +}
> +
>  void net_cleanup(void)
>  {
>      NetClientState *nc;
> @@ -1183,6 +1209,8 @@ void net_cleanup(void)
>              qemu_del_net_client(nc);
>          }
>      }
> +
> +    qemu_del_vm_change_state_handler(net_change_state_entry);
>  }
>  
>  void net_check_clients(void)
> @@ -1268,6 +1296,9 @@ int net_init_clients(void)
>  #endif
>      }
>  
> +    net_change_state_entry =
> +        qemu_add_vm_change_state_handler(net_vm_change_state_handler, NULL);
> +
>      QTAILQ_INIT(&net_clients);
>  
>      if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1)

A problem is the dependency between state change handlers (e.g. virtio).
Current virtio vmstate change handler will be called before this
handler. Which means vdev->vm_running was false when we purge the queue,
this will trigger the assert of vdev->vm_running in virtio_net_flush_tx().

Re: [Qemu-devel] [PATCH 2/3] net: complete all queued packets on VM stop

[Michael S. Tsirkin]

On Thu, Sep 04, 2014 at 06:15:12PM +0800, Jason Wang wrote:
> On 09/04/2014 04:39 PM, Michael S. Tsirkin wrote:
> > This completes all packets, ensuring that callbacks
> > will not run when VM is stopped.
> >
> > Cc: qemu-stable@nongnu.org
> > Cc: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  net/net.c | 33 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/net.c b/net/net.c
> > index 6d930ea..25fdb07 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -47,6 +47,7 @@
> >  # define CONFIG_NET_BRIDGE
> >  #endif
> >  
> > +static VMChangeStateEntry *net_change_state_entry;
> >  static QTAILQ_HEAD(, NetClientState) net_clients;
> >  
> >  const char *host_net_devices[] = {
> > @@ -504,7 +505,8 @@ void qemu_purge_queued_packets(NetClientState *nc)
> >      qemu_net_queue_purge(nc->peer->incoming_queue, nc);
> >  }
> >  
> > -void qemu_flush_queued_packets(NetClientState *nc)
> > +static
> > +void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge)
> >  {
> >      nc->receive_disabled = 0;
> >  
> > @@ -518,9 +520,17 @@ void qemu_flush_queued_packets(NetClientState *nc)
> >           * the file descriptor (for tap, for example).
> >           */
> >          qemu_notify_event();
> > +    } else if (purge) {
> > +        /* Unable to empty the queue, purge remaining packets */
> > +        qemu_net_queue_purge(nc->incoming_queue, nc);
> >      }
> >  }
> >  
> > +void qemu_flush_queued_packets(NetClientState *nc)
> > +{
> > +    qemu_flush_or_purge_queued_packets(nc, false);
> > +}
> > +
> >  static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
> >                                                   unsigned flags,
> >                                                   const uint8_t *buf, int size,
> > @@ -1168,6 +1178,22 @@ void qmp_set_link(const char *name, bool up, Error **errp)
> >      }
> >  }
> >  
> > +static void net_vm_change_state_handler(void *opaque, int running,
> > +                                        RunState state)
> > +{
> > +    /* Complete all queued packets, to guarantee we don't modify
> > +     * state later when VM is not running.
> > +     */
> > +    if (!running) {
> > +        NetClientState *nc;
> > +        NetClientState *tmp;
> > +
> > +        QTAILQ_FOREACH_SAFE(nc, &net_clients, next, tmp) {
> > +            qemu_flush_or_purge_queued_packets(nc, true);
> > +        }
> > +    }
> > +}
> > +
> >  void net_cleanup(void)
> >  {
> >      NetClientState *nc;
> > @@ -1183,6 +1209,8 @@ void net_cleanup(void)
> >              qemu_del_net_client(nc);
> >          }
> >      }
> > +
> > +    qemu_del_vm_change_state_handler(net_change_state_entry);
> >  }
> >  
> >  void net_check_clients(void)
> > @@ -1268,6 +1296,9 @@ int net_init_clients(void)
> >  #endif
> >      }
> >  
> > +    net_change_state_entry =
> > +        qemu_add_vm_change_state_handler(net_vm_change_state_handler, NULL);
> > +
> >      QTAILQ_INIT(&net_clients);
> >  
> >      if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1)
> 
> A problem is the dependency between state change handlers (e.g. virtio).
> Current virtio vmstate change handler will be called before this
> handler. Which means vdev->vm_running was false when we purge the queue,
> this will trigger the assert of vdev->vm_running in virtio_net_flush_tx().

True but that's a virtio bug: it changes vm_running too early.
I will send a patch to fix that now.
Long term with the core changes, we mught be able to get rid of
vm_running field completely.

[Qemu-devel] [PATCH 3/3] virtio-net: purge outstanding packets when starting vhost

[Michael S. Tsirkin]

whenever we start vhost, virtio could have outstanding packets
queued, when they complete later we'll modify the ring
while vhost is processing it.

To prevent this, purge outstanding packets on vhost start.

Cc: qemu-stable@nongnu.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/virtio-net.c | 15 ++++++++++++++-
 net/net.c           |  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 365e266..826a2a5 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -125,10 +125,23 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
         return;
     }
     if (!n->vhost_started) {
-        int r;
+        int r, i;
+
         if (!vhost_net_query(get_vhost_net(nc->peer), vdev)) {
             return;
         }
+
+        /* Any packets outstanding? Purge them to avoid touching rings
+         * when vhost is running.
+         */
+        for (i = 0;  i < queues; i++) {
+            NetClientState *qnc = qemu_get_subqueue(n->nic, i);
+
+            /* Purge both directions: TX and RX. */
+            qemu_net_queue_purge(qnc->peer->incoming_queue, qnc);
+            qemu_net_queue_purge(qnc->incoming_queue, qnc->peer);
+        }
+
         n->vhost_started = 1;
         r = vhost_net_start(vdev, n->nic->ncs, queues);
         if (r < 0) {
diff --git a/net/net.c b/net/net.c
index 25fdb07..b94473e 100644
--- a/net/net.c
+++ b/net/net.c
@@ -41,6 +41,7 @@
 #include "qapi-visit.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/dealloc-visitor.h"
+#include "sysemu/sysemu.h"
 
 /* Net bridge is currently not supported for W32. */
 #if !defined(_WIN32)
-- 
MST

Re: [Qemu-devel] [PATCH 1/3] net: invoke callback when purging queue

[Jason Wang]

On 09/04/2014 04:39 PM, Michael S. Tsirkin wrote:
> devices rely on packet callbacks eventually running,
> but we violate this rule whenever we purge the queue.
> To fix, invoke callbacks on all packets on purge.
> Set length to 0, this way callers can detect that
> this happened and re-queue if necessary.
>
> Cc: qemu-stable@nongnu.org
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  net/queue.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/queue.c b/net/queue.c
> index 859d02a..f948318 100644
> --- a/net/queue.c
> +++ b/net/queue.c
> @@ -233,6 +233,9 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from)
>          if (packet->sender == from) {
>              QTAILQ_REMOVE(&queue->packets, packet, entry);
>              queue->nq_count--;
> +            if (packet->sent_cb) {
> +                packet->sent_cb(packet->sender, 0);
> +            }
>              g_free(packet);
>          }
>      }

Acked-by: Jason Wang <jasowang@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] net: invoke callback when purging queue

[Stefan Hajnoczi]

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

On Thu, Sep 04, 2014 at 11:39:10AM +0300, Michael S. Tsirkin wrote:
> devices rely on packet callbacks eventually running,
> but we violate this rule whenever we purge the queue.
> To fix, invoke callbacks on all packets on purge.
> Set length to 0, this way callers can detect that
> this happened and re-queue if necessary.
> 
> Cc: qemu-stable@nongnu.org
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  net/queue.c | 3 +++
>  1 file changed, 3 insertions(+)

Merged "virtio: don't call device on !vm_running" before this to prevent
the assertion failures that Jason mentioned.

Thanks, applied to my net tree:
https://github.com/stefanha/qemu/commits/net

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]