DPDK patches and discussions
 help / color / mirror / Atom feed
From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
To: Sarosh Arif <sarosh.arif@emumba.com>, david.hunt@intel.com
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] test_distributor.c: prevent memory leakages from the pool
Date: Wed, 15 Apr 2020 03:15:22 +0200
Message-ID: <dacb2be6-aa15-ce10-0fe8-f46fc25447ed@partner.samsung.com> (raw)
In-Reply-To: <20200413091906.782-1-sarosh.arif@emumba.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


  reply	other threads:[~2020-04-15  1:15 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20200413092039eucas1p1372014e18d014115634fc8eb23508004@eucas1p1.samsung.com>
2020-04-13  9:19 ` Sarosh Arif
2020-04-15  1:15   ` Lukasz Wojciechowski [this message]
2020-04-15  6:42   ` [dpdk-dev] [PATCH v2] app/test/test_distributor.c: " Sarosh Arif
2020-04-15  6:52     ` Lukasz Wojciechowski
2020-04-15  7:06       ` Sarosh Arif
2020-04-15  7:08         ` Lukasz Wojciechowski
     [not found]           ` <CABoZmYPV-GXESgjey7-g-j=cNif38O-SZ4L7TfrDTwb6KfDsog@mail.gmail.com>
     [not found]             ` <CABoZmYOhpYqV07Q4JguvSwJmAPzfhO-f8fMKUzsMoUdUGU5hjQ@mail.gmail.com>
2020-06-24 10:02               ` Sarosh Arif
2020-09-01 18:31                 ` Lukasz Wojciechowski
2020-09-08 10:22     ` [dpdk-dev] [v3 PATCH] test_distributor: " Sarosh Arif
2020-09-16 19:01       ` Lukasz Wojciechowski
2020-09-25 14:22       ` David Marchand
2020-09-25 15:31         ` David Hunt
2020-09-28  9:55         ` Sarosh Arif
2020-09-28 10:14           ` David Marchand
2020-09-25 15:26       ` David Hunt
2020-10-19  8:34         ` David Marchand

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=dacb2be6-aa15-ce10-0fe8-f46fc25447ed@partner.samsung.com \
    --to=l.wojciechow@partner.samsung.com \
    --cc=david.hunt@intel.com \
    --cc=dev@dpdk.org \
    --cc=sarosh.arif@emumba.com \
    /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

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