Re: [PATCH v2 net-next 21/26] ice: add XDP and XSK generic per-channel statistics

[Petr Machata]

Jakub Kicinski <kuba@xxxxxxxxxx> writes:

> On Sun, 28 Nov 2021 19:54:53 +0200 Ido Schimmel wrote:
>> > > Right, sure, I am also totally fine with having only a somewhat
>> > > restricted subset of stats available at the interface level and make
>> > > everything else be BPF-based. I'm hoping we can converge of a common
>> > > understanding of what this "minimal set" should be :)
>> > > 
>> > > Agreed. My immediate thought is that "XDP packets are interface packets"
>> > > but that is certainly not what we do today, so not sure if changing it
>> > > at this point would break things?  
>> > 
>> > I'd vote for taking the risk and trying to align all the drivers.  
>> 
>> I agree. I think IFLA_STATS64 in RTM_NEWLINK should contain statistics
>> of all the packets seen by the netdev. The breakdown into software /
>> hardware / XDP should be reported via RTM_NEWSTATS.
>
> Hm, in the offload case "seen by the netdev" may be unclear. For 
> the offload case I believe our recommendation was phrased more like 
> "all packets which would be seen by the netdev if there was no
> routing/tc offload", right?

Yes. The idea is to expose to Linux stats about traffic at conceptually
corresponding objects in the HW.

>
>> Currently, for soft devices such as VLANs, bridges and GRE, user space
>> only sees statistics of packets forwarded by software, which is quite
>> useless when forwarding is offloaded from the kernel to hardware.
>> 
>> Petr is working on exposing hardware statistics for such devices via
>> rtnetlink. Unlike XDP (?), we need to be able to let user space enable /
>> disable hardware statistics as we have a limited number of hardware
>> counters and they can also reduce the bandwidth when enabled. We are
>> thinking of adding a new RTM_SETSTATS for that:
>> 
>> # ip stats set dev swp1 hw_stats on
>
> Does it belong on the switch port? Not the netdev we want to track?

Yes, it does, and is designed that way. That was just muscle memory
typing that "swp1" above :)

You would do e.g. "ip stats set dev swp1.200 hw_stats on" or, "dev br1",
or something like that.

>> For query, something like (under discussion):
>> 
>> # ip stats show dev swp1 // all groups
>> # ip stats show dev swp1 group link
>> # ip stats show dev swp1 group offload // all sub-groups
>> # ip stats show dev swp1 group offload sub-group cpu
>> # ip stats show dev swp1 group offload sub-group hw
>> 
>> Like other iproute2 commands, these follow the nesting of the
>> RTM_{NEW,GET}STATS uAPI.
>
> But we do have IFLA_STATS_LINK_OFFLOAD_XSTATS, isn't it effectively 
> the same use case?

IFLA_STATS_LINK_OFFLOAD_XSTATS is a nest. Currently it carries just
CPU_HIT stats. The idea is to carry HW stats as well in that group.

>> Looking at patch #1 [1], I think that whatever you decide to expose for
>> XDP can be queried via:
>> 
>> # ip stats show dev swp1 group xdp
>> # ip stats show dev swp1 group xdp sub-group regular
>> # ip stats show dev swp1 group xdp sub-group xsk
>> 
>> Regardless, the following command should show statistics of all the
>> packets seen by the netdev:
>> 
>> # ip -s link show dev swp1
>> 
>> There is a PR [2] for node_exporter to use rtnetlink to fetch netdev
>> statistics instead of the old proc interface. It should be possible to
>> extend it to use RTM_*STATS for more fine-grained statistics.
>> 
>> [1] https://lore.kernel.org/netdev/20211123163955.154512-2-alexandr.lobakin@xxxxxxxxx/
>> [2] https://github.com/prometheus/node_exporter/pull/2074
>
> Nice!