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 68A7445A7C; Fri, 4 Oct 2024 19:40:51 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5A58542DA3; Fri, 4 Oct 2024 19:40:51 +0200 (CEST) Received: from mail-pj1-f45.google.com (mail-pj1-f45.google.com [209.85.216.45]) by mails.dpdk.org (Postfix) with ESMTP id 47F9B402E4 for ; Fri, 4 Oct 2024 19:40:50 +0200 (CEST) Received: by mail-pj1-f45.google.com with SMTP id 98e67ed59e1d1-2e09ff2d890so2079905a91.0 for ; Fri, 04 Oct 2024 10:40:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1728063649; x=1728668449; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=lt/mt+F+3d5L/9ccMMEcwXwCqnxo5/eI7ld7RLM266c=; b=k3YqI/HRwg9xf4Tdt6Ei8sZT3AOyhhiept7mFYB5gOmjWV80TTrcJuxEhPRPz5df6F 4s0cHaGfN0pd3EHie0/iR/ZnzO7eQM6R8d+RB33nhnSzMXB/pxD+tS0JT8C8EGp43rKy t5qEEkCldohyWlvzA+7Rux8+ew+uEQcleMgyCR/n4uGoft+/L/AsEPpRYD4dPYY423cz BMLyldLsV2RSyqwYulEK+873HvMKlk9CtMNPr2yEMH91L1dB2uhe9PoLVbPTookVuCJ4 rQpfpEQeQmWDbz1rXXwPCCP5xtd2uWSjEw/ndtR8e6WgXtBzJL+0IEifNKYqFwazZVWM nqiw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728063649; x=1728668449; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=lt/mt+F+3d5L/9ccMMEcwXwCqnxo5/eI7ld7RLM266c=; b=fildR2RXcnozqu0se8+w54HL7lmBHyug45eLooHkZ2cb9+zcs/+b7DOrY8pFROuC7Y J75GTybbgoVkV5jMF4geug7ntdUATsvJIcLno0S8CjTQnR1vXFTQYSiPnyFLVQ+JfH6G QnNPq52hpcE4AKBKAkMXtLOoDEOEQ7aLo4kpVh5XnhIfT1lemI5TAwM4GT5wEkAL/+yV f964qSRZYpowGBgoNYs90IjW6F+C8RRIzrQ9FJusWG+IxbdbCepNIX0J3drfrCWVvgLv alcQn1ZurkPMhH32SX+fIxhx58/wi2UJa/7+Bcwt5EyhtUPvE5a8QnbnjfWtx4DVNnoR XkIg== X-Forwarded-Encrypted: i=1; AJvYcCWW17e/Wv9sWg6JX7loDd/LRBqpHGIUfvkgpaKZAx5TPOtVUERiw01HnRVFTwngtDw6+Lc=@dpdk.org X-Gm-Message-State: AOJu0YzveeIBcwWoZMkWSnBC/6Pr5fNc+IrbzThBK3J5Cfjwl1atVShp CMbtbYt8ow8S0mn2j1cBsdF/mDOXZNi7+pNlDNkc6yHodESWrDbr2vF/deIuKR8= X-Google-Smtp-Source: AGHT+IGr9CMdC6214ZwIi25utbS3kut8XVSsY5lnQPv2eNGYfwYAL6Z/FDPqP/oddBXQcZaZXBz6xg== X-Received: by 2002:a17:90a:db8b:b0:2e0:f81c:7313 with SMTP id 98e67ed59e1d1-2e1e631e48amr4474142a91.27.1728063649403; Fri, 04 Oct 2024 10:40:49 -0700 (PDT) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2e1e85c8f76sm1932472a91.16.2024.10.04.10.40.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 04 Oct 2024 10:40:49 -0700 (PDT) Date: Fri, 4 Oct 2024 10:40:47 -0700 From: Stephen Hemminger To: Mattias =?UTF-8?B?UsO2bm5ibG9t?= Cc: Ferruh Yigit , "John W. Linville" , Thomas Monjalon , dev@dpdk.org, Mattias =?UTF-8?B?UsO2bm5ibG9t?= , Morten =?UTF-8?B?QnLDuHJ1cA==?= Subject: Re: [RFC v3] net/af_packet: make stats reset reliable Message-ID: <20241004104047.375187be@hermes.local> In-Reply-To: <58916959-5633-40a9-b5f6-8b6b17cd3c21@lysator.liu.se> References: <20240425174617.2126159-1-ferruh.yigit@amd.com> <20240503154547.392069-1-ferruh.yigit@amd.com> <20240503150011.55681b97@hermes.local> <432fb280-96e3-4079-b987-990d509b8c79@lysator.liu.se> <20240508082339.6bf74202@hermes.local> <58916959-5633-40a9-b5f6-8b6b17cd3c21@lysator.liu.se> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 Sun, 26 May 2024 09:21:55 +0200 Mattias R=C3=B6nnblom wrote: > On 2024-05-08 17:23, Stephen Hemminger wrote: > > On Wed, 8 May 2024 09:19:02 +0200 > > Mattias R=C3=B6nnblom wrote: > > =20 > >> On 2024-05-04 00:00, Stephen Hemminger wrote: =20 > >>> On Fri, 3 May 2024 16:45:47 +0100 > >>> Ferruh Yigit wrote: > >>> =20 > >>>> For stats reset, use an offset instead of zeroing out actual stats v= alues, > >>>> get_stats() displays diff between stats and offset. > >>>> This way stats only updated in datapath and offset only updated in s= tats > >>>> reset function. This makes stats reset function more reliable. > >>>> > >>>> As stats only written by single thread, we can remove 'volatile' qua= lifier > >>>> which should improve the performance in datapath. > >>>> > >>>> While updating around, 'igb_stats' parameter renamed as 'stats'. > >>>> > >>>> Signed-off-by: Ferruh Yigit > >>>> --- > >>>> Cc: Mattias R=C3=B6nnblom > >>>> Cc: Stephen Hemminger > >>>> Cc: Morten Br=C3=B8rup > >>>> > >>>> This update triggered by mail list discussion [1]. > >>>> > >>>> [1] > >>>> https://inbox.dpdk.org/dev/3b2cf48e-2293-4226-b6cd-5f4dd3969f99@lysa= tor.liu.se/ =20 > >>> > >>> > >>> NAK > >>> > >>> I did not hear a good argument why atomic or volatile was necessary i= n the first place. > >>> Why? > >>> =20 > >> > >> 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 th= at > >> 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. > >> =20 > >>> Why is this driver special (a snowflake) compared to all the other dr= ivers doing software > >>> statistics (tap, virtio, xdp, ring, memif, netvsc, vmware)? =20 > >> > >> If a broken piece of code has been copied around, one place is going to > >> be the first to be fixed. =20 > >=20 > >=20 > > I dislike when any driver does something completely different than vali= d precedent. > > No other driver in DPDK, Vpp, FreeBSD, Linux (and probably Windows) use= s atomic for > > updating statistics. We even got performance benefit at MS from removin= g atomic > > increment of staistics in internal layers. > > =20 >=20 > All of those are using atomic stores when updating the statistics, I'm=20 > sure. Assuring a store being atomic is one thing, and assuring the whole= =20 > read-modify-write cycle is atomic is a completely different (and very=20 > much more expensive) thing. >=20 > > The idea of load tearing is crazy talk of integral types. It would brea= k so many things. > > It is the kind of stupid compiler thing that would send Linus on a rant= and get > > the GCC compiler writers in trouble. > > =20 >=20 > On 32-bit x86, store tearing for 64-bit integral types is the order of=20 > the day. >=20 > For <64 bit types, I agree. The only cases (like the one listed in the=20 > kernel documentation), are going to be rare indeed. >=20 > > The DPDK has always favored performance over strict safety guard rails = everywhere. > > Switching to making every statistic an atomic operation is not in the s= pirit of > > what is required. There is no strict guarantee necessary here. > > =20 >=20 > I think we agree but just use different terminology. >=20 Don't think any real conclusion was reached on this. Af_packet is a less used driver, more concerned about virtio and xdp driver= s. Marking this patch as changes requested, since still under discussion.