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 7D125A04DB for ; Fri, 16 Oct 2020 14:59:03 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 487BC1EDF2; Fri, 16 Oct 2020 14:59:02 +0200 (CEST) Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id B63D81EB4A for ; Fri, 16 Oct 2020 14:58:58 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20201016125847euoutp0274d51e214cab372a82cad61063cb048c~_em1Nh4Nm2819728197euoutp02L for ; Fri, 16 Oct 2020 12:58:47 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20201016125847euoutp0274d51e214cab372a82cad61063cb048c~_em1Nh4Nm2819728197euoutp02L DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1602853127; bh=a5K7lQbUXdYvhxApAiJEEeaHYHU3l/B4iC8TDn341ww=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=eqlReoCY6y4/PPnraX6FDeeT6Mpi0TQpV7AP8+La0NQo+MHoaoke8nkrASuhj4WHZ O/o9bJj7r4xpAwirohyi8xrxUtXWMUDIfPEKec8IFA/Y/ht9qeb15H1Zph5evk2A2m +wOvoAvdD3fg2rjwFqkHYNUzet+sis3uvlyzFOJU= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20201016125842eucas1p2de1738cb51dd81e226ed5a7686c6d50a~_emwW0Puk1215712157eucas1p2J; Fri, 16 Oct 2020 12:58:42 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id A9.2A.05997.109998F5; Fri, 16 Oct 2020 13:58:41 +0100 (BST) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20201016125841eucas1p1cada287a758d4e93be568f49bec0df07~_emv-UhO_0433404334eucas1p1A; Fri, 16 Oct 2020 12:58:41 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20201016125841eusmtrp251c1c63f66ef1bbf69ce65daf5d0f1d7~_emv_vA7Z2182421824eusmtrp2d; Fri, 16 Oct 2020 12:58:41 +0000 (GMT) X-AuditID: cbfec7f4-65dff7000000176d-9e-5f8999018ecc Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id 41.4C.06017.109998F5; Fri, 16 Oct 2020 13:58:41 +0100 (BST) Received: from [106.210.88.70] (unknown [106.210.88.70]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20201016125840eusmtip1f54255389c54d967cb16fe7c0d0362d1~_emvHXWUR2567725677eusmtip15; Fri, 16 Oct 2020 12:58:40 +0000 (GMT) To: Honnappa Nagarahalli , David Hunt , Bruce Richardson Cc: "dev@dpdk.org" , "stable@dpdk.org" , nd , David Marchand , "\"'Lukasz Wojciechowski'\"," From: Lukasz Wojciechowski Message-ID: <572a9ba1-3141-ff11-b283-74a81c7ddc89@partner.samsung.com> Date: Fri, 16 Oct 2020 14:58:39 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.1 MIME-Version: 1.0 In-Reply-To: Content-Transfer-Encoding: 8bit Content-Language: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA01Sa0hTYRjm29nOjtLsOBVfzBQGIkrOrIgTdlGLGhghhSRZzqkH7xd21FIh hcTmnGKLsE0qaYUmXjB0mqi1KamoC39YKpZQbqmklRqhG5rHo+S/532e5708Hx+BiVsFXkRq Vi6tzFJkSHBnvvH9uiUI6crlRz9WnKImG89RVdrfPKqzQY1TyyudPEpnLRVSNk0LokbrNRi1 qXIIwwhZ09MmJNuoeymQGXoWeDLTcg9P9rNvAo8S3HA+nURnpObTyuCz8c4pJpsB5Rhj7/T2 GYQlaEamRgQB5Am4b09XI2dCTDYgsJU7cDVy2i7WEEyUSTlhFUGtyoKxAtvQvKjDOaEewWjp BuKKJQT1/T/47Fg3MgZqhvxZ3p2sQKAdG90xYeQwAp1lHLGjcPIMDOj+CFgsIi/CA9PjnWY+ 6QeNtkCW9iDjoLqtlM9ZXGFYN7eDnchL0NWm3RmDkb5wr6MW47AnTM8947G7gDQLoUKjx7mz L8B06+oudoPFwXYhh71h681eg3E7s30dccVbBJ8qG3ZdodC/acfZ6zAyAFq7gzk6HMyOeYx7 SBeYXHLljnABrbFmlxaBqkzMuaVg1TxCe2sdzXP8aiTR74um3xdHvy+O/v/eOsRvRJ50HpOZ TDPHsujbUkaRyeRlJUsTszNfo+1vNLI5uNaFuh0JZkQSSHJApLqmkosFinymINOMgMAk7qKI sZE4sShJUVBIK7PlyrwMmjGjQwRf4ik6/nzhlphMVuTS6TSdQyv3VB7h5FWCog4HWK9WQ3Rx L1gjDNMtK3+7Zn+lUeZ+0VRsTGV70vjdg6RbWAJz+coL7Um7vM3nSFGnPTrXNLTlN6NNn7WK 1GlVIWo80rW510Nz0/Y5zifX9ET3MA4alr9fn38nHjkfNOAfXxxe2pto/PIhdKrDu7Dvm7TW UvS13PdVX1CkhM+kKEICMSWj+Aesy9pcQgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrIIsWRmVeSWpSXmKPExsVy+t/xu7qMMzvjDXb8VrC4screom/SRyaL 7Su62CzefdrOZDHzaQu7xbOedYwWZ5b3MFv86/jD7sDhsWbeGkaPXwuWsnos3vOSyePguz1M Hu/3XWULYI3SsynKLy1JVcjILy6xVYo2tDDSM7S00DMysdQzNDaPtTIyVdK3s0lJzcksSy3S t0vQyzj4bDFjwbboir37FrM3MN7x6GLk5JAQMJFY+2omWxcjF4eQwFJGid3f9jB1MXIAJWQk PlwSgKgRlvhzrYsNxBYSeM0osfZuJkiJsECExPQT6iCtIgK9jBLbu+YygTjMAicZJba9a2OC GLqRRWJR531mkG42AVuJIzO/soLYvAJuEhMPzmABmcQioCqx6pkWSFhUIE7ix8ReNogSQYmT M5+wgNicAu4SOzZOYgSxmQXMJOZtfsgMYctLNG+dDWWLS9x6Mp9pAqPQLCTts5C0zELSMgtJ ywJGllWMIqmlxbnpucVGesWJucWleel6yfm5mxiB0bft2M8tOxi73gUfYhTgYFTi4W3w6YgX Yk0sK67MPcQowcGsJMLrdPZ0nBBvSmJlVWpRfnxRaU5q8SFGU6DfJjJLiSbnAxNDXkm8oamh uYWlobmxubGZhZI4b4fAwRghgfTEktTs1NSC1CKYPiYOTqkGxgrTWwUbchNd5vB7vY68u+kD +yoT5de95fzzrsS28mTtuuGZnbBBm73nzZ9pYu7zYwNe6pWfWCPVWV0w+7ytjFHFp/qqNVWC 9ektWt6lYSey1V7zP5IIXpNlnZoedPOH+jzFPzZ8a5I/9RkHiMqyBzQvLevqPx3+2bPe88Ph wMt5bQd6Hu5UYinOSDTUYi4qTgQArnbPFtQCAAA= X-CMS-MailID: 20201016125841eucas1p1cada287a758d4e93be568f49bec0df07 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20200925224217eucas1p1bb5f73109b4aeed8f2badf311fa8dfb5 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20200925224217eucas1p1bb5f73109b4aeed8f2badf311fa8dfb5 References: <20200923132541.21417-1-l.wojciechow@partner.samsung.com> <20200925224209.12173-1-l.wojciechow@partner.samsung.com> <20200925224209.12173-3-l.wojciechow@partner.samsung.com> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4 2/8] test/distributor: synchronize lcores statistics X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" W dniu 16.10.2020 o 14:43, Lukasz Wojciechowski pisze: > Hi Honnappa, > > Thank you for your answer. > In the current v7 version I followed your advise and used RELAXED memory model. > And it works without any issues. I guess after fixing other issues found since v4 the distributor works more stable. > I didn't have time to rearrange all tests in the way I proposed, but I guess if they work like this it's not a top priority. > > Can you give an ack on the series? I believe David Marchand is waiting for your opinion to process it. I'm sorry I didn't see your other comments. I'll try to fix them them today. > > Best regards > Lukasz > > W dniu 16.10.2020 o 07:43, Honnappa Nagarahalli pisze: >> >> >>> Hi Honnappa, >>> >>> Many thanks for the review! >>> >>> I'll write my answers here not inline as it would be easier to read them in one >>> place, I think. >>> So first of all I agree with you in 2 things: >>> 1) all uses of statistics must be atomic and lack of that caused most of the >>> problems >>> 2) it would be better to replace barrier and memset in >>> clear_packet_count() with atomic stores as you suggested >>> >>> So I will apply both of above. >>> >>> However I wasn't not fully convinced on changing acquire/release to relaxed. >>> It wood be perfectly ok if it would look like in this Herb Sutter's example: >>> https://youtu.be/KeLBd2[] EJLOU?t=4170 >>> But in his case the counters are cleared before worker threads start and are >>> printout after they are completed. >>> >>> In case of the dpdk distributor tests both worker and main cores are running >>> at the same time. In the sanity_test, the statistics are cleared and verified >>> few times for different hashes of packages. The worker cores are not >>> stopped at this time and they continue their loops in handle procedure. >>> Verification made in main core is an exchange of data as the current statistics >>> indicate how the test will result. >> Agree. The key point we have to note is that the data that is exchanged between the two threads is already atomic (handled_packets is atomic). >> >>> So as I wasn't convinced, I run some tests with both both relaxed and >>> acquire/release modes and they both fail :( The failures caused by statistics >>> errors to number of tests ratio for >>> 200000 tests was: >>> for relaxed: 0,000790562 >>> for acq/rel: 0,000091321 >>> >>> >>> That's why I'm going to modify tests in such way, that they would: >>> 1) clear statistics >>> 2) launch worker threads >>> 3) run test >>> 4) wait for workers procedures to complete >>> 5) check stats, verify results and print them out >>> >>> This way worker main core will use (clear or verify) stats only when there are >>> no worker threads. This would make things simpler and allowing to focus on >>> testing the distributor not tests. And of course relaxed mode would be >>> enough! >> Agree, this would be the only way to ensure that the main thread sees the correct statistics (just like in the video) >> >>> Best regards >>> Lukasz >>> >>> >>> W dniu 29.09.2020 o 07:49, Honnappa Nagarahalli pisze: >>>> >>>> >>>>> Statistics of handled packets are cleared and read on main lcore, >>>>> while they are increased in workers handlers on different lcores. >>>>> >>>>> Without synchronization occasionally showed invalid values. >>>>> This patch uses atomic acquire/release mechanisms to synchronize. >>>> In general, load-acquire and store-release memory orderings are required >>> while synchronizing data (that cannot be updated atomically) between >>> threads. In the situation, making counters atomic is enough. >>>>> Fixes: c3eabff124e6 ("distributor: add unit tests") >>>>> Cc: bruce.richardson@intel.com >>>>> Cc: stable@dpdk.org >>>>> >>>>> Signed-off-by: Lukasz Wojciechowski >>>>> >>>>> Acked-by: David Hunt >>>>> --- >>>>> app/test/test_distributor.c | 39 ++++++++++++++++++++++++----------- >>> -- >>>>> 1 file changed, 26 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/app/test/test_distributor.c >>>>> b/app/test/test_distributor.c index >>>>> 35b25463a..0e49e3714 100644 >>>>> --- a/app/test/test_distributor.c >>>>> +++ b/app/test/test_distributor.c >>>>> @@ -43,7 +43,8 @@ total_packet_count(void) { >>>>> unsigned i, count = 0; >>>>> for (i = 0; i < worker_idx; i++) >>>>> - count += worker_stats[i].handled_packets; >>>>> + count += >>>>> __atomic_load_n(&worker_stats[i].handled_packets, >>>>> + __ATOMIC_ACQUIRE); >>>> RELAXED memory order is sufficient. For ex: the worker threads are not >>> 'releasing' any data that is not atomically updated to the main thread. >>>>> return count; >>>>> } >>>>> >>>>> @@ -52,6 +53,7 @@ static inline void >>>>> clear_packet_count(void) >>>>> { >>>>> memset(&worker_stats, 0, sizeof(worker_stats)); >>>>> + rte_atomic_thread_fence(__ATOMIC_RELEASE); >>>> Ideally, the counters should be set to 0 atomically rather than using a >>> memset. >>>>> } >>>>> >>>>> /* this is the basic worker function for sanity test @@ -72,13 >>>>> +74,13 @@ handle_work(void *arg) >>>>> num = rte_distributor_get_pkt(db, id, buf, buf, num); >>>>> while (!quit) { >>>>> __atomic_fetch_add(&worker_stats[id].handled_packets, >>>>> num, >>>>> - __ATOMIC_RELAXED); >>>>> + __ATOMIC_ACQ_REL); >>>> Using the __ATOMIC_ACQ_REL order does not mean anything to the main >>> thread. The main thread might still see the updates from different threads in >>> different order. >>>>> count += num; >>>>> num = rte_distributor_get_pkt(db, id, >>>>> buf, buf, num); >>>>> } >>>>> __atomic_fetch_add(&worker_stats[id].handled_packets, num, >>>>> - __ATOMIC_RELAXED); >>>>> + __ATOMIC_ACQ_REL); >>>> Same here, do not see why this change is required. >>>> >>>>> count += num; >>>>> rte_distributor_return_pkt(db, id, buf, num); >>>>> return 0; >>>>> @@ -134,7 +136,8 @@ sanity_test(struct worker_params *wp, struct >>>>> rte_mempool *p) >>>>> >>>>> for (i = 0; i < rte_lcore_count() - 1; i++) >>>>> printf("Worker %u handled %u packets\n", i, >>>>> - worker_stats[i].handled_packets); >>>>> + __atomic_load_n(&worker_stats[i].handled_packets, >>>>> + __ATOMIC_ACQUIRE)); >>>> __ATOMIC_RELAXED is enough. >>>> >>>>> printf("Sanity test with all zero hashes done.\n"); >>>>> >>>>> /* pick two flows and check they go correctly */ @@ -159,7 +162,9 >>>>> @@ sanity_test(struct worker_params *wp, struct rte_mempool *p) >>>>> >>>>> for (i = 0; i < rte_lcore_count() - 1; i++) >>>>> printf("Worker %u handled %u packets\n", i, >>>>> - worker_stats[i].handled_packets); >>>>> + __atomic_load_n( >>>>> + &worker_stats[i].handled_packets, >>>>> + __ATOMIC_ACQUIRE)); >>>> __ATOMIC_RELAXED is enough >>>> >>>>> printf("Sanity test with two hash values done\n"); >>>>> } >>>>> >>>>> @@ -185,7 +190,8 @@ sanity_test(struct worker_params *wp, struct >>>>> rte_mempool *p) >>>>> >>>>> for (i = 0; i < rte_lcore_count() - 1; i++) >>>>> printf("Worker %u handled %u packets\n", i, >>>>> - worker_stats[i].handled_packets); >>>>> + __atomic_load_n(&worker_stats[i].handled_packets, >>>>> + __ATOMIC_ACQUIRE)); >>>> __ATOMIC_RELAXED is enough >>>> >>>>> printf("Sanity test with non-zero hashes done\n"); >>>>> >>>>> rte_mempool_put_bulk(p, (void *)bufs, BURST); @@ -280,15 >>>>> +286,17 @@ handle_work_with_free_mbufs(void *arg) >>>>> buf[i] = NULL; >>>>> num = rte_distributor_get_pkt(d, id, buf, buf, num); >>>>> while (!quit) { >>>>> - worker_stats[id].handled_packets += num; >>>>> count += num; >>>>> + __atomic_fetch_add(&worker_stats[id].handled_packets, >>>>> num, >>>>> + __ATOMIC_ACQ_REL); >>>> IMO, the problem would be the non-atomic update of the statistics. So, >>>> __ATOMIC_RELAXED is enough >>>> >>>>> for (i = 0; i < num; i++) >>>>> rte_pktmbuf_free(buf[i]); >>>>> num = rte_distributor_get_pkt(d, >>>>> id, buf, buf, num); >>>>> } >>>>> - worker_stats[id].handled_packets += num; >>>>> count += num; >>>>> + __atomic_fetch_add(&worker_stats[id].handled_packets, num, >>>>> + __ATOMIC_ACQ_REL); >>>> Same here, the problem is non-atomic update of the statistics, >>> __ATOMIC_RELAXED is enough. >>>> Similarly, for changes below, __ATOMIC_RELAXED is enough. >>>> >>>>> rte_distributor_return_pkt(d, id, buf, num); >>>>> return 0; >>>>> } >>>>> @@ -363,8 +371,9 @@ handle_work_for_shutdown_test(void *arg) >>>>> /* wait for quit single globally, or for worker zero, wait >>>>> * for zero_quit */ >>>>> while (!quit && !(id == zero_id && zero_quit)) { >>>>> - worker_stats[id].handled_packets += num; >>>>> count += num; >>>>> + __atomic_fetch_add(&worker_stats[id].handled_packets, >>>>> num, >>>>> + __ATOMIC_ACQ_REL); >>>>> for (i = 0; i < num; i++) >>>>> rte_pktmbuf_free(buf[i]); >>>>> num = rte_distributor_get_pkt(d, >>>>> @@ -379,10 +388,11 @@ handle_work_for_shutdown_test(void *arg) >>>>> >>>>> total += num; >>>>> } >>>>> - worker_stats[id].handled_packets += num; >>>>> count += num; >>>>> returned = rte_distributor_return_pkt(d, id, buf, num); >>>>> >>>>> + __atomic_fetch_add(&worker_stats[id].handled_packets, num, >>>>> + __ATOMIC_ACQ_REL); >>>>> if (id == zero_id) { >>>>> /* for worker zero, allow it to restart to pick up last packet >>>>> * when all workers are shutting down. >>>>> @@ -394,10 +404,11 @@ handle_work_for_shutdown_test(void *arg) >>>>> id, buf, buf, num); >>>>> >>>>> while (!quit) { >>>>> - worker_stats[id].handled_packets += num; >>>>> count += num; >>>>> rte_pktmbuf_free(pkt); >>>>> num = rte_distributor_get_pkt(d, id, buf, buf, num); >>>>> + >>>>> __atomic_fetch_add(&worker_stats[id].handled_packets, >>>>> + num, __ATOMIC_ACQ_REL); >>>>> } >>>>> returned = rte_distributor_return_pkt(d, >>>>> id, buf, num); >>>>> @@ -461,7 +472,8 @@ sanity_test_with_worker_shutdown(struct >>>>> worker_params *wp, >>>>> >>>>> for (i = 0; i < rte_lcore_count() - 1; i++) >>>>> printf("Worker %u handled %u packets\n", i, >>>>> - worker_stats[i].handled_packets); >>>>> + __atomic_load_n(&worker_stats[i].handled_packets, >>>>> + __ATOMIC_ACQUIRE)); >>>>> >>>>> if (total_packet_count() != BURST * 2) { >>>>> printf("Line %d: Error, not all packets flushed. " >>>>> @@ -514,7 +526,8 @@ test_flush_with_worker_shutdown(struct >>>>> worker_params *wp, >>>>> zero_quit = 0; >>>>> for (i = 0; i < rte_lcore_count() - 1; i++) >>>>> printf("Worker %u handled %u packets\n", i, >>>>> - worker_stats[i].handled_packets); >>>>> + __atomic_load_n(&worker_stats[i].handled_packets, >>>>> + __ATOMIC_ACQUIRE)); >>>>> >>>>> if (total_packet_count() != BURST) { >>>>> printf("Line %d: Error, not all packets flushed. " >>>>> -- >>>>> 2.17.1 >>> -- >>> Lukasz Wojciechowski >>> Principal Software Engineer >>> >>> Samsung R&D Institute Poland >>> Samsung Electronics >>> Office +48 22 377 88 25 >>> l.wojciechow@partner.samsung.com -- Lukasz Wojciechowski Principal Software Engineer Samsung R&D Institute Poland Samsung Electronics Office +48 22 377 88 25 l.wojciechow@partner.samsung.com