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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).