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 E59E343FD1; Wed, 8 May 2024 09:19:07 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A89F340A6E; Wed, 8 May 2024 09:19:07 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 2C148402AF for ; Wed, 8 May 2024 09:19:06 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 811BD915F for ; Wed, 8 May 2024 09:19:05 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 740D0919D; Wed, 8 May 2024 09:19:05 +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 A376D9199; Wed, 8 May 2024 09:19:02 +0200 (CEST) Message-ID: <432fb280-96e3-4079-b987-990d509b8c79@lysator.liu.se> Date: Wed, 8 May 2024 09:19:02 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC v3] net/af_packet: make stats reset reliable To: Stephen Hemminger , Ferruh Yigit Cc: "John W. Linville" , Thomas Monjalon , dev@dpdk.org, =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , =?UTF-8?Q?Morten_Br=C3=B8rup?= References: <20240425174617.2126159-1-ferruh.yigit@amd.com> <20240503154547.392069-1-ferruh.yigit@amd.com> <20240503150011.55681b97@hermes.local> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <20240503150011.55681b97@hermes.local> 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-04 00:00, Stephen Hemminger wrote: > On Fri, 3 May 2024 16:45:47 +0100 > Ferruh Yigit wrote: > >> For stats reset, use an offset instead of zeroing out actual stats values, >> get_stats() displays diff between stats and offset. >> This way stats only updated in datapath and offset only updated in stats >> reset function. This makes stats reset function more reliable. >> >> As stats only written by single thread, we can remove 'volatile' qualifier >> which should improve the performance in datapath. >> >> While updating around, 'igb_stats' parameter renamed as 'stats'. >> >> Signed-off-by: Ferruh Yigit >> --- >> Cc: Mattias Rönnblom >> Cc: Stephen Hemminger >> Cc: Morten Brørup >> >> This update triggered by mail list discussion [1]. >> >> [1] >> https://inbox.dpdk.org/dev/3b2cf48e-2293-4226-b6cd-5f4dd3969f99@lysator.liu.se/ > > > NAK > > I did not hear a good argument why atomic or volatile was necessary in the first place. > Why? > On the reader side, loads should be atomic. On the writer side, stores should be atomic. Updates (stores) should actually occur in a timely manner. The complete read-modify-write cycle need not be atomic, since we only have a single writer. All this for the per-lcore counter case. If load or store tearing occurs, the counter values may occasionally take totally bogus values. I think that should be avoided. Especially since it will likely come at a very reasonable cost. From what it seems to me, load or store tearing may well occur. GCC may generate two 32-bit stores for a program-level 64-bit store on 32-bit x86. If you have constant and immediate-data store instructions, constant writes may also be end up teared. The kernel documentation has some example of this. Add LTO, it's not necessarily going to be all that clear what is storing-a-constant and what is not. Maybe you care a little less if statistics are occasionally broken, or some transient, inconsistent state, but generally they should work, and they should never have some totally bogus values. So, statistics aren't snow flakes, mostly just business as usual. We can't both have a culture that promotes C11-style parallel programming, or, at the extreme, push the C11 APIs as-is, and the say "and btw you don't have to care about the standard when it comes to statistics". We could adopt the Linux kernel's rules, programming model, and APIs (ignoring legal issues). That would be very old school, maybe somewhat over-engineered for our purpose, include a fair amount of inline assembler, and also and may well depend on GCC or GCC-like compilers, just like what I believe the kernel does. We could use something in-between, heavily inspired by C11 but still with an opportunity to work around compiler issues, library issues, and extend the API for our use case. I agree we shouldn't have to mark statistics _Atomic, or RTE_ATOMIC(), rte_atomic64_t, or rte_sometimes_atomic_and_sometimes_not64_t. Just keeping the usual C integer types seems like a better option to me. > Why is this driver special (a snowflake) compared to all the other drivers doing software > statistics (tap, virtio, xdp, ring, memif, netvsc, vmware)? If a broken piece of code has been copied around, one place is going to be the first to be fixed.