Re: [Qemu-devel] [PATCH 1/1] net: fix queue's purge on VM stop

[Jason Wang]

On 06/01/2015 06:14 PM, Thibaut Collet wrote:
> Hi,
>
> >- vhost-user set received_disabled to true to prevent this from
> >happening. but looks like qemu_flush_or_purge_queue_packets() change it
> >to false unconditionally. And I don't quite understand this, probably we
> >can just remove this and the issue is fixed?
>
> I am afraid that solution can cause issues with other netdev backends.
> If for any reason a netdev backend is no more able to treat packets it
> is unvalidated by setting the received_disabled to true. This boolean
> is reset to false only by the qemu_flush_or_purge_queue_packets
> function. This way allows to restart communication with a netdev
> backend that will be temporary done. I do not know if this case can
> occur but I do not want to break this mechanism.

Then we can only set receive_disabled to zero when doing flush. And keep
the value when purge.

>
> >- If not, maybe you just need another flag like receive_disabled. This
> >seems can save a lot of codes.
>
> Idea of my fix is to avoid enqueuing packet. This solution has the
> advantage to provide dynamic configuration of queue size for future
> use but modified several files.
>
> Your suggestion is more vhost user centric, and has the disadvantage
> to enqueue packet that will be never send, nor free during the flush
> operation. Memory will be normally freed by the system when qemu stops
> but it can increase the risk of memory leak.

The issue is this only works when no sent_cb (though it was not an issue
for vhost-user since it does not even use a sent_cb).

My suggestion is to just check this flag and drop the packet if it was
true in qemu_net_queue_append{_iov}(). But I'm ok to your method also.

>
> Regards.
>
> Thibaut.
>
> On Mon, Jun 1, 2015 at 11:17 AM, Jason Wang <address@hidden
> <mailto:address@hidden>> wrote:
>
>
>
>     On 05/28/2015 04:03 PM, Thibaut Collet wrote:
>     > For netdev backend with no receive callback, packets must not be
>     enqueued. When
>     > VM stops the enqueued packets are purged (see fixes
>     ca77d85e1dbf). That causes a
>     > qemu segfault due to a call of a NULL pointer function.
>     >
>     > Function to create net client is modified to allow backend to
>     specify the size
>     > of the queue.
>     > For netdev backend with no receive callback, the queue size is
>     set to 0 to
>     > prevent enqueuing of packets.
>     > For other netdev backend, a default queue size is provided. This
>     value (set to
>     > 10000) is the value previously set to any queue.
>     >
>     > Signed-off-by: Thibaut Collet <address@hidden
>     <mailto:address@hidden>>
>     > ---
>     >  include/net/net.h   |    3 ++-
>     >  include/net/queue.h |    5 ++++-
>     >  net/dump.c          |    3 ++-
>     >  net/hub.c           |    3 ++-
>     >  net/l2tpv3.c        |    3 ++-
>     >  net/net.c           |   10 ++++++----
>     >  net/netmap.c        |    3 ++-
>     >  net/queue.c         |    4 ++--
>     >  net/slirp.c         |    3 ++-
>     >  net/socket.c        |    9 ++++++---
>     >  net/tap-win32.c     |    3 ++-
>     >  net/tap.c           |    3 ++-
>     >  net/vde.c           |    3 ++-
>     >  net/vhost-user.c    |    4 +++-
>     >  14 files changed, 39 insertions(+), 20 deletions(-)
>     >
>     > diff --git a/include/net/net.h b/include/net/net.h
>     > index e66ca03..cb3e2df 100644
>     > --- a/include/net/net.h
>     > +++ b/include/net/net.h
>     > @@ -104,7 +104,8 @@ int qemu_find_net_clients_except(const char
>     *id, NetClientState **ncs,
>     >  NetClientState *qemu_new_net_client(NetClientInfo *info,
>     >                                      NetClientState *peer,
>     >                                      const char *model,
>     > -                                    const char *name);
>     > +                                    const char *name,
>     > +                                    uint32_t queue_size);
>     >  NICState *qemu_new_nic(NetClientInfo *info,
>     >                         NICConf *conf,
>     >                         const char *model,
>     > diff --git a/include/net/queue.h b/include/net/queue.h
>     > index fc02b33..4659c5a 100644
>     > --- a/include/net/queue.h
>     > +++ b/include/net/queue.h
>     > @@ -34,7 +34,10 @@ typedef void (NetPacketSent) (NetClientState
>     *sender, ssize_t ret);
>     >  #define QEMU_NET_PACKET_FLAG_NONE  0
>     >  #define QEMU_NET_PACKET_FLAG_RAW  (1<<0)
>     >
>     > -NetQueue *qemu_new_net_queue(void *opaque);
>     > +#define QEMU_DEFAULT_QUEUE_SIZE  10000
>     > +#define QEMU_EMPTY_QUEUE         0
>     > +
>     > +NetQueue *qemu_new_net_queue(void *opaque, uint32_t queue_size);
>     >
>     >  void qemu_del_net_queue(NetQueue *queue);
>     >
>     > diff --git a/net/dump.c b/net/dump.c
>     > index 9d3a09e..299b09e 100644
>     > --- a/net/dump.c
>     > +++ b/net/dump.c
>     > @@ -129,7 +129,8 @@ static int net_dump_init(NetClientState
>     *peer, const char *device,
>     >          return -1;
>     >      }
>     >
>     > -    nc = qemu_new_net_client(&net_dump_info, peer, device, name);
>     > +    nc = qemu_new_net_client(&net_dump_info, peer, device, name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >
>     >      snprintf(nc->info_str, sizeof(nc->info_str),
>     >               "dump to %s (len=%d)", filename, len);
>     > diff --git a/net/hub.c b/net/hub.c
>     > index 2b60ab9..a42cac3 100644
>     > --- a/net/hub.c
>     > +++ b/net/hub.c
>     > @@ -151,7 +151,8 @@ static NetHubPort *net_hub_port_new(NetHub
>     *hub, const char *name)
>     >          name = default_name;
>     >      }
>     >
>     > -    nc = qemu_new_net_client(&net_hub_port_info, NULL, "hub",
>     name);
>     > +    nc = qemu_new_net_client(&net_hub_port_info, NULL, "hub", name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >      port = DO_UPCAST(NetHubPort, nc, nc);
>     >      port->id = id;
>     >      port->hub = hub;
>     > diff --git a/net/l2tpv3.c b/net/l2tpv3.c
>     > index 8c598b0..1d2e4e5 100644
>     > --- a/net/l2tpv3.c
>     > +++ b/net/l2tpv3.c
>     > @@ -548,7 +548,8 @@ int net_init_l2tpv3(const NetClientOptions
>     *opts,
>     >      struct addrinfo *result = NULL;
>     >      char *srcport, *dstport;
>     >
>     > -    nc = qemu_new_net_client(&net_l2tpv3_info, peer, "l2tpv3",
>     name);
>     > +    nc = qemu_new_net_client(&net_l2tpv3_info, peer, "l2tpv3",
>     name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >
>     >      s = DO_UPCAST(NetL2TPV3State, nc, nc);
>     >
>     > diff --git a/net/net.c b/net/net.c
>     > index 7427f6a..bcf69da 100644
>     > --- a/net/net.c
>     > +++ b/net/net.c
>     > @@ -214,6 +214,7 @@ static void
>     qemu_net_client_setup(NetClientState *nc,
>     >                                    NetClientState *peer,
>     >                                    const char *model,
>     >                                    const char *name,
>     > +                                  uint32_t queue_size,
>     >                                    NetClientDestructor *destructor)
>     >  {
>     >      nc->info = info;
>     > @@ -231,21 +232,22 @@ static void
>     qemu_net_client_setup(NetClientState *nc,
>     >      }
>     >      QTAILQ_INSERT_TAIL(&net_clients, nc, next);
>     >
>     > -    nc->incoming_queue = qemu_new_net_queue(nc);
>     > +    nc->incoming_queue = qemu_new_net_queue(nc, queue_size);
>     >      nc->destructor = destructor;
>     >  }
>     >
>     >  NetClientState *qemu_new_net_client(NetClientInfo *info,
>     >                                      NetClientState *peer,
>     >                                      const char *model,
>     > -                                    const char *name)
>     > +                                    const char *name,
>     > +                                    uint32_t queue_size)
>     >  {
>     >      NetClientState *nc;
>     >
>     >      assert(info->size >= sizeof(NetClientState));
>     >
>     >      nc = g_malloc0(info->size);
>     > -    qemu_net_client_setup(nc, info, peer, model, name,
>     > +    qemu_net_client_setup(nc, info, peer, model, name, queue_size,
>     >                            qemu_net_client_destructor);
>     >
>     >      return nc;
>     > @@ -271,7 +273,7 @@ NICState *qemu_new_nic(NetClientInfo *info,
>     >
>     >      for (i = 0; i < queues; i++) {
>     >          qemu_net_client_setup(&nic->ncs[i], info, peers[i],
>     model, name,
>     > -                              NULL);
>     > +                              QEMU_DEFAULT_QUEUE_SIZE, NULL);
>     >          nic->ncs[i].queue_index = i;
>     >      }
>     >
>     > diff --git a/net/netmap.c b/net/netmap.c
>     > index 0c1772b..3ddfc8e 100644
>     > --- a/net/netmap.c
>     > +++ b/net/netmap.c
>     > @@ -461,7 +461,8 @@ int net_init_netmap(const NetClientOptions
>     *opts,
>     >          return -1;
>     >      }
>     >      /* Create the object. */
>     > -    nc = qemu_new_net_client(&net_netmap_info, peer, "netmap",
>     name);
>     > +    nc = qemu_new_net_client(&net_netmap_info, peer, "netmap",
>     name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >      s = DO_UPCAST(NetmapState, nc, nc);
>     >      s->me = me;
>     >      s->vnet_hdr_len = 0;
>     > diff --git a/net/queue.c b/net/queue.c
>     > index ebbe2bb..d6ddda5 100644
>     > --- a/net/queue.c
>     > +++ b/net/queue.c
>     > @@ -58,14 +58,14 @@ struct NetQueue {
>     >      unsigned delivering : 1;
>     >  };
>     >
>     > -NetQueue *qemu_new_net_queue(void *opaque)
>     > +NetQueue *qemu_new_net_queue(void *opaque, uint32_t queue_size)
>     >  {
>     >      NetQueue *queue;
>     >
>     >      queue = g_new0(NetQueue, 1);
>     >
>     >      queue->opaque = opaque;
>     > -    queue->nq_maxlen = 10000;
>     > +    queue->nq_maxlen = queue_size;
>     >      queue->nq_count = 0;
>     >
>     >      QTAILQ_INIT(&queue->packets);
>     > diff --git a/net/slirp.c b/net/slirp.c
>     > index 9bbed74..c91e929 100644
>     > --- a/net/slirp.c
>     > +++ b/net/slirp.c
>     > @@ -234,7 +234,8 @@ static int net_slirp_init(NetClientState
>     *peer, const char *model,
>     >      }
>     >  #endif
>     >
>     > -    nc = qemu_new_net_client(&net_slirp_info, peer, model, name);
>     > +    nc = qemu_new_net_client(&net_slirp_info, peer, model, name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >
>     >      snprintf(nc->info_str, sizeof(nc->info_str),
>     >               "net=%s,restrict=%s", inet_ntoa(net),
>     > diff --git a/net/socket.c b/net/socket.c
>     > index c30e03f..d5c718c 100644
>     > --- a/net/socket.c
>     > +++ b/net/socket.c
>     > @@ -387,7 +387,8 @@ static NetSocketState
>     *net_socket_fd_init_dgram(NetClientState *peer,
>     >          }
>     >      }
>     >
>     > -    nc = qemu_new_net_client(&net_dgram_socket_info, peer,
>     model, name);
>     > +    nc = qemu_new_net_client(&net_dgram_socket_info, peer,
>     model, name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >
>     >      s = DO_UPCAST(NetSocketState, nc, nc);
>     >
>     > @@ -436,7 +437,8 @@ static NetSocketState
>     *net_socket_fd_init_stream(NetClientState *peer,
>     >      NetClientState *nc;
>     >      NetSocketState *s;
>     >
>     > -    nc = qemu_new_net_client(&net_socket_info, peer, model, name);
>     > +    nc = qemu_new_net_client(&net_socket_info, peer, model, name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >
>     >      snprintf(nc->info_str, sizeof(nc->info_str), "socket:
>     fd=%d", fd);
>     >
>     > @@ -543,7 +545,8 @@ static int
>     net_socket_listen_init(NetClientState *peer,
>     >          return -1;
>     >      }
>     >
>     > -    nc = qemu_new_net_client(&net_socket_info, peer, model, name);
>     > +    nc = qemu_new_net_client(&net_socket_info, peer, model, name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >      s = DO_UPCAST(NetSocketState, nc, nc);
>     >      s->fd = -1;
>     >      s->listen_fd = fd;
>     > diff --git a/net/tap-win32.c b/net/tap-win32.c
>     > index 8aee611..dd6046a 100644
>     > --- a/net/tap-win32.c
>     > +++ b/net/tap-win32.c
>     > @@ -737,7 +737,8 @@ static int tap_win32_init(NetClientState
>     *peer, const char *model,
>     >          return -1;
>     >      }
>     >
>     > -    nc = qemu_new_net_client(&net_tap_win32_info, peer, model,
>     name);
>     > +    nc = qemu_new_net_client(&net_tap_win32_info, peer, model,
>     name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >
>     >      s = DO_UPCAST(TAPState, nc, nc);
>     >
>     > diff --git a/net/tap.c b/net/tap.c
>     > index 968df46..383d3ad 100644
>     > --- a/net/tap.c
>     > +++ b/net/tap.c
>     > @@ -346,7 +346,8 @@ static TAPState
>     *net_tap_fd_init(NetClientState *peer,
>     >      NetClientState *nc;
>     >      TAPState *s;
>     >
>     > -    nc = qemu_new_net_client(&net_tap_info, peer, model, name);
>     > +    nc = qemu_new_net_client(&net_tap_info, peer, model, name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >
>     >      s = DO_UPCAST(TAPState, nc, nc);
>     >
>     > diff --git a/net/vde.c b/net/vde.c
>     > index 2a619fb..b6d28cf 100644
>     > --- a/net/vde.c
>     > +++ b/net/vde.c
>     > @@ -95,7 +95,8 @@ static int net_vde_init(NetClientState *peer,
>     const char *model,
>     >          return -1;
>     >      }
>     >
>     > -    nc = qemu_new_net_client(&net_vde_info, peer, model, name);
>     > +    nc = qemu_new_net_client(&net_vde_info, peer, model, name,
>     > +                             QEMU_DEFAULT_QUEUE_SIZE);
>     >
>     >      snprintf(nc->info_str, sizeof(nc->info_str), "sock=%s,fd=%d",
>     >               sock, vde_datafd(vde));
>     > diff --git a/net/vhost-user.c b/net/vhost-user.c
>     > index 1d86a2b..71f8758 100644
>     > --- a/net/vhost-user.c
>     > +++ b/net/vhost-user.c
>     > @@ -137,7 +137,9 @@ static int
>     net_vhost_user_init(NetClientState *peer, const char *device,
>     >      NetClientState *nc;
>     >      VhostUserState *s;
>     >
>     > -    nc = qemu_new_net_client(&net_vhost_user_info, peer,
>     device, name);
>     > +    /* We don't provide a valid queue: no receive callback */
>     > +    nc = qemu_new_net_client(&net_vhost_user_info, peer,
>     device, name,
>     > +                             QEMU_EMPTY_QUEUE);
>     >
>     >      snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to
>     %s",
>     >               chr->label);
>
>     Two questions:
>
>     - vhost-user set received_disabled to true to prevent this from
>     happening. but looks like qemu_flush_or_purge_queue_packets()
>     change it
>     to false unconditionally. And I don't quite understand this,
>     probably we
>     can just remove this and the issue is fixed?
>     - If not, maybe you just need another flag like receive_disabled. This
>     seems can save a lot of codes.
>
>     Thanks
>
>