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 <jasow...@redhat.com
> <mailto:jasow...@redhat.com>> 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 <thibaut.col...@6wind.com
> <mailto:thibaut.col...@6wind.com>>
> > ---
> > 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
>
>
