Re: [dpdk-dev] Memory leak when adding/removing vhost_user ports

[Christian Ehrhardt <christian.ehrhardt@canonical.com>]

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.
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
>
>
>