From: Jason Wang <jasowang@redhat.com>
To: Thibaut Collet <thibaut.collet@6wind.com>
Cc: Vincenzo Maffione <v.maffione@gmail.com>,
Giuseppe Lettieri <g.lettieri@iet.unipi.it>,
Luigi Rizzo <rizzo@iet.unipi.it>,
qemu-devel@nongnu.org, stefanha@redhat.com
Subject: Re: [Qemu-devel] [PATCH 1/1] net: fix queue's purge on VM stop
Date: Wed, 03 Jun 2015 17:40:25 +0800 [thread overview]
Message-ID: <556ECB89.6030103@redhat.com> (raw)
In-Reply-To: <CABUUfwPqBBbbnPoPGSJG___ZzJ55jLfWKamF1hbTWPJxjSE3TQ@mail.gmail.com>
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 <jasowang@redhat.com
> <mailto:jasowang@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.collet@6wind.com
> <mailto:thibaut.collet@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
>
>
prev parent reply other threads:[~2015-06-03 9:40 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-28 8:03 [Qemu-devel] [PATCH 1/1] net: fix queue's purge on VM stop Thibaut Collet
2015-05-29 13:12 ` Stefan Hajnoczi
2015-05-29 14:28 ` Thibaut Collet
2015-06-02 10:34 ` Stefan Hajnoczi
2015-06-03 7:56 ` Thibaut Collet
2015-06-03 9:42 ` Jason Wang
2015-06-03 13:40 ` Stefan Hajnoczi
2015-06-04 9:35 ` Jason Wang
2015-06-05 13:24 ` [Qemu-devel] [PATCH v2] net: Add support of VIRTIO_NET_F_GUEST_ANNOUNCE for vhost-net/vhost-user Thibaut Collet
2015-06-08 5:55 ` Jason Wang
2015-06-08 8:21 ` Thibaut Collet
2015-06-08 9:14 ` Jason Wang
2015-06-08 10:01 ` Thibaut Collet
2015-06-08 10:09 ` Michael S. Tsirkin
2015-06-08 10:22 ` Jason Wang
2015-06-08 13:32 ` Stefan Hajnoczi
2015-06-08 14:05 ` Thibaut Collet
2015-06-08 15:13 ` Stefan Hajnoczi
2015-06-08 15:32 ` Thibaut Collet
2015-06-09 10:38 ` Stefan Hajnoczi
2015-06-09 10:43 ` Stefan Hajnoczi
2015-06-08 16:13 ` Michael S. Tsirkin
2015-06-09 2:35 ` Jason Wang
2015-06-08 10:12 ` Michael S. Tsirkin
2015-06-08 11:29 ` Thibaut Collet
2015-06-08 12:45 ` Michael S. Tsirkin
2015-06-08 13:20 ` Thibaut Collet
2015-06-08 15:48 ` Michael S. Tsirkin
2015-06-08 13:25 ` Stefan Hajnoczi
2015-06-03 9:42 ` [Qemu-devel] [PATCH 1/1] net: fix queue's purge on VM stop Stefan Hajnoczi
2015-06-01 9:17 ` Jason Wang
2015-06-01 10:14 ` Thibaut Collet
2015-06-03 9:40 ` Jason Wang [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=556ECB89.6030103@redhat.com \
--to=jasowang@redhat.com \
--cc=g.lettieri@iet.unipi.it \
--cc=qemu-devel@nongnu.org \
--cc=rizzo@iet.unipi.it \
--cc=stefanha@redhat.com \
--cc=thibaut.collet@6wind.com \
--cc=v.maffione@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.