From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 261F2A057B; Wed, 15 Apr 2020 03:15:30 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E9AB91D40F; Wed, 15 Apr 2020 03:15:28 +0200 (CEST) Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 0F9B91D406 for ; Wed, 15 Apr 2020 03:15:27 +0200 (CEST) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20200415011527euoutp02b40767ebfaaa08055087ef89e06aaff4~F2UNiXWgN1059010590euoutp02m for ; Wed, 15 Apr 2020 01:15:27 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20200415011527euoutp02b40767ebfaaa08055087ef89e06aaff4~F2UNiXWgN1059010590euoutp02m DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1586913327; bh=AdjNPsbLa6BOvRf7+iCtRkHe0m2PYlv75NZ5iId8s08=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=fjFItu6zK6O5Obrw+jpdA9BP1iPhET2o3vlA6+PhBt2LXO1p8dQ8Ky9uUFjBCyCVV UcdY0zrqrRsReN5ndVJp0Q212vM1T9b774BIZMvhJdh9EVXiAOqF8iQatpA4lcjf0d TJCr7OwXp2EpUM+3xFdlYOSwU0cVzsaw6mW8pe34= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20200415011526eucas1p18d9b66aee26f5b9dcf4140f6b16fc007~F2UMhs5l62357223572eucas1p14; Wed, 15 Apr 2020 01:15:26 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id 04.F4.60698.D20669E5; Wed, 15 Apr 2020 02:15:25 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20200415011525eucas1p1869aa77acb6adfa08fb5fed9f45a8094~F2ULkNG6T0570305703eucas1p1M; Wed, 15 Apr 2020 01:15:25 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20200415011525eusmtrp1844c28d316523be5bb150a6a9486dbf1~F2ULjpZsC1003110031eusmtrp1m; Wed, 15 Apr 2020 01:15:25 +0000 (GMT) X-AuditID: cbfec7f5-a29ff7000001ed1a-dc-5e96602de950 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id 09.A7.07950.C20669E5; Wed, 15 Apr 2020 02:15:24 +0100 (BST) Received: from [106.210.88.70] (unknown [106.210.88.70]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20200415011524eusmtip2c0b7e873f4baf243144ea3d8305b3f4d~F2UK3uE6K0278502785eusmtip2O; Wed, 15 Apr 2020 01:15:24 +0000 (GMT) To: Sarosh Arif , david.hunt@intel.com Cc: dev@dpdk.org From: Lukasz Wojciechowski Message-ID: Date: Wed, 15 Apr 2020 03:15:22 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200413091906.782-1-sarosh.arif@emumba.com> Content-Transfer-Encoding: 8bit Content-Language: pl X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupileLIzCtJLcpLzFFi42LZduzneV3dhGlxBsf3W1n0TfrIZPHu03Ym i1PrvrM7MHv8WrCU1eP0zyfMHov3vGQKYI7isklJzcksSy3St0vgyjiwZQd7wUe9ij0T17E2 MH5X6WLk5JAQMJHY2dXH3MXIxSEksIJRYv+tHewgCSGBL4wSD5blQSQ+A9nP7jHDdOy4vg2q YzmjxMPus1Adbxklztz27GLk4BAWiJa4cZcTJCwiYCex8/UbRhCbWUBA4vG9Z6wgNpuArcSR mV/BbF4BN4mOew/ZQGwWAVWJ3V/fsIKMERWIlZh+LQSiRFDi5MwnLCA2p4CVxJRj06BGyks0 b53NDGGLSNx41MIIcpqEQDu7xKuXX1kgbnaROL3gNDuELSzx6vgWKFtG4vTkHhaIhm2MEld/ /4Tq3s8ocb13BVSVtcThf7/ZQC5iFtCUWL9LH8SUEHCUuHopHMLkk7jxVhDiBj6JSdumM0OE eSU62oQgZuhJPO2Zygiz9c/aJywTGJVmIflsFpJvZiH5ZhbC2gWMLKsYxVNLi3PTU4uN81LL 9YoTc4tL89L1kvNzNzECU8jpf8e/7mDc9yfpEKMAB6MSD6/C+qlxQqyJZcWVuYcYJTiYlUR4 1+cChXhTEiurUovy44tKc1KLDzFKc7AoifMaL3oZKySQnliSmp2aWpBaBJNl4uCUamC8sfFd 0vqU5q4Lav3nrzXl/TBQXlx3StCbLyaXu37/t4hEC96a44tljrxrl1RfkSLSzszDe1Bi1zb+ 3b8risz2TK8qkswMc753b+/WWOaSS0UbY81bnGyqshIirnhN8lDnlJQ2OTB59pUZVSfcUj23 Tfsd7c2zsCvYRvtGr/Ih+bPr5z6a9EaJpTgj0VCLuag4EQDrVvcwHQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprHIsWRmVeSWpSXmKPExsVy+t/xe7o6CdPiDI7stbTom/SRyeLdp+1M FqfWfWd3YPb4tWApq8fpn0+YPRbveckUwBylZ1OUX1qSqpCRX1xiqxRtaGGkZ2hpoWdkYqln aGwea2VkqqRvZ5OSmpNZllqkb5egl3Fgyw72go96FXsmrmNtYPyu0sXIySEhYCKx4/o25i5G Lg4hgaWMEj/+7GXvYuQASshIfLgkAFEjLPHnWhcbRM1rRok5C26wgNQIC0RL3LjLCVIjImAn sfP1G0YQm1lAQOLxvWesEPW9jBL/G1eDJdgEbCWOzPzKCmLzCrhJdNx7yAZiswioSuz++oYV ZKaoQKxEy0VNiBJBiZMzn7CA2JwCVhJTjk2Dmm8mMW/zQ2YIW16ieetsKFtE4sajFsYJjEKz kLTPQtIyC0nLLCQtCxhZVjGKpJYW56bnFhvpFSfmFpfmpesl5+duYgTGzbZjP7fsYOx6F3yI UYCDUYmH98SaqXFCrIllxZW5hxglOJiVRHjX5wKFeFMSK6tSi/Lji0pzUosPMZoC/TaRWUo0 OR8Y03kl8YamhuYWlobmxubGZhZK4rwdAgdjhATSE0tSs1NTC1KLYPqYODilGhjDjr5+ksHd udnH5f0EufK3D+oXirR/c3davWIjj7vmyyyFY9rRJd9PRIW+cPDTXM3W4JS1UmTBFSm77++/ hy109/7s6DQ3SvOW+tfTsfLzuDse7UphOan/81z69Icpq3RD2zbGMP0+m/nVXobj12PO69pF Z2bk3kg3jzfYvvkG69To0P8FdoFKLMUZiYZazEXFiQCdcc1HsQIAAA== X-CMS-MailID: 20200415011525eucas1p1869aa77acb6adfa08fb5fed9f45a8094 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20200413092039eucas1p1372014e18d014115634fc8eb23508004 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20200413092039eucas1p1372014e18d014115634fc8eb23508004 References: <20200413091906.782-1-sarosh.arif@emumba.com> Subject: Re: [dpdk-dev] [PATCH] test_distributor.c: prevent memory leakages from the pool X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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< + 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