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 1A8B2A04BB for ; Thu, 17 Sep 2020 13:50:18 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 880BC1D643; Thu, 17 Sep 2020 13:50:17 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id D0DE31D62A; Thu, 17 Sep 2020 13:50:12 +0200 (CEST) IronPort-SDR: YKNBI3OKOw76M9hIB3+1E+VmRvWIPAcF65KZSdTBFWiGiltrCv8xnrR9Yyht9NFObd3kj4U/e6 ZIcM7ttffRuw== X-IronPort-AV: E=McAfee;i="6000,8403,9746"; a="244514103" X-IronPort-AV: E=Sophos;i="5.76,436,1592895600"; d="scan'208";a="244514103" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Sep 2020 04:50:11 -0700 IronPort-SDR: SnlN6zAaOyzXRdjOOE/Pb/xRWxcEI2CqmIzThwUaatVDYzvK1LcvVEpoNU0nLOn8KXPDu8q9mY s5jiGmtejTRQ== X-IronPort-AV: E=Sophos;i="5.76,436,1592895600"; d="scan'208";a="483716285" Received: from dhunt5-mobl5.ger.corp.intel.com (HELO [10.213.228.49]) ([10.213.228.49]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Sep 2020 04:50:10 -0700 To: Lukasz Wojciechowski , Bruce Richardson Cc: dev@dpdk.org, stable@dpdk.org References: <20200915193449.13310-1-l.wojciechow@partner.samsung.com> <20200915193449.13310-3-l.wojciechow@partner.samsung.com> From: David Hunt Message-ID: Date: Thu, 17 Sep 2020 12:50:08 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20200915193449.13310-3-l.wojciechow@partner.samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB Subject: Re: [dpdk-stable] [PATCH v1 2/6] app/test: synchronize statistics between lcores 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" On 15/9/2020 8:34 PM, Lukasz Wojciechowski wrote: > 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. > > Fixes: c3eabff124e6 ("distributor: add unit tests") > Cc: bruce.richardson@intel.com > Cc: stable@dpdk.org > > Signed-off-by: Lukasz Wojciechowski > --- > 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); > 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); > } > > /* 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); > 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); > 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)); > 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)); > 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)); > 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); > 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); > 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. " Thanks. Acked-by: David Hunt