DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] mempool: don't leak ring on failure
@ 2014-06-24 15:49 Stephen Hemminger
  2014-06-24 16:16 ` Ananyev, Konstantin
  2014-06-25  7:46 ` Olivier MATZ
  0 siblings, 2 replies; 7+ messages in thread
From: Stephen Hemminger @ 2014-06-24 15:49 UTC (permalink / raw)
  To: dev


If mempool can not be created because of insufficient memory
it returns an error but has already created a ring (and leaves it
behind). This prevents code from trying one mempool size and then
retrying with a smaller size if the bigger size fails.

Reordering to do ring creation after getting memory fixes
the problem.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>


--- a/lib/librte_mempool/rte_mempool.c	2014-06-24 08:20:28.513771717 -0700
+++ b/lib/librte_mempool/rte_mempool.c	2014-06-24 08:20:28.513771717 -0700
@@ -473,15 +473,6 @@ rte_mempool_xmem_create(const char *name
 
 	rte_rwlock_write_lock(RTE_EAL_MEMPOOL_RWLOCK);
 
-	/* allocate the ring that will be used to store objects */
-	/* Ring functions will return appropriate errors if we are
-	 * running as a secondary process etc., so no checks made
-	 * in this function for that condition */
-	rte_snprintf(rg_name, sizeof(rg_name), RTE_MEMPOOL_MZ_FORMAT, name);
-	r = rte_ring_create(rg_name, rte_align32pow2(n+1), socket_id, rg_flags);
-	if (r == NULL)
-		goto exit;
-
 	/*
 	 * reserve a memory zone for this mempool: private data is
 	 * cache-aligned
@@ -542,6 +533,15 @@ rte_mempool_xmem_create(const char *name
 		startaddr = (void*)addr;
 	}
 
+	/* allocate the ring that will be used to store objects */
+	/* Ring functions will return appropriate errors if we are
+	 * running as a secondary process etc., so no checks made
+	 * in this function for that condition */
+	rte_snprintf(rg_name, sizeof(rg_name), RTE_MEMPOOL_MZ_FORMAT, name);
+	r = rte_ring_create(rg_name, rte_align32pow2(n+1), socket_id, rg_flags);
+	if (r == NULL)
+		goto exit;
+
 	/* init the mempool structure */
 	mp = startaddr;
 	memset(mp, 0, sizeof(*mp));

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: don't leak ring on failure
  2014-06-24 15:49 [dpdk-dev] [PATCH] mempool: don't leak ring on failure Stephen Hemminger
@ 2014-06-24 16:16 ` Ananyev, Konstantin
  2014-06-24 17:40   ` Stephen Hemminger
  2014-06-24 17:41   ` Richardson, Bruce
  2014-06-25  7:46 ` Olivier MATZ
  1 sibling, 2 replies; 7+ messages in thread
From: Ananyev, Konstantin @ 2014-06-24 16:16 UTC (permalink / raw)
  To: Stephen Hemminger, dev

Hi Stephen,

> 
> If mempool can not be created because of insufficient memory
> it returns an error but has already created a ring (and leaves it
> behind). This prevents code from trying one mempool size and then
> retrying with a smaller size if the bigger size fails.
> 
> Reordering to do ring creation after getting memory fixes
> the problem.
>

But now, memzone created for the actual mempool could get leaked instead?
 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> 
> --- a/lib/librte_mempool/rte_mempool.c	2014-06-24 08:20:28.513771717 -0700
> +++ b/lib/librte_mempool/rte_mempool.c	2014-06-24 08:20:28.513771717 -0700
> @@ -473,15 +473,6 @@ rte_mempool_xmem_create(const char *name
> 
>  	rte_rwlock_write_lock(RTE_EAL_MEMPOOL_RWLOCK);
> 
> -	/* allocate the ring that will be used to store objects */
> -	/* Ring functions will return appropriate errors if we are
> -	 * running as a secondary process etc., so no checks made
> -	 * in this function for that condition */
> -	rte_snprintf(rg_name, sizeof(rg_name), RTE_MEMPOOL_MZ_FORMAT, name);
> -	r = rte_ring_create(rg_name, rte_align32pow2(n+1), socket_id, rg_flags);
> -	if (r == NULL)
> -		goto exit;
> -
>  	/*
>  	 * reserve a memory zone for this mempool: private data is
>  	 * cache-aligned
> @@ -542,6 +533,15 @@ rte_mempool_xmem_create(const char *name
>  		startaddr = (void*)addr;
>  	}
> 
> +	/* allocate the ring that will be used to store objects */
> +	/* Ring functions will return appropriate errors if we are
> +	 * running as a secondary process etc., so no checks made
> +	 * in this function for that condition */
> +	rte_snprintf(rg_name, sizeof(rg_name), RTE_MEMPOOL_MZ_FORMAT, name);
> +	r = rte_ring_create(rg_name, rte_align32pow2(n+1), socket_id, rg_flags);
> +	if (r == NULL)
> +		goto exit;
> +
>  	/* init the mempool structure */
>  	mp = startaddr;
>  	memset(mp, 0, sizeof(*mp));

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: don't leak ring on failure
  2014-06-24 16:16 ` Ananyev, Konstantin
@ 2014-06-24 17:40   ` Stephen Hemminger
  2014-06-24 17:48     ` Ananyev, Konstantin
  2014-06-24 17:41   ` Richardson, Bruce
  1 sibling, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2014-06-24 17:40 UTC (permalink / raw)
  To: Ananyev, Konstantin; +Cc: dev

On Tue, 24 Jun 2014 16:16:02 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> But now, memzone created for the actual mempool could get leaked instead?

Since memzone's can't be destroyed, then only solution would be if
checked if memzone with same name already exists.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: don't leak ring on failure
  2014-06-24 16:16 ` Ananyev, Konstantin
  2014-06-24 17:40   ` Stephen Hemminger
@ 2014-06-24 17:41   ` Richardson, Bruce
  1 sibling, 0 replies; 7+ messages in thread
From: Richardson, Bruce @ 2014-06-24 17:41 UTC (permalink / raw)
  To: Ananyev, Konstantin, Stephen Hemminger, dev

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Tuesday, June 24, 2014 9:16 AM
> To: Stephen Hemminger; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] mempool: don't leak ring on failure
> 
> Hi Stephen,
> 
> >
> > If mempool can not be created because of insufficient memory
> > it returns an error but has already created a ring (and leaves it
> > behind). This prevents code from trying one mempool size and then
> > retrying with a smaller size if the bigger size fails.
> >
> > Reordering to do ring creation after getting memory fixes
> > the problem.
> >
> 
> But now, memzone created for the actual mempool could get leaked instead?
> 

Is either of these a problem that really needs to be fixed? If there is not enough memory to create the mempool what is the app likely to do, other than just quit with an error message?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: don't leak ring on failure
  2014-06-24 17:40   ` Stephen Hemminger
@ 2014-06-24 17:48     ` Ananyev, Konstantin
  0 siblings, 0 replies; 7+ messages in thread
From: Ananyev, Konstantin @ 2014-06-24 17:48 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

> 
> On Tue, 24 Jun 2014 16:16:02 +0000
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> 
> > But now, memzone created for the actual mempool could get leaked instead?
> 
> Since memzone's can't be destroyed, then only solution would be if
> checked if memzone with same name already exists.

Actually, wouldn't it be the better way to create one memzone for both mempool and its ring?
Now we have rte_ring_init().

Konstantin

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: don't leak ring on failure
  2014-06-24 15:49 [dpdk-dev] [PATCH] mempool: don't leak ring on failure Stephen Hemminger
  2014-06-24 16:16 ` Ananyev, Konstantin
@ 2014-06-25  7:46 ` Olivier MATZ
  2014-06-25  8:01   ` Olivier MATZ
  1 sibling, 1 reply; 7+ messages in thread
From: Olivier MATZ @ 2014-06-25  7:46 UTC (permalink / raw)
  To: Stephen Hemminger, dev

Hello Stephen,

On 06/24/2014 05:49 PM, Stephen Hemminger wrote:
> If mempool can not be created because of insufficient memory
> it returns an error but has already created a ring (and leaves it
> behind). This prevents code from trying one mempool size and then
> retrying with a smaller size if the bigger size fails.
>
> Reordering to do ring creation after getting memory fixes
> the problem.

Your patch moves the creation of the ring after the call to
rte_memzone_reserve(), so now it tries to create the memory
for the object pool before the ring. The problem disappears
because the object pool is usually much bigger than the ring,
so once the first allocation is done, the second is unlikely
to fail.

I think this explanation could be added in the commit log.

Acked-by: Olivier Matz <olivier.matz@6wind.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: don't leak ring on failure
  2014-06-25  7:46 ` Olivier MATZ
@ 2014-06-25  8:01   ` Olivier MATZ
  0 siblings, 0 replies; 7+ messages in thread
From: Olivier MATZ @ 2014-06-25  8:01 UTC (permalink / raw)
  To: Stephen Hemminger, dev

On 06/25/2014 09:46 AM, Olivier MATZ wrote:
> Your patch moves the creation of the ring after the call to
> rte_memzone_reserve(), so now it tries to create the memory
> for the object pool before the ring. The problem disappears
> because the object pool is usually much bigger than the ring,
> so once the first allocation is done, the second is unlikely
> to fail.
>
> I think this explanation could be added in the commit log.
>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Sorry, I didn't see that Konstantin and Bruce already answered to
this.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-06-25  8:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-24 15:49 [dpdk-dev] [PATCH] mempool: don't leak ring on failure Stephen Hemminger
2014-06-24 16:16 ` Ananyev, Konstantin
2014-06-24 17:40   ` Stephen Hemminger
2014-06-24 17:48     ` Ananyev, Konstantin
2014-06-24 17:41   ` Richardson, Bruce
2014-06-25  7:46 ` Olivier MATZ
2014-06-25  8:01   ` Olivier MATZ

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git