[PATCH] mempool: don't leak ring on failure

[PATCH] mempool: don't leak ring on failure

[Stephen Hemminger]

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-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.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));

Re: [PATCH] mempool: don't leak ring on failure

[Ananyev, Konstantin]

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-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.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));

Re: [PATCH] mempool: don't leak ring on failure

[Stephen Hemminger]

On Tue, 24 Jun 2014 16:16:02 +0000
"Ananyev, Konstantin" <konstantin.ananyev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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.

Re: [PATCH] mempool: don't leak ring on failure

[Ananyev, Konstantin]

> 
> On Tue, 24 Jun 2014 16:16:02 +0000
> "Ananyev, Konstantin" <konstantin.ananyev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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

Re: [PATCH] mempool: don't leak ring on failure

[Richardson, Bruce]

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Ananyev, Konstantin
> Sent: Tuesday, June 24, 2014 9:16 AM
> To: Stephen Hemminger; dev-VfR2kkLFssw@public.gmane.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?

Re: [PATCH] mempool: don't leak ring on failure

[Olivier MATZ]

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-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>

Re: [PATCH] mempool: don't leak ring on failure

[Olivier MATZ]

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-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>

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