Re: [PATCH net-next v4 7/8] netdev: add queue stats

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 14 Mar 2024 16:54:58 +0800 Xuan Zhuo wrote:
> +      -
> +        name: rx-hw-drops
> +        doc: |
> +          Number of packets that arrived at the device but never left it,
> +          encompassing packets dropped for reasons such as insufficient buffer

s/encompassing/including/

> +          space, processing errors, as well as those affected by explicitly
> +          defined policies and packet filtering criteria.

I'm afraid the attempt to "correct my English" ended up decreasing the
clarity. Maybe use what I suggested more directly?

  Number of all packets which entered the device, but never left it,
  including but not limited to: packets dropped due to lack of buffer
  space, processing errors, explicit or implicit policies and packet filters.

> +        type: uint
> +      -
> +        name: rx-hw-drop-overruns
> +        doc: |
> +          Number of packets that arrived at the device but never left it due to
> +          no descriptors.

I put a paragraph in my previous email explaining how tricky this is to
state for HW devices and you just went with the virtio spec :|

  Number of packets dropped due to transient lack of resources, such as
  buffer space, host descriptors etc.

> +      -
> +        name: rx-csum-bad
> +        doc: |
> +          Number of packets that are dropped by device due to invalid checksum.

Quoting myself:

>> Maybe add a note in "bad" that packets with bad csum are not
>> discarded, but still delivered to the stack.

Devices should not drop packets due to invalid L4 checksums.

> +        type: uint
> +      -
> +        name: rx-hw-gro-packets
> +        doc: |
> +          Number of packets that area coalesced from smaller packets by the device,
> +          that do not cover LRO packets.

s/area/were/ ?
s/that do not cover LRO packets/Counts only packets coalesced with the
HW-GRO netdevice feature, LRO-coalesced packets are not counted./

> +        type: uint
> +      -
> +        name: rx-hw-gro-bytes
> +        doc: see `rx-hw-gro-packets`.
> +        type: uint
> +      -
> +        name: rx-hw-gro-wire-packets
> +        doc: |
> +          Number of packets that area coalesced to bigger packets(no LRO packets)
> +          by the device.

s/area/were/
s/packets(no LRO packets)/packets by the HW-GRO feature of the device/

> +        type: uint
> +      -
> +        name: rx-hw-gro-wire-bytes
> +        doc: see `rx-hw-gro-wire-packets`.

Please make sure the "See `xyz`." references are capitalized (See)
and end with a full stop.

> +        type: uint
> +      -
> +        name: rx-hw-drop-ratelimits
> +        doc: |
> +          Number of the packets dropped by the device due to the received
> +          packets bitrate exceeding the device speed limit.

Maybe s/speed/rate/

> +        type: uint
> +      -
> +        name: tx-hw-drops
> +        doc: |
> +          Number of packets that arrived at the device but never left it,
> +          encompassing packets dropped for reasons such as processing errors, as
> +          well as those affected by explicitly defined policies and packet
> +          filtering criteria.
> +        type: uint
> +      -
> +        name: tx-hw-drop-errors
> +        doc: Number of packets dropped because they were invalid or malformed.
> +        type: uint
> +      -
> +        name: tx-csum-none
> +        doc: |
> +          Number of packets that do not require the device to calculate the
> +          checksum.

I think we should use past tense everywhere.

> +        type: uint
> +      -
> +        name: tx-needs-csum
> +        doc: |
> +          Number of packets that require the device to calculate the
> +          checksum.
> +        type: uint
> +      -
> +        name: tx-hw-gso-packets
> +        doc: |
> +          Number of packets that necessitated segmentation into smaller packets
> +          by the device.
> +        type: uint
> +      -
> +        name: tx-hw-gso-bytes
> +        doc: |
> +          Successfully segmented into smaller packets by the device, see
> +          `tx-hw-gso-packets`.

Maybe stick to "see `tx-hw-gso-packets`", none of the other counters
mention the send being "successful".

> +        type: uint
> +      -
> +        name: tx-hw-gso-wire-packets
> +        doc: |
> +          Number of the small packets that segmented from bigger packets by the
> +          device.

Maybe

  Number of wire-sized packets generated by processing
  `tx-hw-gso-packets`

?

> +        type: uint
> +      -
> +        name: tx-hw-gso-wire-bytes
> +        doc: See `tx-hw-gso-wire-packets`.
> +
> +        type: uint
> +      -
> +        name: tx-hw-drop-ratelimits
> +        doc: |
> +          Number of the packets dropped by the device due to the transmit
> +          packets bitrate exceeding the device speed limit.

s/speed/rate/

> +        type: uint
>  
>  operations:
>    list:
> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> index 1ec408585373..c7ac4539eafc 100644
> --- a/include/net/netdev_queues.h
> +++ b/include/net/netdev_queues.h
> @@ -9,11 +9,38 @@ struct netdev_queue_stats_rx {
>  	u64 bytes;
>  	u64 packets;
>  	u64 alloc_fail;
> +
> +	u64 hw_drops;
> +	u64 hw_drop_overruns;
> +
> +	u64 csum_unnecessary;
> +	u64 csum_none;
> +	u64 csum_bad;
> +
> +	u64 hw_gro_packets;
> +	u64 hw_gro_bytes;
> +	u64 hw_gro_wire_packets;
> +	u64 hw_gro_wire_bytes;
> +
> +	u64 hw_drop_ratelimits;
>  };
>  
>  struct netdev_queue_stats_tx {
>  	u64 bytes;
>  	u64 packets;
> +
> +	u64 hw_drops;
> +	u64 hw_drop_errors;
> +
> +	u64 csum_none;
> +	u64 needs_csum;
> +
> +	u64 hw_gso_packets;
> +	u64 hw_gso_bytes;
> +	u64 hw_gso_wire_packets;
> +	u64 hw_gso_wire_bytes;
> +
> +	u64 hw_drop_ratelimits;
>  };
>  
>  /**
> diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> index bb65ee840cda..702d69b09f14 100644
> --- a/include/uapi/linux/netdev.h
> +++ b/include/uapi/linux/netdev.h
> @@ -147,6 +147,26 @@ enum {
>  	NETDEV_A_QSTATS_TX_BYTES,
>  	NETDEV_A_QSTATS_RX_ALLOC_FAIL,
>  

Looks hand written. Once you update the yaml file, you should run:

./tools/net/ynl/ynl-regen.sh

This will generate the uAPI changes for you.

> +	NETDEV_A_QSTATS_RX_HW_DROPS,
> +	NETDEV_A_QSTATS_RX_HW_DROP_OVERRUNS,
> +	NETDEV_A_QSTATS_RX_CSUM_UNNECESSARY,
> +	NETDEV_A_QSTATS_RX_CSUM_NONE,
> +	NETDEV_A_QSTATS_RX_CSUM_BAD,
> +	NETDEV_A_QSTATS_RX_HW_GRO_PACKETS,
> +	NETDEV_A_QSTATS_RX_HW_GRO_BYTES,
> +	NETDEV_A_QSTATS_RX_HW_GRO_WIRE_PACKETS,
> +	NETDEV_A_QSTATS_RX_HW_GRO_WIRE_BYTES,
> +	NETDEV_A_QSTATS_RX_HW_DROP_RATELIMITS,
> +	NETDEV_A_QSTATS_TX_HW_DROPS,
> +	NETDEV_A_QSTATS_TX_HW_DROP_ERRORS,
> +	NETDEV_A_QSTATS_TX_CSUM_NONE,
> +	NETDEV_A_QSTATS_TX_NEEDS_CSUM,
> +	NETDEV_A_QSTATS_TX_HW_GSO_PACKETS,
> +	NETDEV_A_QSTATS_TX_HW_GSO_BYTES,
> +	NETDEV_A_QSTATS_TX_HW_GSO_WIRE_PACKETS,
> +	NETDEV_A_QSTATS_TX_HW_GSO_WIRE_BYTES,
> +	NETDEV_A_QSTATS_TX_HW_DROP_RATELIMITS,
> +
>  	__NETDEV_A_QSTATS_MAX,
>  	NETDEV_A_QSTATS_MAX = (__NETDEV_A_QSTATS_MAX - 1)
>  };