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 8203A43FD1; Wed, 8 May 2024 08:34:52 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6114740A6E; Wed, 8 May 2024 08:34: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 E8329402AF for ; Wed, 8 May 2024 08:34:50 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id AD4698F39 for ; Wed, 8 May 2024 08:34:50 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id A1B288E60; Wed, 8 May 2024 08:34: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 36CA08ECA; Wed, 8 May 2024 08:34:49 +0200 (CEST) Message-ID: <75008697-147b-4045-ad3c-a7d3cc2017f1@lysator.liu.se> Date: Wed, 8 May 2024 08:34: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> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F425@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-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. >> >>>     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); >>> } >