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 0E8DFA04C0; Sat, 26 Sep 2020 00:45:27 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9E8441EA7A; Sat, 26 Sep 2020 00:42:42 +0200 (CEST) Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id B77B91E9F9 for ; Sat, 26 Sep 2020 00:42:27 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20200925224226euoutp0209185b6b58b2c961acc7cf5f34445971~4KBbysU_t2413424134euoutp02E for ; Fri, 25 Sep 2020 22:42:26 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20200925224226euoutp0209185b6b58b2c961acc7cf5f34445971~4KBbysU_t2413424134euoutp02E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1601073746; bh=Jby1MCwoHT+l7PQMNJVAse/QenN2t9wRrmRskM0ygNc=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=Ywb0VoHEyLDIICdyAy5sBEAS+6fbV8RLGFewMQrLbiky+hRjhYAR4y3fJ2xwMZk9Z Ps1N346nnEMA1Coyh+db7WKs0y31AjilNgxHqx2vbJL/JfsD4pWjH6HbJhDUM4NC8Q s7TcSYNrsGP5pWOkllPV/3D/wlDQo0Vwhx731VOE= Received: from eusmges1new.samsung.com (unknown [203.254.199.242]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20200925224225eucas1p2bf66db801de2d02d3de879771c589c66~4KBaywOCh0960309603eucas1p2p; Fri, 25 Sep 2020 22:42:25 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges1new.samsung.com (EUCPMTA) with SMTP id 0C.6B.06456.1527E6F5; Fri, 25 Sep 2020 23:42:25 +0100 (BST) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20200925224224eucas1p12f91b77b1b2fc84a30e8bc04dd154075~4KBZ1UTF62520225202eucas1p1l; Fri, 25 Sep 2020 22:42:24 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20200925224224eusmtrp28577732ad7760832a664496e15e4e33c~4KBZ0zcYg0483804838eusmtrp2L; Fri, 25 Sep 2020 22:42:24 +0000 (GMT) X-AuditID: cbfec7f2-7efff70000001938-05-5f6e7251af08 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id B0.CE.06017.0527E6F5; Fri, 25 Sep 2020 23:42:24 +0100 (BST) Received: from [106.210.88.70] (unknown [106.210.88.70]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20200925224223eusmtip248cc283d7b37878b4cf8c2c6c0ebbcb6~4KBZX6Bn41081510815eusmtip26; Fri, 25 Sep 2020 22:42:23 +0000 (GMT) To: David Marchand Cc: dev , David Hunt , "\"'Lukasz Wojciechowski'\"," From: Lukasz Wojciechowski Message-ID: <531988f8-7208-68dd-785d-b9c1d76b6333@partner.samsung.com> Date: Sat, 26 Sep 2020 00:42:22 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Transfer-Encoding: 8bit Content-Language: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprBKsWRmVeSWpSXmKPExsWy7djP87qBRXnxBnOeiFv0TfrIZLF9RReb xbtP25ksnvWsY3Rg8fi1YCmrx+I9L5k8Dr7bw+Txft9VtgCWKC6blNSczLLUIn27BK6MV9+O MxZ8tK7oPyzSwHhZvYuRk0NCwETi7/anzF2MXBxCAisYJSZP2sEE4XxhlJj/8RxU5jOjxIze C8wwLb/uHWCDSCxnlPh6ay4LhPOWUeLXqh1sIFXCAv4SB7ZdYAexRQT0JCZu28QCYjML1Eoc /rcfLM4mYCtxZOZXVhCbV8BNYvOXH2A1LAKqEvOnHAabIyoQJ3Hs1CMWiBpBiZMzn4DZnAKB El/vrWeGmCkv0bx1NpQtLnHryXywHyQEJrNLdE7/wAhxtovEg6Z/bBC2sMSr41vYIWwZif87 YRq2MUpc/f2TEcLZzyhxvXcFVJU10Nm/gbo5gFZoSqzfpQ8RdpSY8/suWFhCgE/ixltBiCP4 JCZtm84MEeaV6GgTgqjWk3jaM5URZu2ftU9YJjAqzULy2iwk78xC8s4shL0LGFlWMYqnlhbn pqcWG+allusVJ+YWl+al6yXn525iBKaX0/+Of9rB+PVS0iFGAQ5GJR7eE49y44VYE8uKK3MP MUpwMCuJ8DqdPR0nxJuSWFmVWpQfX1Sak1p8iFGag0VJnNd40ctYIYH0xJLU7NTUgtQimCwT B6dUA2PQ9pAAjr2xzZmhzq/Mlimrsvx+nmxc/e7MotDzYdZbYs+/lv9jf+qgp3b56tCHx1Je yTlwtLjUxGeqTn5sOuvuf2Or+XXSnhkM67R2bI5Ld7ubPtN754I9ZTs+z1vWv+qzR2VS3Q4H lTfsG176mX/eV2cYx90s+rllkt/aM7OypW5UM/yIeKTEUpyRaKjFXFScCAAv0MSHKwMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrBIsWRmVeSWpSXmKPExsVy+t/xe7oBRXnxBlt/Slr0TfrIZLF9RReb xbtP25ksnvWsY3Rg8fi1YCmrx+I9L5k8Dr7bw+Txft9VtgCWKD2bovzSklSFjPziElulaEML Iz1DSws9IxNLPUNj81grI1MlfTublNSczLLUIn27BL2MV9+OMxZ8tK7oPyzSwHhZvYuRk0NC wETi170DbF2MXBxCAksZJb6+vMzexcgBlJCR+HBJAKJGWOLPtS6omteMEo+6+5hBaoQFfCXa GytAakQE9CQmbtvEAmIzC9RKbH7RxgxRP4dJomvlRzaQBJuArcSRmV9ZQWxeATeJzV9+gDWw CKhKzJ9yGKxGVCBO4kzPCzaIGkGJkzOfgNVwCgRKfL23nhligZnEvM0PoWx5ieats6FscYlb T+YzTWAUmoWkfRaSlllIWmYhaVnAyLKKUSS1tDg3PbfYSK84Mbe4NC9dLzk/dxMjMJa2Hfu5 ZQdj17vgQ4wCHIxKPLwKT3PjhVgTy4orcw8xSnAwK4nwOp09HSfEm5JYWZValB9fVJqTWnyI 0RTouYnMUqLJ+cA4zyuJNzQ1NLewNDQ3Njc2s1AS5+0QOBgjJJCeWJKanZpakFoE08fEwSnV wHjpTdt+49MTtt5yPbxoi0TatbVaMZx/zmUp1indlIq68T9X+KFy/D6G55OLzI0ZFa9tWfLA hDXR5o+V7Bw+GW6dsoL8Sh6Trosvlz6dM3vlxkthVsnN8kKsQeuWbVFgaNzlUqK4IaV0/6rX E5+/KpzIvP7Dt+Lkg5N+nGENfTc396rKh+PJNiuUWIozEg21mIuKEwG9aRKJuwIAAA== X-CMS-MailID: 20200925224224eucas1p12f91b77b1b2fc84a30e8bc04dd154075 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20200923132544eucas1p29470697e7cb6621cc65e6e676c3e5d69 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20200923132544eucas1p29470697e7cb6621cc65e6e676c3e5d69 References: <20200923014713.16932-1-l.wojciechow@partner.samsung.com> <20200923132541.21417-1-l.wojciechow@partner.samsung.com> Subject: Re: [dpdk-dev] [PATCH v3 0/8] fix distributor synchronization issues 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" Hello David, Thank you for your review. W dniu 25.09.2020 o 14:31, David Marchand pisze: > Hello Lukasz, > > On Wed, Sep 23, 2020 at 3:25 PM Lukasz Wojciechowski > wrote: >> During review and verification of the patch created by Sarosh Arif: >> "test_distributor: prevent memory leakages from the pool" I found out >> that running distributor unit tests multiple times in a row causes fails. >> So I investigated all the issues I found. >> >> There are few synchronization issues that might cause deadlocks >> or corrupted data. They are fixed with this set of patches for both tests >> and librte_distributor library. >> >> --- >> v3: >> * add missing acked and tested by statements from v1 >> >> v2: >> * assign NULL to freed mbufs in distributor test >> * fix handshake check on legacy single distributor >> rte_distributor_return_pkt_single() >> * add patch 7 passing NULL to legacy API calls if no bufs are returned >> * add patch 8 fixing API documentation >> >> Lukasz Wojciechowski (8): >> app/test: fix deadlock in distributor test >> app/test: synchronize statistics between lcores >> app/test: fix freeing mbufs in distributor tests >> app/test: collect return mbufs in distributor test > For these patches, we can use the "test/distributor: " prefix, and we > then avoid repeating "in distributor test" Changed > >> distributor: fix missing handshake synchronization >> distributor: fix handshake deadlock >> distributor: do not use oldpkt when not needed >> distributor: align API documentation with code > Thanks for working on those fixes ! > > Here is a suggestion: > > - we can move this new patch 7 before patch 3 in the series, and > update the unit test: > * by passing NULL to the first call to rte_distributor_get_pkt(), > there is no need for buf[] array init in handle_work(), > handle_work_with_free_mbufs() and handle_work_for_shutdown_test(), > * at all points of those functions the buf[] array then contains only > [0, num[ valid entries, > * bonus point, this makes the UT check passing NULL oldpkt, > > - the former patch 3 is then easier to do since there is no need for > buf[] array clearing, > > This gives the following diff, wdyt? I reorder patches as you suggested. I added unit test changes in the same patch that changes distributor lib. The changes follow your diff. This also simplified "fix freeing mbufs" patch. It's applied in v4. > > diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c > index f31b54edf3..b7ab93ecbe 100644 > --- a/app/test/test_distributor.c > +++ b/app/test/test_distributor.c > @@ -65,13 +65,10 @@ handle_work(void *arg) > struct rte_mbuf *buf[8] __rte_cache_aligned; > struct worker_params *wp = arg; > struct rte_distributor *db = wp->dist; > - unsigned int count = 0, num = 0; > + unsigned int count = 0, num; > unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED); > - int i; > > - for (i = 0; i < 8; i++) > - buf[i] = NULL; > - num = rte_distributor_get_pkt(db, id, buf, buf, num); > + num = rte_distributor_get_pkt(db, id, buf, NULL, 0); > while (!quit) { > __atomic_fetch_add(&worker_stats[id].handled_packets, num, > __ATOMIC_ACQ_REL); > @@ -277,22 +274,17 @@ handle_work_with_free_mbufs(void *arg) > struct rte_mbuf *buf[8] __rte_cache_aligned; > struct worker_params *wp = arg; > struct rte_distributor *d = wp->dist; > + unsigned int num; > unsigned int i; > - unsigned int num = 0; > unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED); > > - for (i = 0; i < 8; i++) > - buf[i] = NULL; > - num = rte_distributor_get_pkt(d, id, buf, buf, 0); > + num = rte_distributor_get_pkt(d, id, buf, NULL, 0); > while (!quit) { > __atomic_fetch_add(&worker_stats[id].handled_packets, num, > __ATOMIC_ACQ_REL); > - for (i = 0; i < num; i++) { > + for (i = 0; i < num; i++) > rte_pktmbuf_free(buf[i]); > - buf[i] = NULL; > - } > - num = rte_distributor_get_pkt(d, > - id, buf, buf, 0); > + num = rte_distributor_get_pkt(d, id, buf, NULL, 0); > } > __atomic_fetch_add(&worker_stats[id].handled_packets, num, > __ATOMIC_ACQ_REL); > @@ -347,15 +339,13 @@ handle_work_for_shutdown_test(void *arg) > struct rte_mbuf *buf[8] __rte_cache_aligned; > struct worker_params *wp = arg; > struct rte_distributor *d = wp->dist; > - unsigned int num = 0; > + unsigned int num; > unsigned int i; > unsigned int zero_id = 0; > const unsigned int id = __atomic_fetch_add(&worker_idx, 1, > __ATOMIC_RELAXED); > - for (i = 0; i < 8; i++) > - buf[i] = NULL; > > - num = rte_distributor_get_pkt(d, id, buf, buf, 0); > + num = rte_distributor_get_pkt(d, id, buf, NULL, 0); > > zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE); > if (id == zero_id && num > 0) { > @@ -369,12 +359,9 @@ handle_work_for_shutdown_test(void *arg) > while (!quit && !(id == zero_id && zero_quit)) { > __atomic_fetch_add(&worker_stats[id].handled_packets, num, > __ATOMIC_ACQ_REL); > - for (i = 0; i < num; i++) { > + for (i = 0; i < num; i++) > rte_pktmbuf_free(buf[i]); > - buf[i] = NULL; > - } > - num = rte_distributor_get_pkt(d, > - id, buf, buf, 0); > + num = rte_distributor_get_pkt(d, id, buf, NULL, 0); > > zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE); > if (id == zero_id && num > 0) { > @@ -392,21 +379,16 @@ handle_work_for_shutdown_test(void *arg) > while (zero_quit) > usleep(100); > > - for (i = 0; i < num; i++) { > + for (i = 0; i < num; i++) > rte_pktmbuf_free(buf[i]); > - buf[i] = NULL; > - } > - num = rte_distributor_get_pkt(d, > - id, buf, buf, 0); > + num = rte_distributor_get_pkt(d, id, buf, NULL, 0); > > while (!quit) { > __atomic_fetch_add(&worker_stats[id].handled_packets, > num, __ATOMIC_ACQ_REL); > - for (i = 0; i < num; i++) { > + for (i = 0; i < num; i++) > rte_pktmbuf_free(buf[i]); > - buf[i] = NULL; > - } > - num = rte_distributor_get_pkt(d, id, buf, buf, 0); > + num = rte_distributor_get_pkt(d, id, buf, NULL, 0); > } > } > rte_distributor_return_pkt(d, id, buf, num); > > -- Lukasz Wojciechowski Principal Software Engineer Samsung R&D Institute Poland Samsung Electronics Office +48 22 377 88 25 l.wojciechow@partner.samsung.com