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 B1A3BA04B5 for ; Fri, 2 Oct 2020 13:25:27 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6C75D1D575; Fri, 2 Oct 2020 13:25:26 +0200 (CEST) Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 0B7AA1D575 for ; Fri, 2 Oct 2020 13:25:25 +0200 (CEST) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20201002112523euoutp028775d10f8ea7df0070c92c478b8963ed~6KTSO1W9H1763417634euoutp02I for ; Fri, 2 Oct 2020 11:25:23 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20201002112523euoutp028775d10f8ea7df0070c92c478b8963ed~6KTSO1W9H1763417634euoutp02I DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1601637923; bh=ijwyVsBbUTSFV7h2y2wr6qxoL2NQuAGuHORd2SAKddY=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=RDmIK7Xmgx6EXrdNzJg3+aTFJ7XejeCSQxAAm6SGRO3YzgXWH6ZebbTY5qwUpVMbs UU2czYTd5L954G5Xz/SCufoSj2ZmLJG4V/K5/zxAbYNGWOUsqMIWS2IdNFGjiHKbDk rt/+4vYa30+RzJFaU/peZXyD6rjR5TNwFkpxEC8s= Received: from eusmges1new.samsung.com (unknown [203.254.199.242]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20201002112522eucas1p2d0ae19dd5271d4b1b100533c25e96c39~6KTR-Tkiy2555825558eucas1p2x; Fri, 2 Oct 2020 11:25:22 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges1new.samsung.com (EUCPMTA) with SMTP id CB.09.06456.22E077F5; Fri, 2 Oct 2020 12:25:22 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20201002112522eucas1p1b26d12e062435e55191337ddd3ded4a3~6KTRokGwi1083910839eucas1p1p; Fri, 2 Oct 2020 11:25:22 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20201002112522eusmtrp1eaa7efa5125a61c5f63995b218b85da7~6KTRn8JEa1259212592eusmtrp1c; Fri, 2 Oct 2020 11:25:22 +0000 (GMT) X-AuditID: cbfec7f2-7efff70000001938-66-5f770e227f8d Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id DA.38.06017.22E077F5; Fri, 2 Oct 2020 12:25:22 +0100 (BST) Received: from [106.210.88.70] (unknown [106.210.88.70]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20201002112521eusmtip1a1de6b646d26ca13bb330ed89374bd46~6KTQscTcv0824708247eusmtip1G; Fri, 2 Oct 2020 11:25:21 +0000 (GMT) To: Honnappa Nagarahalli , David Hunt , Bruce Richardson Cc: "dev@dpdk.org" , "stable@dpdk.org" , nd , "\"'Lukasz Wojciechowski'\"," From: Lukasz Wojciechowski Message-ID: Date: Fri, 2 Oct 2020 13:25:19 +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+NgFlrOKsWRmVeSWpSXmKPExsWy7djP87pKfOXxBvuuGFvcWGVv0TfpI5PF u0/bmSxmPm1ht3jWs47R4szyHmaLfx1/2B3YPdbMW8Po8WvBUlaPxXteMnkcfLeHKYAlissm JTUnsyy1SN8ugSvj2uTrrAXnvSrm7NvJ1sC41LqLkZNDQsBE4unNDcxdjFwcQgIrGCWer1nO DuF8YZSYs+4sK4TzmVFiS/sdNpiWGSfXQCWWM0rcfT6BCSQhJPCWUWLerNIuRg4OYYEIiekn 1EFqRAS6GSUmnT3DCOIwC8xllNg5rZERpIFNwFbiyMyvrCA2r4CbxM75b9hBbBYBFYnjM9vA bFGBOIljpx6xQNQISpyc+QTM5hSIlVh1dTrYRcwC8hLNW2czQ9jiEreezGcCWSYhsIld4t/2 68wQZ7tInO/dAmULS7w6voUdwpaROD25hwWiYRujxNXfPxkhnP2MEtd7V0BVWUsc/vebDeQ3 ZgFNifW79CHCjhKH/rxgBglLCPBJ3HgrCHEEn8SkbdOhwrwSHW1CENV6Ek97pjLCrP2z9gnL BEalWUhem4XknVlI3pmFsHcBI8sqRvHU0uLc9NRiw7zUcr3ixNzi0rx0veT83E2MwORz+t/x TzsYv15KOsQowMGoxMObcag0Xog1say4MvcQowQHs5IIr9PZ03FCvCmJlVWpRfnxRaU5qcWH GKU5WJTEeY0XvYwVEkhPLEnNTk0tSC2CyTJxcEo1MK5ftn2GqO4Wa1enxmw1R62fHUWLzq5Z OjEkoHj526zoI3mO33t/TFP739/3xsNu9Y8lb9dKTyhOVzUNP8s4Y+2J7DIle+ONYicWKbMs q4u8dVZfM+dJc+KJ+teMrQE8nJzmMg/e3FH2Vs2pYFj82nNpT1WU0ZxlS2db/uhyk963fFnc rjtaKUosxRmJhlrMRcWJAEQ1G9Y6AwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrJIsWRmVeSWpSXmKPExsVy+t/xu7pKfOXxBosvS1ncWGVv0TfpI5PF u0/bmSxmPm1ht3jWs47R4szyHmaLfx1/2B3YPdbMW8Po8WvBUlaPxXteMnkcfLeHKYAlSs+m KL+0JFUhI7+4xFYp2tDCSM/Q0kLPyMRSz9DYPNbKyFRJ384mJTUnsyy1SN8uQS/j2uTrrAXn vSrm7NvJ1sC41LqLkZNDQsBEYsbJNaxdjFwcQgJLGSXuNjcxdzFyACVkJD5cEoCoEZb4c62L DaLmNaPEhJdrGUFqhAUiJKafUAeJiwj0Mkps75rLBOIwC8xllJja+JQRouMJk8Tc7z2MIKPY BGwljsz8ygpi8wq4Seyc/4YdxGYRUJE4PrMNzBYViJM40/OCDaJGUOLkzCcsIDanQKzEqqvT weLMAmYS8zY/ZIaw5SWat86GssUlbj2ZzzSBUWgWkvZZSFpmIWmZhaRlASPLKkaR1NLi3PTc YiO94sTc4tK8dL3k/NxNjMBo23bs55YdjF3vgg8xCnAwKvHwChwojRdiTSwrrsw9xCjBwawk wut09nScEG9KYmVValF+fFFpTmrxIUZToOcmMkuJJucDE0FeSbyhqaG5haWhubG5sZmFkjhv h8DBGCGB9MSS1OzU1ILUIpg+Jg5OqQbG9skvzmVGXhOUsE+SZUx7Mcv4R/y8mccn666YctTq RcEvHrVNMUkMuT88PT/rns7g137wdnr/sc8LPgY4WCW/0e64ff/+ZHdNNk5vwfRV2ktrVihs N5vazc3kzNri/jvvnpzf6wTxz+lPSpU+bGE8ee616xIvWe893lP2PpXZu2v/MwPBN1vuKLEU ZyQaajEXFScCAPwEyjrMAgAA X-CMS-MailID: 20201002112522eucas1p1b26d12e062435e55191337ddd3ded4a3 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" 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/KeLBd2EJLOU?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. 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! 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