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
next prev parent 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