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

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Thu, 14 Mar 2024 16:21:42 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> 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.

I apologize for the misunderstanding; that was not my intention. Your original
phrasing is indeed clear and comprehensive. Let's use your suggested
description.

> 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 :|

Sorry, not native speaker, I misunderstood.

Other is ok. I will fix in next version.

Thanks.

>
>   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)
> >  };
>