Re: Memory leak when adding/removing vhost_user ports

[Yuanhan Liu <yuanhan.liu@linux.intel.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
> 
> 
> 
>