From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A9C4343FD1; Wed, 8 May 2024 09:23:52 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 712754361D; Wed, 8 May 2024 09:23:52 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id D901E40A6E for ; Wed, 8 May 2024 09:23:50 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 91ABE91B8 for ; Wed, 8 May 2024 09:23:50 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 8263D9220; Wed, 8 May 2024 09:23:50 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.3 required=5.0 tests=ALL_TRUSTED,AWL, T_SCC_BODY_TEXT_LINE autolearn=disabled version=4.0.0 X-Spam-Score: -1.3 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 956F19171; Wed, 8 May 2024 09:23:48 +0200 (CEST) Message-ID: Date: Wed, 8 May 2024 09:23:48 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC v2] net/af_packet: make stats reset reliable To: =?UTF-8?Q?Morten_Br=C3=B8rup?= , Ferruh Yigit , "John W. Linville" Cc: Thomas Monjalon , dev@dpdk.org, =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , Stephen Hemminger References: <20240425174617.2126159-1-ferruh.yigit@amd.com> <20240426143848.2280689-1-ferruh.yigit@amd.com> <108e0c40-33e7-4eed-83de-eaedee454480@lysator.liu.se> <238675d1-b0bb-41df-8338-a1052c1a88c1@lysator.liu.se> <98CBD80474FA8B44BF855DF32C47DC35E9F425@smartserver.smartshare.dk> <75008697-147b-4045-ad3c-a7d3cc2017f1@lysator.liu.se> <98CBD80474FA8B44BF855DF32C47DC35E9F427@smartserver.smartshare.dk> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F427@smartserver.smartshare.dk> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 2024-05-08 09:10, Morten Brørup wrote: >> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se] >> Sent: Wednesday, 8 May 2024 08.35 >> >> On 2024-05-07 21:19, Morten Brørup wrote: >>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se] >>>> Sent: Tuesday, 7 May 2024 09.24 >>>> >>>> On 2024-04-28 17:11, Mattias Rönnblom wrote: >>>>> On 2024-04-26 16:38, Ferruh Yigit wrote: >>> >>> [...] >>> >>>>> static uint64_t >>>>> counter_value(struct counter *counter) >>>>> { >>>>>     uint64_t count = __atomic_load_n(&counter->count, >>>> __ATOMIC_RELAXED); >>>>>     uint64_t offset = __atomic_load_n(&counter->offset, >>>> __ATOMIC_RELAXED); >>>>> >>>> >>>> Since the count and the offset are written to independently, without any >>>> ordering restrictions, an update and a reset in quick succession may >>>> cause the offset store to be globally visible before the new count. >>> >>> Good catch. >>> This may happen when a thread calls stats_add() and then the same thread >> calls stats_reset(). >>> >>>> In such a scenario, a reader could see an offset > count. >>>> >>>> Thus, unless I'm missing something, one should add a >>>> >>>> if (unlikely(offset > count)) >>>> return 0; >>>> >>>> here. With the appropriate comment explaining why this might be. >>>> >>>> Another approach would be to think about what memory barriers may be >>>> required to make sure one sees the count update before the offset >>>> update, but, intuitively, that seems like both more complex and more >>>> costly (performance-wise). >>> >>> I think it can be done without affecting stats_add(), by using "offset" with >> Release-Consume ordering: >>> - stats_reset() must write "offset" with memory_order_release, so >> "counter" cannot be visible after it, and >>> - stats_get() must read "offset" with memory_order_consume, so no reads or >> writes in the current thread dependent on "offset" can be reordered before >> this load, and writes to "counter" (a data-dependent variable) in other >> threads that release "offset" are visible in the current thread. >>> >> >> That was the kind of complexity I was thinking about. Those barriers >> come with a non-zero cost, both with different instructions being used >> and compiler optimizations being prevented. > > Yep, you mentioned that there might be a more complex alternative, so I decided to explore it. :-) > > This approach doesn't impose any new requirements on stats_add(), so the data plane performance is not affected. > OK, I get it now. That's a good point. In my thought experiment, I had a thread both updating and resetting the counter, which should be allowed, but you could have barriers sit only in the reset routine. > For per-thread counters, stats_add() can store "counter" using memory_order_relaxed. Or, if the architecture prevents tearing of 64-bit variables, using volatile. > > Counters shared by multiple threads must be atomically incremented using rte_atomic_fetch_add_explicit() with memory_order_relaxed. > >> >>>> >>>>>     return count + offset; >>>>> } >>>>> >>>>> static void >>>>> counter_reset(struct counter *counter) >>>>> { >>>>>     uint64_t count = __atomic_load_n(&counter->count, >>>> __ATOMIC_RELAXED); >>>>> >>>>>     __atomic_store_n(&counter->offset, count, __ATOMIC_RELAXED); >>>>> } >>>>> >>>>> static void >>>>> counter_add(struct counter *counter, uint64_t operand) >>>>> { >>>>>     __atomic_store_n(&counter->count, counter->count + operand, >>>>> __ATOMIC_RELAXED); >>>>> } >>>