CC: Willem and some driver folks for more input, context: extending https://lore.kernel.org/all/20240306195509.1502746-1-kuba@xxxxxxxxxx/ to cover virtio stats. On Tue, 27 Feb 2024 16:03:00 +0800 Xuan Zhuo wrote: > +static const struct virtnet_stat_desc virtnet_stats_rx_basic_desc[] = { > + VIRTNET_STATS_DESC(rx, basic, packets), > + VIRTNET_STATS_DESC(rx, basic, bytes), Covered. > + VIRTNET_STATS_DESC(rx, basic, notifications), > + VIRTNET_STATS_DESC(rx, basic, interrupts), I haven't seen HW devices count interrupts coming from a specific queue (there's usually a lot more queues than IRQs these days), let's keep these in ethtool -S for now, unless someone has a HW use case. > + VIRTNET_STATS_DESC(rx, basic, drops), > + VIRTNET_STATS_DESC(rx, basic, drop_overruns), These are important, but we need to make sure we have a good definition for vendors to follow... drops I'd define as "sum of all packets which came into the device, but never left it, including but not limited to: packets dropped due to lack of buffer space, processing errors, explicitly set policies and packet filters." Call it hw-rx-drops ? overruns is a bit harder to precisely define. I was thinking of something more broad, like: "packets dropped due to transient lack of resources, such as buffer space, host descriptors etc." For context why not just go with virtio spec definition of "no descriptors" - for HW devices, what exact point in the pipeline drops depends on how back pressure is configured/implemented, and fetching descriptors is high latency, so differentiating between "PCIe is slow" and "host didn't post descriptors" is hard in practice. Call it hw-rx-drop-overruns ? > +static const struct virtnet_stat_desc virtnet_stats_tx_basic_desc[] = { > + VIRTNET_STATS_DESC(tx, basic, packets), > + VIRTNET_STATS_DESC(tx, basic, bytes), > + > + VIRTNET_STATS_DESC(tx, basic, notifications), > + VIRTNET_STATS_DESC(tx, basic, interrupts), > + > + VIRTNET_STATS_DESC(tx, basic, drops), These 5 same as rx. > + VIRTNET_STATS_DESC(tx, basic, drop_malformed), These I'd call hw-tx-drop-errors, "packets dropped because they were invalid or malformed"? > +static const struct virtnet_stat_desc virtnet_stats_rx_csum_desc[] = { > + VIRTNET_STATS_DESC(rx, csum, csum_valid), I think in kernel parlance that would translate to CHECKSUM_UNNECESSARY? So let's call it rx-csum-unnecessary ? I'd skip the hw- prefix for this one, it doesn't matter to the user if the HW or SW counted it. > + VIRTNET_STATS_DESC(rx, csum, needs_csum), Hm, I think this is a bit software/virt device specific, presumably rx-csum-partial for the kernel, up to you whether to make it ethtool -S or netlink. > + VIRTNET_STATS_DESC(rx, csum, csum_none), > + VIRTNET_STATS_DESC(rx, csum, csum_bad), These two make sense as is in netlink, should be fairly commonly reported by devices. Maybe add a note in "bad" that packets with bad csum are not discarded, but still delivered to the stack. > +static const struct virtnet_stat_desc virtnet_stats_tx_csum_desc[] = { > + VIRTNET_STATS_DESC(tx, csum, needs_csum), > + VIRTNET_STATS_DESC(tx, csum, csum_none), tx- version of what names we pick for rx-, netlink seems appropriate. > +static const struct virtnet_stat_desc virtnet_stats_rx_gso_desc[] = { > + VIRTNET_STATS_DESC(rx, gso, gso_packets), > + VIRTNET_STATS_DESC(rx, gso, gso_bytes), I used the term "GSO" in conversations about Rx and it often confuses people. Let's use "GRO", so hw-gro-packets, and hw-gro-bytes ? Or maybe coalesce? "hw-rx-coalesce" ? That's quite a bit longer.. Ah, and please mention in the doc that these counters "do not cover LRO i.e. any coalescing implementation which doesn't follow GRO rules". > + VIRTNET_STATS_DESC(rx, gso, gso_packets_coalesced), hw-gro-wire-packets ? No strong preference on the naming, but I find that saying -wire makes it 100% clear to everyone what the meaning is. > + VIRTNET_STATS_DESC(rx, gso, gso_bytes_coalesced), The documentation in the virtio spec seems to be identical to the one for gso_packets, which gotta be unintentional? I'm guessing this is hw-gro-wire-bytes? I.e. headers counted multiple times? > +static const struct virtnet_stat_desc virtnet_stats_tx_gso_desc[] = { > + VIRTNET_STATS_DESC(tx, gso, gso_packets), > + VIRTNET_STATS_DESC(tx, gso, gso_bytes), > + VIRTNET_STATS_DESC(tx, gso, gso_segments), > + VIRTNET_STATS_DESC(tx, gso, gso_segments_bytes), these 4 make sense as mirror of the Rx > + VIRTNET_STATS_DESC(tx, gso, gso_packets_noseg), > + VIRTNET_STATS_DESC(tx, gso, gso_bytes_noseg), Not sure what these are :) unless someone knows what it is and that HW devices report it, let's keep them in ethtool -S ? > +static const struct virtnet_stat_desc virtnet_stats_rx_speed_desc[] = { > + VIRTNET_STATS_DESC(rx, speed, packets_allowance_exceeded), hw-rx-drop-ratelimits ? "Allowance exceeded" is a bit of a mouthful to me, perhaps others disagree. The description from the virtio spec is quite good. > + VIRTNET_STATS_DESC(rx, speed, bytes_allowance_exceeded), No strong preference whether to expose this as a standard stat or ethtool -S, we don't generally keep byte counters for drops, so this would be special. > +static const struct virtnet_stat_desc virtnet_stats_tx_speed_desc[] = { > + VIRTNET_STATS_DESC(tx, speed, packets_allowance_exceeded), > + VIRTNET_STATS_DESC(tx, speed, packets_allowance_exceeded), same as rx
