Re: [PATCH net-next v8 10/10] bng_en: add support for ethtool -S stats display

[Bhargava Chenna Marreddy]

> This isn't a bug, but Documentation/networking/statistics.rst flags a
> potential concern about interface duplication. The patch adds per-queue
> ring statistics (rxqN_ucast_packets, rxqN_mcast_packets,
> rxqN_bcast_packets, rxqN_ucast_bytes, rxqN_mcast_bytes, rxqN_bcast_bytes,
> and tx equivalents) to the ethtool -S interface.
>
> However, this driver already implements the standard per-queue statistics
> API via netdev_stat_ops (bnge_get_queue_stats_rx and
> bnge_get_queue_stats_tx in bnge_netdev.c). That interface exposes
> per-queue packets and bytes through netlink, which is the designated
> mechanism for per-queue counters.
>
> According to Documentation/networking/statistics.rst, per-queue stats
> should use the standard netdev_stat_ops interface rather than ethtool -S.
> The documentation states that drivers should avoid adding new counters to
> ethtool -S for values that have a standard uAPI.
>
> While the ethtool -S stats provide a ucast/mcast/bcast breakdown not
> directly available from the standard per-queue API (which aggregates to
> total packets and bytes), this still represents duplication of the
> fundamental per-queue packet and byte counters. Could these per-queue
> stats be omitted from ethtool -S, keeping only the truly hardware-specific
> stats like TPA, port-level, and priority stats that don't have a standard
> interface?

Thanks, Simon.

These counters report unicast, multicast, and broadcast traffic
separately. This
granularity is missing from the standard per-queue API, but we find it
essential for
hardware-level debugging (e.g., multicast storms or verifying RSS
steering logic).

Since this data is lost in the standard view, would it be acceptable
to provide these
via ethtool -S for better visibility?

Thanks,
Bhargava Marreddy

>
> [ ... ]
>
> > @@ -262,6 +740,9 @@ static const struct ethtool_ops bnge_ethtool_ops = {
> >       .nway_reset             = bnge_nway_reset,
> >       .get_pauseparam         = bnge_get_pauseparam,
> >       .set_pauseparam         = bnge_set_pauseparam,
> > +     .get_sset_count         = bnge_get_sset_count,
> > +     .get_strings            = bnge_get_strings,
> > +     .get_ethtool_stats      = bnge_get_ethtool_stats,
> >       .get_eth_phy_stats      = bnge_get_eth_phy_stats,
> >       .get_eth_mac_stats      = bnge_get_eth_mac_stats,
> >       .get_eth_ctrl_stats     = bnge_get_eth_ctrl_stats,

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature