Re: [PATCH v2 net-next 00/26] net: introduce and use generic XDP stats

From: Jakub Kicinski <kuba@kernel.org>
Date: Tue, 30 Nov 2021 08:12:07 -0800

On Tue, 30 Nov 2021 16:56:12 +0100 Alexander Lobakin wrote:

quoted

3. XDP and XSK ctrs separately or not.

My PoV is that those are two quite different worlds.
However, stats for actions on XSK really make a little sense since
99% of time we have xskmap redirect. So I think it'd be fine to just
expand stats structure with xsk_{rx,tx}_{packets,bytes} and count
the rest (actions, errors) together with XDP.


Rest:
 - don't create a separate `ip` command and report under `-s`;
 - save some RTNL skb space by skipping zeroed counters.

Let me ruin this point of clarity for you. I think that stats should 
be skipped when they are not collected (see ETHTOOL_STAT_NOT_SET).
If messages get large user should use the GETSTATS call and avoid 
the problem more effectively.

Well, it was Dave's thought here: [0]

Another thought on this patch: with individual attributes you could save
some overhead by not sending 0 counters to userspace. e.g., define a
helper that does:

I know about ETHTOOL_STAT_NOT_SET, but RTNL xstats doesn't use this,
does it?
GETSTATS is another thing, and I'll use it, thanks.

quoted

Also, regarding that I count all on the stack and then add to the
storage once in a polling cycle -- most drivers don't do that and
just increment the values in the storage directly, but this can be
less performant for frequently updated stats (or it's just my
embedded past).
Re u64 vs u64_stats_t -- the latter is more universal and
architecture-friendly, the former is used directly in most of the
drivers primarily because those drivers and the corresponding HW
are being run on 64-bit systems in the vast majority of cases, and
Ethtools stats themselves are not so critical to guard them with
anti-tearing. Anyways, local64_t is cheap on ARM64/x86_64 I guess?

[0] https://lore.kernel.org/netdev/a4602b15-25b1-c388-73b4-1f97f6f0e555@gmail.com/ (local)

Al