From patchwork Tue Apr 19 16:33:50 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christian Ehrhardt X-Patchwork-Id: 12118 X-Patchwork-Delegate: yuanhan.liu@linux.intel.com Return-Path: X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 47C7A2C6A; Tue, 19 Apr 2016 18:34:13 +0200 (CEST) Received: from mail-qg0-f44.google.com (mail-qg0-f44.google.com [209.85.192.44]) by dpdk.org (Postfix) with ESMTP id 0D3022C37 for ; Tue, 19 Apr 2016 18:34:11 +0200 (CEST) Received: by mail-qg0-f44.google.com with SMTP id f74so10876393qge.2 for ; Tue, 19 Apr 2016 09:34:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=qhoCo/S5AQfbTqMhFqz+IrCe4D2XGITKrL3WikZw20Y=; b=eCeGF4FpCfW3a03utlYT4NZOIkNvSG6qbm3e82XLPxe3mMfkzHADByaEcUhtKWlaUY iF7VSD3FRqsQQlV+URBrNIsE0GGGKvJ9XIdD55i08Bl3Kcrc68+EfztXo486Ubh6R8Dn EWrmF5vLnEw9HPf54rNoUw9e3+3EwTkbVQRcOR4ZWaJy8gCOZEzthgkr7+fsM3jHZKTW zrOVMyFNJ77J8LpBlIfKM7eSriQOoGOt0VeX0CuJ6dzM1GMIpzjN1D26NtWSQT+GNtX5 rR7us5PWQTCi5giIGc6erZbJW6UNK9AM3Nggybarrfs7Y5SxkT4SMZbFzd7eGe4uI41I bVEA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=qhoCo/S5AQfbTqMhFqz+IrCe4D2XGITKrL3WikZw20Y=; b=J2p7j8YERN614aJ+gpxO67+PrI7ozPbOFQVgIO4mbalzlNejQmnScI9psGBlVzHZwU F3O4ERHCbnEL/UDS63pSiCoo6l8SkfBSiJWDcu0Vuuu6KaEC6qtMpo1AmBWWpwTnppC3 C5WClTk4ZJ5eyhWr3qpPzWQ+TNnXQxnFZDCymW+bETRtTXY0X/7JwSdMiSywHnAKaOza diAfrlmZkHai6MmUMJYPa80q9g3sOl2efO91q26dIcSqm3Z26JcrgrbQc39rvC7vEWK0 HyP6BEa8LxkyqjbTxvdRD68LyTebNJ7BRvrPsq9U5GYC+KPirCcwSbUo5h658zI+uQzE IqGA== X-Gm-Message-State: AOPr4FVFMNX+JjePYAKOYSLasFwNxxH+Sp3T3PEbe+Ms4hZkQzkOHqArnyr+OvCgFDsz/48thrRZAxufHqSsai/c X-Received: by 10.140.92.65 with SMTP id a59mr3497361qge.93.1461083650256; Tue, 19 Apr 2016 09:34:10 -0700 (PDT) MIME-Version: 1.0 Received: by 10.55.212.91 with HTTP; Tue, 19 Apr 2016 09:33:50 -0700 (PDT) In-Reply-To: <20160418181422.GE2576@yliu-dev.sh.intel.com> References: <20160418174650.GD2576@yliu-dev.sh.intel.com> <20160418181422.GE2576@yliu-dev.sh.intel.com> From: Christian Ehrhardt Date: Tue, 19 Apr 2016 18:33:50 +0200 Message-ID: To: Yuanhan Liu Cc: dev , Daniele Di Proietto X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] Memory leak when adding/removing vhost_user ports X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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) Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd On Mon, Apr 18, 2016 at 8:14 PM, Yuanhan Liu 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 > > > 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 */