From: Stephen Hemminger <stephen@networkplumber.org>
To: Ferruh Yigit <ferruh.yigit@amd.com>
Cc: dev@dpdk.org, "John W. Linville" <linville@tuxdriver.com>,
"Mattias Rönnblom" <hofors@lysator.liu.se>,
"Morten Brørup" <mb@smartsharesystems.com>
Subject: Re: [PATCH] net/af_packet: fix statistics
Date: Thu, 2 May 2024 09:16:58 -0700 [thread overview]
Message-ID: <20240502091658.2d51ca75@hermes.local> (raw)
In-Reply-To: <b802ad44-a705-4a1b-82ee-92e5d0b9e7d8@amd.com>
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.
next prev parent reply other threads:[~2024-05-02 16:17 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 [this message]
2024-05-02 17:57 ` Mattias Rönnblom
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=20240502091658.2d51ca75@hermes.local \
--to=stephen@networkplumber.org \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=hofors@lysator.liu.se \
--cc=linville@tuxdriver.com \
--cc=mb@smartsharesystems.com \
/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).