diff mbox series

[v3] test_distributor: prevent memory leakages from the pool

Message ID 20200908102204.727240-1-sarosh.arif@emumba.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series [v3] test_distributor: prevent memory leakages from the pool |

Checks

Context Check Description
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Sarosh Arif Sept. 8, 2020, 10:22 a.m. UTC 
  rte_mempool_get_bulk is used to get bufs/many_bufs from the pool,
but at some locations when test fails the bufs/many_bufs are
not returned back to the pool.
Due to this, multiple executions of distributor_autotest gives the
following error message: Error getting mbufs from pool.
To resolve this issue rte_mempool_put_bulk is used whenever the test
fails and returns.

Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
---
v2:
remove double freeing of mbufs
v3:
resubmit to run the tests again
---
 app/test/test_distributor.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Lukasz Wojciechowski Sept. 16, 2020, 7:01 p.m. UTC | #1 
W dniu 08.09.2020 o 12:22, Sarosh Arif pisze:
> rte_mempool_get_bulk is used to get bufs/many_bufs from the pool,
> but at some locations when test fails the bufs/many_bufs are
> not returned back to the pool.
> Due to this, multiple executions of distributor_autotest gives the
> following error message: Error getting mbufs from pool.
> To resolve this issue rte_mempool_put_bulk is used whenever the test
> fails and returns.
>
> Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> ---
> v2:
> remove double freeing of mbufs
> v3:
> resubmit to run the tests again
> ---
>   app/test/test_distributor.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> index ba1f81cf8..1a893a7d9 100644
> --- a/app/test/test_distributor.c
> +++ b/app/test/test_distributor.c
> @@ -128,6 +128,7 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
>   		printf("Line %d: Error, not all packets flushed. "
>   				"Expected %u, got %u\n",
>   				__LINE__, BURST, total_packet_count());
> +		rte_mempool_put_bulk(p, (void *)bufs, BURST);
>   		return -1;
>   	}
>   
> @@ -153,6 +154,7 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
>   			printf("Line %d: Error, not all packets flushed. "
>   					"Expected %u, got %u\n",
>   					__LINE__, BURST, total_packet_count());
> +			rte_mempool_put_bulk(p, (void *)bufs, BURST);
>   			return -1;
>   		}
>   
> @@ -179,6 +181,7 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
>   		printf("Line %d: Error, not all packets flushed. "
>   				"Expected %u, got %u\n",
>   				__LINE__, BURST, total_packet_count());
> +		rte_mempool_put_bulk(p, (void *)bufs, BURST);
>   		return -1;
>   	}
>   
> @@ -233,6 +236,7 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
>   	if (num_returned != BIG_BATCH) {
>   		printf("line %d: Missing packets, expected %d\n",
>   				__LINE__, num_returned);
> +		rte_mempool_put_bulk(p, (void *)many_bufs, BIG_BATCH);
>   		return -1;
>   	}
>   
> @@ -247,6 +251,7 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
>   
>   		if (j == BIG_BATCH) {
>   			printf("Error: could not find source packet #%u\n", i);
> +			rte_mempool_put_bulk(p, (void *)many_bufs, BIG_BATCH);
>   			return -1;
>   		}
>   	}
Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
David Marchand Sept. 25, 2020, 2:22 p.m. UTC | #2 
On Tue, Sep 8, 2020 at 12:22 PM Sarosh Arif <sarosh.arif@emumba.com> wrote:
>
> rte_mempool_get_bulk is used to get bufs/many_bufs from the pool,
> but at some locations when test fails the bufs/many_bufs are
> not returned back to the pool.
> Due to this, multiple executions of distributor_autotest gives the
> following error message: Error getting mbufs from pool.
> To resolve this issue rte_mempool_put_bulk is used whenever the test
> fails and returns.
>
> Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>

It deserves a Fixes: line.

I can see a ack from Lukasz.
David, review please?
Hunt, David Sept. 25, 2020, 3:26 p.m. UTC | #3 
On 8/9/2020 11:22 AM, Sarosh Arif wrote:
> rte_mempool_get_bulk is used to get bufs/many_bufs from the pool,
> but at some locations when test fails the bufs/many_bufs are
> not returned back to the pool.
> Due to this, multiple executions of distributor_autotest gives the
> following error message: Error getting mbufs from pool.
> To resolve this issue rte_mempool_put_bulk is used whenever the test
> fails and returns.
>
> Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> ---
> v2:
> remove double freeing of mbufs
> v3:
> resubmit to run the tests again
> ---
> }

--snip--

Looks good to me. Even though I could not repeat the conditions to cause 
one of these errors, the additons make sense.

In my testing, I did add in several rte_mempool_avail_count() checks to 
see if there were leakages, and all the mempool counts were
stable, but that was due to the fixes in the patch set from Lukasz : 
http://patches.dpdk.org/project/dpdk/list/?series=12442

However, if there are situations where packets are not returned from 
workers, this should clean them up nicely before returning.

Reviewed-by: David Hunt <david.hunt@intel.com>
Hunt, David Sept. 25, 2020, 3:31 p.m. UTC | #4 
Hi David,

On 25/9/2020 3:22 PM, David Marchand wrote:
> On Tue, Sep 8, 2020 at 12:22 PM Sarosh Arif <sarosh.arif@emumba.com> wrote:
>> rte_mempool_get_bulk is used to get bufs/many_bufs from the pool,
>> but at some locations when test fails the bufs/many_bufs are
>> not returned back to the pool.
>> Due to this, multiple executions of distributor_autotest gives the
>> following error message: Error getting mbufs from pool.
>> To resolve this issue rte_mempool_put_bulk is used whenever the test
>> fails and returns.
>>
>> Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> It deserves a Fixes: line.
>
> I can see a ack from Lukasz.
> David, review please?

Done. Apoligies, I should have put you on CC:

My review comments have just been sent to the mailing list.

Rgds,
Dave.
Sarosh Arif Sept. 28, 2020, 9:55 a.m. UTC | #5 
On Fri, Sep 25, 2020 at 7:22 PM David Marchand <david.marchand@redhat.com>
wrote:

> On Tue, Sep 8, 2020 at 12:22 PM Sarosh Arif <sarosh.arif@emumba.com>
> wrote:
> >
> > rte_mempool_get_bulk is used to get bufs/many_bufs from the pool,
> > but at some locations when test fails the bufs/many_bufs are
> > not returned back to the pool.
> > Due to this, multiple executions of distributor_autotest gives the
> > following error message: Error getting mbufs from pool.
> > To resolve this issue rte_mempool_put_bulk is used whenever the test
> > fails and returns.
> >
> > Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
>
> It deserves a Fixes: line.
>
Fixes: c3eabff124e6 ("distributor: add unit tests")

Should I submit another version of this patch to add fixes?

>
> I can see a ack from Lukasz.
> David, review please?
>
>
> --
> David Marchand
>
>
David Marchand Sept. 28, 2020, 10:14 a.m. UTC | #6 
On Mon, Sep 28, 2020 at 11:55 AM Sarosh Arif <sarosh.arif@emumba.com> wrote:
> On Fri, Sep 25, 2020 at 7:22 PM David Marchand <david.marchand@redhat.com> wrote:
>> It deserves a Fixes: line.
>
> Fixes: c3eabff124e6 ("distributor: add unit tests")
>
> Should I submit another version of this patch to add fixes?

I was waiting for a review from David H. and this Fixes line lgtm.
I will update when applying (with the other ut tests fixes from
Lukasz), no need for a new revision.

Thanks.
David Marchand Oct. 19, 2020, 8:34 a.m. UTC | #7 
On Fri, Sep 25, 2020 at 5:27 PM David Hunt <david.hunt@intel.com> wrote:
> On 8/9/2020 11:22 AM, Sarosh Arif wrote:
> > rte_mempool_get_bulk is used to get bufs/many_bufs from the pool,
> > but at some locations when test fails the bufs/many_bufs are
> > not returned back to the pool.
> > Due to this, multiple executions of distributor_autotest gives the
> > following error message: Error getting mbufs from pool.
> > To resolve this issue rte_mempool_put_bulk is used whenever the test
> > fails and returns.
> >

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: stable@dpdk.org

> > Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Reviewed-by: David Hunt <david.hunt@intel.com>

Applied, thanks Sarosh.
diff mbox series

Patch

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index ba1f81cf8..1a893a7d9 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -128,6 +128,7 @@  sanity_test(struct worker_params *wp, struct rte_mempool *p)
 		printf("Line %d: Error, not all packets flushed. "
 				"Expected %u, got %u\n",
 				__LINE__, BURST, total_packet_count());
+		rte_mempool_put_bulk(p, (void *)bufs, BURST);
 		return -1;
 	}
 
@@ -153,6 +154,7 @@  sanity_test(struct worker_params *wp, struct rte_mempool *p)
 			printf("Line %d: Error, not all packets flushed. "
 					"Expected %u, got %u\n",
 					__LINE__, BURST, total_packet_count());
+			rte_mempool_put_bulk(p, (void *)bufs, BURST);
 			return -1;
 		}
 
@@ -179,6 +181,7 @@  sanity_test(struct worker_params *wp, struct rte_mempool *p)
 		printf("Line %d: Error, not all packets flushed. "
 				"Expected %u, got %u\n",
 				__LINE__, BURST, total_packet_count());
+		rte_mempool_put_bulk(p, (void *)bufs, BURST);
 		return -1;
 	}
 
@@ -233,6 +236,7 @@  sanity_test(struct worker_params *wp, struct rte_mempool *p)
 	if (num_returned != BIG_BATCH) {
 		printf("line %d: Missing packets, expected %d\n",
 				__LINE__, num_returned);
+		rte_mempool_put_bulk(p, (void *)many_bufs, BIG_BATCH);
 		return -1;
 	}
 
@@ -247,6 +251,7 @@  sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 		if (j == BIG_BATCH) {
 			printf("Error: could not find source packet #%u\n", i);
+			rte_mempool_put_bulk(p, (void *)many_bufs, BIG_BATCH);
 			return -1;
 		}
 	}