From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <dev-bounces@dpdk.org> Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0B1D743FE3; Wed, 8 May 2024 17:23:49 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D5D6740A6E; Wed, 8 May 2024 17:23:48 +0200 (CEST) Received: from mail-pl1-f174.google.com (mail-pl1-f174.google.com [209.85.214.174]) by mails.dpdk.org (Postfix) with ESMTP id 5B5D040042 for <dev@dpdk.org>; Wed, 8 May 2024 17:23:47 +0200 (CEST) Received: by mail-pl1-f174.google.com with SMTP id d9443c01a7336-1ec4dc64c6cso31803185ad.0 for <dev@dpdk.org>; Wed, 08 May 2024 08:23:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1715181826; x=1715786626; 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=tOQHz/TplD7ozpb5DJhji7wgGinUmCp8QgqZT2IfU7k=; b=ka0aORwkFGuJFB2rDgehvFMIQWYbMIPsYwDpdIWfq/hSm1ylEv2uad/JshHamuW/rJ kBTyJ3t83/Sn1oTineeDhlSGrJ7Xmpwtu6l7PY1iCYoqJ/qItQYddz4pO0f9d3p2V5bA dDefKHFISKKbL1J+xXVYf7ufr+bNl+oOp51YE8arCydClZFdnBORvSZjmg9PcHDIDfeU 4d5FmkiPzNNL7tqfYM1ru0S0SZmzHrtc6St7oD09ClHMXtYVveWiup6hFYGp3UN1Pk+t uFWYRv9f5jo4blBBNPfIqNU9wYrMuMJwV24/TmF46KKR/J66hNsRQX3/HoFu7Aa8TiI5 QaUg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715181826; x=1715786626; 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=tOQHz/TplD7ozpb5DJhji7wgGinUmCp8QgqZT2IfU7k=; b=vrOvbcWsGrq/vz1Mi+hpIHdSdpGFZaSbVIt6yvUKoZONqzkbBIg+G67ShCnsjtRMki 4696B51NzZT1QbmOEH4f6d4oq6Y9xzO4MvkONOY0F0G5xishnhS3p6sTbL6HU2UXcnFt i3Rrmx9YEQpRW8NQiuZRWQ3qh+ABmA0FOhuZB0/haJvsKNnF60R5WVBLWLCNQna3AzE+ 9eSysbcQPPUIv8YpH8VZFzBmoLcw23sDYvvf+IN1u6ganycOt9eWcWSWjpHK0B3feku+ NjdEANOKkHqUPUqVYD+eRMk58KeVIiqECeEcIafdp/2uGkkrK+yjGU9ljXU0NfGflZJu DcIQ== X-Forwarded-Encrypted: i=1; AJvYcCXd28vmvG9wJucUdkV8oGNt4T3d9rWL5nUDnC922DdjrVFPf6BX6KuMbZ2YsYoZ7NcR1AgFtJy3KzchWDM= X-Gm-Message-State: AOJu0YwWSqAs6Pa89ai5pSZVCFFS7ocluAtLsa6edmez6aXQILxPwAUD St9i7gT1x2n1UqTWpB3u+lZ52GAYiho6/j+1rZRNRPqDihE9uxvvSk7Cm/VTlzs= X-Google-Smtp-Source: AGHT+IFiOLxTK2iLWcqXT6eZzhzi2StTxP1YRisJQa0EClHYfoKEfGS7GROAc0qjLSj5CKUAyIq6Aw== X-Received: by 2002:a17:90a:9605:b0:2b2:97b4:d3b8 with SMTP id 98e67ed59e1d1-2b616be48b3mr2753930a91.43.1715181826251; Wed, 08 May 2024 08:23:46 -0700 (PDT) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2b62863a5fbsm1586850a91.4.2024.05.08.08.23.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 08 May 2024 08:23:42 -0700 (PDT) Date: Wed, 8 May 2024 08:23:39 -0700 From: Stephen Hemminger <stephen@networkplumber.org> To: Mattias =?UTF-8?B?UsO2bm5ibG9t?= <hofors@lysator.liu.se> Cc: Ferruh Yigit <ferruh.yigit@amd.com>, "John W. Linville" <linville@tuxdriver.com>, Thomas Monjalon <thomas@monjalon.net>, dev@dpdk.org, Mattias =?UTF-8?B?UsO2bm5ibG9t?= <mattias.ronnblom@ericsson.com>, Morten =?UTF-8?B?QnLDuHJ1cA==?= <mb@smartsharesystems.com> Subject: Re: [RFC v3] net/af_packet: make stats reset reliable Message-ID: <20240508082339.6bf74202@hermes.local> In-Reply-To: <432fb280-96e3-4079-b987-990d509b8c79@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> 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 <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org On Wed, 8 May 2024 09:19:02 +0200 Mattias R=C3=B6nnblom <hofors@lysator.liu.se> wrote: > On 2024-05-04 00:00, Stephen Hemminger wrote: > > On Fri, 3 May 2024 16:45:47 +0100 > > Ferruh Yigit <ferruh.yigit@amd.com> wrote: > > =20 > >> For stats reset, use an offset instead of zeroing out actual stats val= ues, > >> get_stats() displays diff between stats and offset. > >> This way stats only updated in datapath and offset only updated in sta= ts > >> reset function. This makes stats reset function more reliable. > >> > >> As stats only written by single thread, we can remove 'volatile' quali= fier > >> which should improve the performance in datapath. > >> > >> While updating around, 'igb_stats' parameter renamed as 'stats'. > >> > >> Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com> > >> --- > >> Cc: Mattias R=C3=B6nnblom <mattias.ronnblom@ericsson.com> > >> Cc: Stephen Hemminger <stephen@networkplumber.org> > >> Cc: Morten Br=C3=B8rup <mb@smartsharesystems.com> > >> > >> This update triggered by mail list discussion [1]. > >> > >> [1] > >> https://inbox.dpdk.org/dev/3b2cf48e-2293-4226-b6cd-5f4dd3969f99@lysato= r.liu.se/ =20 > >=20 > >=20 > > NAK > >=20 > > I did not hear a good argument why atomic or volatile was necessary in = the first place. > > Why? > > =20 >=20 > On the reader side, loads should be atomic. > On the writer side, stores should be atomic. >=20 > Updates (stores) should actually occur in a timely manner. The complete=20 > read-modify-write cycle need not be atomic, since we only have a single=20 > writer. All this for the per-lcore counter case. >=20 > If load or store tearing occurs, the counter values may occasionally=20 > take totally bogus values. I think that should be avoided. Especially=20 > since it will likely come at a very reasonable cost. >=20 > From what it seems to me, load or store tearing may well occur. GCC may= =20 > generate two 32-bit stores for a program-level 64-bit store on 32-bit=20 > x86. If you have constant and immediate-data store instructions,=20 > constant writes may also be end up teared. The kernel documentation has=20 > some example of this. Add LTO, it's not necessarily going to be all that= =20 > clear what is storing-a-constant and what is not. >=20 > Maybe you care a little less if statistics are occasionally broken, or=20 > some transient, inconsistent state, but generally they should work, and=20 > they should never have some totally bogus values. So, statistics aren't=20 > snow flakes, mostly just business as usual. >=20 > We can't both have a culture that promotes C11-style parallel=20 > programming, or, at the extreme, push the C11 APIs as-is, and the say=20 > "and btw you don't have to care about the standard when it comes to=20 > statistics". >=20 > We could adopt the Linux kernel's rules, programming model, and APIs=20 > (ignoring legal issues). That would be very old school, maybe somewhat=20 > over-engineered for our purpose, include a fair amount of inline=20 > assembler, and also and may well depend on GCC or GCC-like compilers,=20 > just like what I believe the kernel does. >=20 > We could use something in-between, heavily inspired by C11 but still=20 > with an opportunity to work around compiler issues, library issues, and > extend the API for our use case. >=20 > I agree we shouldn't have to mark statistics _Atomic, or RTE_ATOMIC(),=20 > rte_atomic64_t, or rte_sometimes_atomic_and_sometimes_not64_t. Just=20 > 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 driv= ers doing software > > statistics (tap, virtio, xdp, ring, memif, netvsc, vmware)? =20 >=20 > If a broken piece of code has been copied around, one place is going to=20 > be the first to be fixed. I dislike when any driver does something completely different than valid pr= ecedent. No other driver in DPDK, Vpp, FreeBSD, Linux (and probably Windows) uses at= omic for updating statistics. We even got performance benefit at MS from removing at= omic increment of staistics in internal layers. The idea of load tearing is crazy talk of integral types. It would break 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 The DPDK has always favored performance over strict safety guard rails ever= ywhere. Switching to making every statistic an atomic operation is not in the spiri= t of what is required. There is no strict guarantee necessary here.