From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <olivier.matz@6wind.com>
Received: from mail-wm0-f48.google.com (mail-wm0-f48.google.com [74.125.82.48])
by dpdk.org (Postfix) with ESMTP id 3BD1D532E
for <stable@dpdk.org>; Mon, 10 Apr 2017 22:09:04 +0200 (CEST)
Received: by mail-wm0-f48.google.com with SMTP id t189so46173464wmt.1
for <stable@dpdk.org>; Mon, 10 Apr 2017 13:09:04 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=6wind-com.20150623.gappssmtp.com; s=20150623;
h=date:from:to:cc:subject:message-id:in-reply-to:references
:mime-version:content-transfer-encoding;
bh=MLIcHbl29JfTxMwDAFgJSSUuLqBiQ5wdlz3ud3NqaDU=;
b=lEUQUyYsh+81HyOGiBQU0AftMb/8MVsISL8yeD4JERg8zMdu6t0MjSZH00oXjjtCBf
08A3biNACWHxxWlvwCLKDVBhRF79BWhMScFC4XogJTeB02F0Mw3OiieOo5+U+wEaTk6m
ZssRJFjgd4lajtYjW8q9xnXfzF2DAkN9Tb9/gWiUJqH3kaUdCv2nbg/G3iieyJD53wpA
JqHHIU/sTvBHa/rnsdzdnbHmn2aML6wTsmUF/r1m+yhf0zeLPPHIi3OkPjEptrDsN+y7
UwlsoHaV949zFRAOxHcbpkGnGnB/nM+iIdiCKxOeztonRzR+24f9Wx86QHzG/gPhaoYu
mmew==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to
:references:mime-version:content-transfer-encoding;
bh=MLIcHbl29JfTxMwDAFgJSSUuLqBiQ5wdlz3ud3NqaDU=;
b=ByieQYW/00Ua7slvjzYcH4SNsvYDwFItwAwLelN3nIdCqynNNGuxFbN69RLEcZmWWj
sLTniKbVoVdqqW8Jx34dgJdDiWcXXxayI3QbVGsAGf72mDo5eHdSnSSQaZVIWE7gZY+O
SSvVhCk5QF9rxV6rNt64+ejQAEMOTW9jh3NKx2U2RaPZ3wYbm/HTm8OQ3nc0AHy0TLKN
cCdcbxuaJ7fk5OdETUiFe6tU28Dq4LLsUL/TPDecXVZfXXPQxbs5Eel3tL9hO3FGYKaq
jbktzf1ZFaSKWz4ZEfbxxfEvb3TdeoYKTnHmHe7M/HOcEDHseBrovcoY9JqrKvfrEq7b
zl2Q==
X-Gm-Message-State: AN3rC/6bKkQIPebYZYcdTGvyMBP8lVjQmFI38FOj6MJZSHmS5ZOGjvKn
hZSTAeSi8Q54VgAj
X-Received: by 10.28.13.69 with SMTP id 66mr11249189wmn.137.1491854943797;
Mon, 10 Apr 2017 13:09:03 -0700 (PDT)
Received: from neon (bc535-h01-176-146-114-125.dsl.sta.abo.bbox.fr.
[176.146.114.125])
by smtp.gmail.com with ESMTPSA id v188sm11592306wmg.11.2017.04.10.13.09.03
(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);
Mon, 10 Apr 2017 13:09:03 -0700 (PDT)
Date: Mon, 10 Apr 2017 22:09:00 +0200
From: Olivier MATZ <olivier.matz@6wind.com>
To: santosh <santosh.shukla@caviumnetworks.com>
Cc: dev@dpdk.org, stable@dpdk.org, Shreyansh Jain <shreyansh.jain@nxp.com>
Message-ID: <20170410220900.1ba05ecd@neon>
In-Reply-To: <d0ea6fc4-7cbb-8766-616e-097c4e0fbb14@caviumnetworks.com>
References: <1491382264-23489-1-git-send-email-santosh.shukla@caviumnetworks.com>
<20170406064549.7966-1-santosh.shukla@caviumnetworks.com>
<20170407175102.4f2152c1@platinum>
<BLUPR0701MB17140B8FD2D59B1A7835769FEA0E0@BLUPR0701MB1714.namprd07.prod.outlook.com>
<d0ea6fc4-7cbb-8766-616e-097c4e0fbb14@caviumnetworks.com>
X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-stable] [PATCH v2 1/2] test/mempool_perf: Free mempool on
exit
X-BeenThere: stable@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches for DPDK stable branches <stable.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/stable>,
<mailto:stable-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/stable/>
List-Post: <mailto:stable@dpdk.org>
List-Help: <mailto:stable-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/stable>,
<mailto:stable-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 10 Apr 2017 20:09:04 -0000
Hi Santosh,
On Mon, 10 Apr 2017 01:13:43 +0530
santosh <santosh.shukla@caviumnetworks.com> wrote:
> Hi Olivier,
>
> On Monday 10 April 2017 12:47 AM, Shukla, Santosh wrote:
>
> >
> >
> > --------------------------------------------------------------------------------
> > *From:* Olivier Matz <olivier.matz@6wind.com>
> > *Sent:* Friday, April 7, 2017 9:21 PM
> > *To:* Shukla, Santosh
> > *Cc:* dev@dpdk.org; hemant.agrawal@nxp.com; shreyansh.jain@nxp.com; stable@dpdk.org
> > *Subject:* Re: [PATCH v2 1/2] test/mempool_perf: Free mempool on exit
> > Hi Santosh,
> >
> > On Thu, 6 Apr 2017 12:15:48 +0530, Santosh Shukla
> > <santosh.shukla@caviumnetworks.com> wrote:
> > > Mempool_perf test not freeing pool memory.
> > >
> > > Cc: stable@dpdk.org
> > > Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> > > Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> > > ---
> > > v1 --> v2:
> > > * Fixed patch context
> > >
> > > test/test/test_mempool_perf.c | 31 +++++++++++++++++++------------
> > > 1 file changed, 19 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/test/test/test_mempool_perf.c b/test/test/test_mempool_perf.c
> > > index ebf1721ac..3c45971ab 100644
> > > --- a/test/test/test_mempool_perf.c
> > > +++ b/test/test/test_mempool_perf.c
> > > @@ -312,6 +312,8 @@ do_one_mempool_test(unsigned cores)
> > > static int
> > > test_mempool_perf(void)
> > > {
> > > + int ret = -1;
> > > +
> > > rte_atomic32_init(&synchro);
> > >
> > > /* create a mempool (without cache) */
> > > @@ -322,7 +324,7 @@ test_mempool_perf(void)
> > > my_obj_init, NULL,
> > > SOCKET_ID_ANY, 0);
> > > if (mp_nocache == NULL)
> > > - return -1;
> > > + goto err;
> > >
> > > /* create a mempool (with cache) */
> > > if (mp_cache == NULL)
> >
> > [...]
> >
> > >
> > > - return 0;
> > > + ret = 0;
> > > +
> > > +err:
> > > + rte_mempool_free(mp_cache);
> > > + rte_mempool_free(mp_nocache);
> > > + return ret;
> >
> >
> > Since mp_cache and mp_nocache are global variables, this won't
> > work properly due to the way mempool are created:
> >
> > /* create a mempool (without cache) */
> > if (mp_nocache == NULL)
> > mp_nocache = rte_mempool_create("perf_test_nocache", MEMPOOL_SIZE,
> > MEMPOOL_ELT_SIZE, 0, 0,
> > NULL, NULL,
> > my_obj_init, NULL,
> > SOCKET_ID_ANY, 0);
> >
> > The if() should be removed, else we'll have a use after free the next
> > time.
>
> I understand your point.
> But I think problem is rte_mempool_free() not referencing mp = null
> after freeing resources. Result of that is mp_nocache still has valid
> address, Although internal resources (mz/_ops_handle) were actually freed by
> rte_mempool_free(), right?
>
> So rather removing above if(), why not
> - Application explicit set mp_nocache = NULL after mempool_free().
> ie..
>
> err:
> rte_mempool_free(xxx);
> xxx = NULL;
>
>
> Or
> - Let rte_mempool_free() { - do mp = null; }
>
> And yes remove that if condition anyway. As its a dead-code
> for either of above 2 options.
>
> Does that make sense to you? If so then which one you prefer?
Yes, it makes sense.
My first preference would be removing the global vars (as suggested
below). Else your proposition is ok too.
>
> > If you want to do more clean-up, you can try to remove the global variables,
> > but it's maybe harder.
>
> Removing global var won't be harder imo, May be you know more but
> here is my point of view, after going through code:
>
> - All test_func like
> do_one_mempool_test -> launch_cores --> per_lcore_mempool_test -> using 'mp'
>
> where 'mp' is global.
>
> how about,
> - As you said Yes - remove global var ie.. mp_cache/nocache, default_pool, mp
> - Add 'rte_mempool *mp' as argument in do_one_mempool_test() func and other func too.
>
> Thus get rid of globals from app.
>
> Does that make sense to you?
Yes, looks good. It would be clearer without global vars.
Historically, it was not possible to free a mempool, that's why it was
done like this.
Thanks!
Olivier