From: "Mattias Rönnblom" <hofors@lysator.liu.se>
To: Stephen Hemminger <stephen@networkplumber.org>,
Ferruh Yigit <ferruh.yigit@amd.com>
Cc: dev@dpdk.org, "John W. Linville" <linville@tuxdriver.com>,
"Morten Brørup" <mb@smartsharesystems.com>
Subject: Re: [PATCH] net/af_packet: fix statistics
Date: Thu, 2 May 2024 19:57:55 +0200 [thread overview]
Message-ID: <d4e5cea1-5067-4b1d-98e3-ebe8bd1f3772@lysator.liu.se> (raw)
In-Reply-To: <20240502091658.2d51ca75@hermes.local>
On 2024-05-02 18:16, Stephen Hemminger wrote:
> On Thu, 2 May 2024 15:12:43 +0100
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
>> I am not referring multiple core sharing a queue, this is wrong for DPDK.
>>
>> For single core case, if a variable doesn't have 'volatile' qualifier
>> and load/stores are not atomic, as compiler is not aware that there may
>> be other threads will access this value, what prevents compiler to hold
>> stats value in a register and store it memory at later stage, let say at
>> the end of the application forwarding loop?
>
> No other driver does this; it is not a problem since once rx/tx burst
> function returns, there is no state the compiler is aware of.
>
>
>> For this case keeping 'volatile' qualifier works. But Mattias has a
>> suggestion to use "normal load + normal add + atomic store" in datapath.
>>
>>
>> For getting stats, which will be in different thread, you are already
>> casting it to 'volatile' to enforce compiler read from memory each time.
>>
>>
>> Additionally when we take stats reset into account, which can happen in
>> different thread, 'volatile' is not enough.
>> For reliable stats reset, either we need to use full atomics, or offset
>> logic which I am trying to in other patch.
>
> If you want to attack the problem in the general case, there should be
> standard set of functions for handling per-core stats. If you look at
> Linux kernel you will see macros around this function:
>
> netdev_core_stats_inc((struct net_device *dev, u32 offset) {
> struct net_device_core_stats __percpu *p = READ_ONCE(dev->core_stats);
> unsigned long __percpu *field;
> ...
> field = (__force unsigned long __percpu *)((__force void *)p + offset);
> this_cpu_inc(*field);
> }
>
> The expansion of this_cpu_inc is nested but ends up being:
> *field += 1;
>
> No volatile is needed.
>
> The conclusion I reach from this is:
> - atomic is not needed for per-cpu data. See "volatile considered harmful"
> using a form of compiler barrier is useful but should be hidden in generic code.
> - what is the state of the better per-lcore patches?
> that should be used here.
> - need a more generic percpu stats infrastructure
> - ethedev needs to have a generic percpu infra and not reinvented in xdp, tap, packet, etc.
>
>
In this case, the counters are already effectively "per-lcore", since a
single RX/TX queue pair is not MT safe, and thus is only used by one
thread (or at least, by one thread at a time).
My impression is that both the Linux kernel and DPDK tend to assume that
no load or store tearing occurs for accesses to small, naturally
aligned, integer types. I'm guessing it's also commonly assumed there
are limits to how large section of the whole program the compiler may
reason about, to eliminate data and code/instructions that have no
effect on that thread. Thus, volatile are not needed to assure that for
example counter state is actually maintained, and updates actually occurs.
I will continue work on the lcore variables patch set as soon as time
permits. That will be after my attempts to complete the bitops and the
bitset patch sets have concluded.
next prev parent reply other threads:[~2024-05-02 17:57 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 15:39 Stephen Hemminger
2024-05-01 16:25 ` Ferruh Yigit
2024-05-01 16:42 ` Stephen Hemminger
2024-05-02 13:48 ` Ferruh Yigit
2024-05-01 16:43 ` Stephen Hemminger
2024-05-02 14:12 ` Ferruh Yigit
2024-05-02 16:16 ` Stephen Hemminger
2024-05-02 17:57 ` Mattias Rönnblom [this message]
2024-05-01 16:44 ` Stephen Hemminger
2024-05-01 18:18 ` Morten Brørup
2024-05-02 13:47 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d4e5cea1-5067-4b1d-98e3-ebe8bd1f3772@lysator.liu.se \
--to=hofors@lysator.liu.se \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=linville@tuxdriver.com \
--cc=mb@smartsharesystems.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).