DPDK patches and discussions
 help / color / mirror / Atom feed
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>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] test/mempool_perf: Free mempool on exit
Date: Mon, 10 Apr 2017 22:09:00 +0200	[thread overview]
Message-ID: <20170410220900.1ba05ecd@neon> (raw)
In-Reply-To: <d0ea6fc4-7cbb-8766-616e-097c4e0fbb14@caviumnetworks.com>

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

  parent reply	other threads:[~2017-04-10 20:09 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-05  8:51 [dpdk-dev] [PATCH " Santosh Shukla
2017-04-05  8:51 ` [dpdk-dev] [PATCH 2/2] test/mempool_perf: support default mempool autotest Santosh Shukla
2017-04-05 10:02   ` Shreyansh Jain
2017-04-05 12:40     ` santosh
2017-04-05  9:57 ` [dpdk-dev] [PATCH 1/2] test/mempool_perf: Free mempool on exit Shreyansh Jain
2017-04-05 12:33   ` santosh
2017-04-06  6:45 ` [dpdk-dev] [PATCH v2 " Santosh Shukla
2017-04-06  6:45   ` [dpdk-dev] [PATCH v2 2/2] test/mempool_perf: support default mempool autotest Santosh Shukla
2017-04-07 15:51   ` [dpdk-dev] [PATCH v2 1/2] test/mempool_perf: Free mempool on exit Olivier Matz
     [not found]     ` <BLUPR0701MB17140B8FD2D59B1A7835769FEA0E0@BLUPR0701MB1714.namprd07.prod.outlook.com>
     [not found]       ` <d0ea6fc4-7cbb-8766-616e-097c4e0fbb14@caviumnetworks.com>
2017-04-10 20:09         ` Olivier MATZ [this message]
2017-04-18  8:34   ` [dpdk-dev] [PATCH v3 1/3] test/test/mempool_perf: Remove mempool global vars Santosh Shukla
2017-04-18  8:34     ` [dpdk-dev] [PATCH v3 2/3] test/test/mempool_perf: Free mempool on exit Santosh Shukla
2017-04-18  8:34     ` [dpdk-dev] [PATCH v3 3/3] test/test/mempool_perf: support default mempool autotest Santosh Shukla
2017-04-18 13:42     ` [dpdk-dev] [PATCH v3 1/3] test/test/mempool_perf: Remove mempool global vars Olivier MATZ
2017-04-18 14:39       ` santosh
2017-04-18 14:41     ` [dpdk-dev] [PATCH v4 " Santosh Shukla
2017-04-18 14:41       ` [dpdk-dev] [PATCH v4 2/3] test/test/mempool_perf: Free mempool on exit Santosh Shukla
2017-04-18 14:41       ` [dpdk-dev] [PATCH v4 3/3] test/test/mempool_perf: support default mempool autotest Santosh Shukla
2017-04-18 15:31       ` [dpdk-dev] [PATCH v4 1/3] test/test/mempool_perf: Remove mempool global vars Olivier MATZ
2017-04-19 12:48         ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon

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=20170410220900.1ba05ecd@neon \
    --to=olivier.matz@6wind.com \
    --cc=dev@dpdk.org \
    --cc=santosh.shukla@caviumnetworks.com \
    --cc=shreyansh.jain@nxp.com \
    --cc=stable@dpdk.org \
    /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).