Re: [dpdk-dev] [PATCH] test_distributor.c: prevent memory leakages from the pool

[Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>]

Hi, Sarosh

I believe commit message title should begin with app/test not the file name

Other comments inline

W dniu 13.04.2020 o 11:19, Sarosh Arif pisze:
> rte_mempool_get_bulk is used to get bufs/many_bufs from the pool,
> but at some locations when test passes/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
> passes/fails and returns.
>
> Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> ---
>   app/test/test_distributor.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> index ba1f81cf8..8608b4ce8 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;
>   		}
>   	}
> @@ -327,10 +332,12 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
>   		printf("Line %u: Packet count is incorrect, %u, expected %u\n",
>   				__LINE__, total_packet_count(),
>   				(1<<ITER_POWER));
> +		rte_mempool_put_bulk(p, (void *)bufs, BURST);
This modification can cause double free of mbufs, as they are freed 
already with rte_pktmbuf_free in handle_work_with_free_mbufs (line 290).
Even if you assume that this condition will be run when not all of the 
packets were processed by worker, you don't know which were and which 
were not processed.
Please remove it from your patch.
>   		return -1;
>   	}
>   
>   	printf("Sanity test with mbuf alloc/free passed\n\n");
> +	rte_mempool_put_bulk(p, (void *)bufs, BURST);

This modification causes double free of mbufs, as they are freed already 
with rte_pktmbuf_free in handle_work_with_free_mbufs (line 290).
Please remove it from your patch.

>   	return 0;
>   }
>   

> @@ -450,10 +457,14 @@ sanity_test_with_worker_shutdown(struct worker_params *wp,
>   		printf("Line %d: Error, not all packets flushed. "
>   				"Expected %u, got %u\n",
>   				__LINE__, BURST * 2, total_packet_count());
> +		for (i = 0; i < 2; i++)
> +			rte_mempool_put_bulk(p, (void *)bufs, BURST);
This modification can cause double free of mbufs, as they are freed 
already with rte_pktmbuf_free in handle_work_for_shutdown_test (line 367).
Please remove it from your patch.
>   		return -1;
>   	}
>   
>   	printf("Sanity test with worker shutdown passed\n\n");
> +	for (i = 0; i < 2; i++)
> +		rte_mempool_put_bulk(p, (void *)bufs, BURST);
This modification causes double free of mbufs, as they are freed already 
with rte_pktmbuf_free in handle_work_for_shutdown_test (line 367).
Please remove it from your patch.
>   	return 0;
>   }
>   
> @@ -503,10 +514,12 @@ test_flush_with_worker_shutdown(struct worker_params *wp,
>   		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);
This modification can cause double free of mbufs, as they are freed 
already with rte_pktmbuf_free in handle_work_for_shutdown_test (line 367).
Please remove it from your patch.
>   		return -1;
>   	}
>   
>   	printf("Flush test with worker shutdown passed\n\n");
> +	rte_mempool_put_bulk(p, (void *)bufs, BURST);
This modification causes double free of mbufs, as they are freed already 
with rte_pktmbuf_free in handle_work_for_shutdown_test (line 367).
Please remove it from your patch.
>   	return 0;
>   }
>   
Generally there are 3 handlers for workers:
* handle_work
* handle_work_with_free_mbufs
* handle_work_for_shutdown_test

The first one doesn't do anything with mbufs and sens it back to the 
test, where they should be released.
In the other 2 handlers mbufs are released, so they shouldn't be put 
back to mempool in test function.

Keeping this simple rules will be the asy way to clean the code and 
manage pool object properly.

Please consider also, that the there are still few places, where this 
rules are broken and there is one more tricky place: quit_workers 
function, which is called in all tests.

You can easily test proper usage of mempool by defining 
RTE_LIBRTE_MEMPOOL_DEBUG to 1


-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com