From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Cc: dev <dev@dpdk.org>, Daniele Di Proietto <diproiettod@vmware.com>
Subject: Re: [dpdk-dev] Memory leak when adding/removing vhost_user ports
Date: Tue, 19 Apr 2016 22:04:45 -0700 [thread overview]
Message-ID: <20160420050445.GB5872@yliu-dev.sh.intel.com> (raw)
In-Reply-To: <CAATJJ0+i0+Qyv4sUwqPJAgPF3QdAS-RP3jZr+_OvGZLjoMEPPA@mail.gmail.com>
On Tue, Apr 19, 2016 at 06:33:50PM +0200, Christian Ehrhardt wrote:
> Hi,
> thanks for the patch.
> I backported it this way to my DPDK 2.2 based environment for now (see below):
>
> With that applied one (and only one) of my two guests looses connectivity after
> removing the ports the first time.
Yeah, that's should be because I invoked the "->destroy_device()"
callback.
BTW, I'm curious how do you do the test? I saw you added 256 ports, but
with 2 guests only? So, 254 of them are idle, just for testing the
memory leak bug?
And then you remove all of them, without stopping the guest. How that's
gonna work? I mean, the vhost-user connection would be broken, and data
flow would not work.
--yliu
> No traffic seems to pass, setting the device in the guest down/up doesn't get
> anything.
> But It isn't totally broken - stopping/starting the guest gets it working
> again.
> So openvswitch/dpdk is still somewhat working - it just seems the guest lost
> something, after tapping on that vhost_user interface again it works.
>
> I will check tomorrow and let you know:
> - if I'm more lucky with that patch on top of 16.04
> - if it looses connectivity after the first or a certain amount of port removes
>
> If you find issues with my backport adaption let me know.
>
>
> ---
>
> Backport and reasoning:
>
> new fix relies on a lot of new code, vhost_destroy_device looks totally
> different from the former destroy_device.
> History on todays function content:
> 4796ad63 - original code moved from examples to lib
> a90ca1a1 - this replaces ops->destroy_device with vhost_destroy_device
> 71dc571e - simple check against null pointers
> 45ca9c6f - this changed the code from linked list to arrays
>
> New code cleans with:
> notify_ops->destroy_device (callback into the parent)
> cleanup_device was existing before even in 2.2 code
> free_device as well existing before even in 2.2 code
> Old code cleans with:
> notify_ops->destroy_device - still there
> rm_config_ll_entry -> eventually calls cleanup_device and free_device
> (just in the more complex linked list way)
>
> So the "only" adaption for backporting needed is to replace
> vhost_destroy_device
> with ops->destroy_device(ctx)
>
> Index: dpdk/lib/librte_vhost/vhost_user/vhost-net-user.c
> ===================================================================
> --- dpdk.orig/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ dpdk/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -310,6 +310,7 @@ vserver_new_vq_conn(int fd, void *dat, _
> }
>
> vdev_ctx.fh = fh;
> + vserver->fh = fh;
> size = strnlen(vserver->path, PATH_MAX);
> ops->set_ifname(vdev_ctx, vserver->path,
> size);
> @@ -516,6 +517,11 @@ rte_vhost_driver_unregister(const char *
>
> for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
> if (!strcmp(g_vhost_server.server[i]->path, path)) {
> + struct vhost_device_ctx ctx;
> +
> + ctx.fh = g_vhost_server.server[i]->fh;
> + ops->destroy_device(ctx);
> +
> fdset_del(&g_vhost_server.fdset,
> g_vhost_server.server[i]->listenfd);
>
> Index: dpdk/lib/librte_vhost/vhost_user/vhost-net-user.h
> ===================================================================
> --- dpdk.orig/lib/librte_vhost/vhost_user/vhost-net-user.h
> +++ dpdk/lib/librte_vhost/vhost_user/vhost-net-user.h
> @@ -43,6 +43,7 @@
> struct vhost_server {
> char *path; /**< The path the uds is bind to. */
> int listenfd; /**< The listener sockfd. */
> + uint32_t fh;
> };
>
> /* refer to hw/virtio/vhost-user.c */
>
>
>
>
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
>
> On Mon, Apr 18, 2016 at 8:14 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com>
> wrote:
>
> On Mon, Apr 18, 2016 at 10:46:50AM -0700, Yuanhan Liu wrote:
> > On Mon, Apr 18, 2016 at 07:18:05PM +0200, Christian Ehrhardt wrote:
> > > I assume there is a leak somewhere on adding/removing vhost_user ports.
> > > Although it could also be "only" a fragmentation issue.
> > >
> > > Reproduction is easy:
> > > I set up a pair of nicely working OVS-DPDK connected KVM Guests.
> > > Then in a loop I
> > > - add up to more 512 ports
> > > - test connectivity between the two guests
> > > - remove up to 512 ports
> > >
> > > Depending on memory and the amount of multiqueue/rxq I use it seems to
> > > slightly change when exactly it breaks. But for my default setup of 4
> > > queues and 5G Hugepages initialized by DPDK it always breaks at the
> sixth
> > > iteration.
> > > Here a link to the stack trace indicating a memory shortage (TBC):
> > > https://launchpadlibrarian.net/253916410/apport-retrace.log
> > >
> > > Known Todos:
> > > - I want to track it down more, and will try to come up with a non
> > > openvswitch based looping testcase that might show it as well to
> simplify
> > > debugging.
> > > - in use were Openvswitch-dpdk 2.5 and DPDK 2.2; Retest with DPDK 16.04
> and
> > > Openvswitch master is planned.
> > >
> > > I will go on debugging this and let you know, but I wanted to give a
> heads
> > > up to everyone.
> >
> > Thanks for the report.
> >
> > > In case this is a known issue for some of you please let me know.
> >
> > Yeah, it might be. I'm wondering that virtio_net struct is not freed.
> > It will be freed only (if I'm not mistaken) when guest quits, by far.
>
> Would you try following diff and to see if it fix your issue?
>
> --yliu
>
> ---
> lib/librte_vhost/vhost_user/vhost-net-user.c | 6 ++++++
> lib/librte_vhost/vhost_user/vhost-net-user.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/
> librte_vhost/vhost_user/vhost-net-user.c
> index df2bd64..8f7ebd7 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -309,6 +309,7 @@ vserver_new_vq_conn(int fd, void *dat, __rte_unused int
> *remove)
> }
>
> vdev_ctx.fh = fh;
> + vserver->fh = fh;
> size = strnlen(vserver->path, PATH_MAX);
> vhost_set_ifname(vdev_ctx, vserver->path,
> size);
> @@ -501,6 +502,11 @@ rte_vhost_driver_unregister(const char *path)
>
> for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
> if (!strcmp(g_vhost_server.server[i]->path, path)) {
> + struct vhost_device_ctx ctx;
> +
> + ctx.fh = g_vhost_server.server[i]->fh;
> + vhost_destroy_device(ctx);
> +
> fdset_del(&g_vhost_server.fdset,
> g_vhost_server.server[i]->listenfd);
>
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h b/lib/
> librte_vhost/vhost_user/vhost-net-user.h
> index e3bb413..7cf21db 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.h
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
> @@ -43,6 +43,7 @@
> struct vhost_server {
> char *path; /**< The path the uds is bind to. */
> int listenfd; /**< The listener sockfd. */
> + uint32_t fh;
> };
>
> /* refer to hw/virtio/vhost-user.c */
> --
> 1.9.3
>
>
>
>
next prev parent reply other threads:[~2016-04-20 5:03 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-18 17:18 Christian Ehrhardt
2016-04-18 17:46 ` Yuanhan Liu
2016-04-18 18:14 ` Yuanhan Liu
2016-04-19 16:33 ` Christian Ehrhardt
2016-04-20 5:04 ` Yuanhan Liu [this message]
2016-04-20 6:18 ` Christian Ehrhardt
2016-04-21 5:54 ` Yuanhan Liu
2016-04-21 9:07 ` Christian Ehrhardt
2016-07-06 12:24 ` [dpdk-dev] [PATCH v2] " Christian Ehrhardt
2016-07-06 12:24 ` [dpdk-dev] [PATCH v2] vhost_user: avoid crash when exeeding file descriptors Christian Ehrhardt
2016-07-12 8:37 ` Yuanhan Liu
2016-07-15 19:46 ` Thomas Monjalon
2016-07-06 12:26 ` [dpdk-dev] [PATCH v2] Memory leak when adding/removing vhost_user ports Christian Ehrhardt
2016-07-06 12:30 ` Christian Ehrhardt
2016-07-06 12:37 ` Christian Ehrhardt
2016-07-06 13:08 ` Yuanhan Liu
2016-07-12 12:08 ` Yuanhan Liu
2016-07-19 13:50 ` Christian Ehrhardt
2016-04-21 11:01 ` [dpdk-dev] " Ilya Maximets
2016-04-21 14:04 ` Christian Ehrhardt
2016-04-21 16:56 ` Yuanhan Liu
2016-04-21 16:54 ` Yuanhan Liu
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=20160420050445.GB5872@yliu-dev.sh.intel.com \
--to=yuanhan.liu@linux.intel.com \
--cc=christian.ehrhardt@canonical.com \
--cc=dev@dpdk.org \
--cc=diproiettod@vmware.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).