On Thu, 14 Mar 2024 16:21:42 -0700, Jakub Kicinski <kuba@xxxxxxxxxx> 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)
> > };
>
