Re: [PATCH net-next 07/21] ethernet, ena: convert to standard XDP stats

[Alexander Lobakin <alexandr.lobakin@intel.com>]

From: Shay Agroskin <shayagr@amazon.com>
Date: Wed, 4 Aug 2021 16:04:59 +0300

> Alexander Lobakin <alexandr.lobakin@intel.com> writes:
> 
> >
> >
> >
> > Its 6 XDP per-channel counters align just fine with the standard
> > stats.
> > Drop them from the custom Ethtool statistics and expose to the
> > standard stats infra instead.
> >
> > Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > ---
> >  drivers/net/ethernet/amazon/ena/ena_ethtool.c | 46 
> >  ++++++++++++++++---
> >  1 file changed, 40 insertions(+), 6 deletions(-)
> 
> Hi,
> thanks for making this patch. I like the idea of splitting stats 
> into a per-queue basis
> 
> >
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c 
> > b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > index 851a198cec82..1b6563641575 100644
> > --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > @@ -90,12 +90,6 @@ static const struct ena_stats 
> > ena_stats_rx_strings[] = {
> >         ENA_STAT_RX_ENTRY(bad_req_id),
> >         ENA_STAT_RX_ENTRY(empty_rx_ring),
> >         ENA_STAT_RX_ENTRY(csum_unchecked),
> > -       ENA_STAT_RX_ENTRY(xdp_aborted),
> > -       ENA_STAT_RX_ENTRY(xdp_drop),
> > -       ENA_STAT_RX_ENTRY(xdp_pass),
> > -       ENA_STAT_RX_ENTRY(xdp_tx),
> > -       ENA_STAT_RX_ENTRY(xdp_invalid),
> > -       ENA_STAT_RX_ENTRY(xdp_redirect),
> >
> 
> The ena_stats_rx_strings array is (indirectly) accessed through 
> ena_get_stats() function which is used for both fetching ethtool 
> stats and
> for sharing the stats with the device in case of an error (through 
> ena_dump_stats_ex() function).
> 
> The latter use is broken by removing the XDP specific stats from 
> ena_stats_rx_strings array.
> 
> I can submit an adaptation for the new system later (similar to 
> mlx5) if you prefer

Feel free to either do that (I'll exclude this patch from that
series then) or you can give me some little tips or examples or
anything on how to improve this one, so ena would stay converted.
Both ways are fine for me.

> thanks,
> Shay

Thanks,
Al

> >  };
> >
> >  static const struct ena_stats ena_stats_ena_com_strings[] = {
> > @@ -324,6 +318,44 @@ static void ena_get_ethtool_strings(struct 
> > net_device *netdev,
> >         }
> >  }
> >
> > +static int ena_get_std_stats_channels(struct net_device 
> > *netdev, u32 sset)
> > +{
> > +       const struct ena_adapter *adapter = netdev_priv(netdev);
> > +
> > +       switch (sset) {
> > +       case ETH_SS_STATS_XDP:
> > +               return adapter->num_io_queues;
> > +       default:
> > +               return -EOPNOTSUPP;
> > +       }
> > +}
> > +
> > +static void ena_get_xdp_stats(struct net_device *netdev,
> > +                             struct ethtool_xdp_stats 
> > *xdp_stats)
> > +{
> > +       const struct ena_adapter *adapter = netdev_priv(netdev);
> > +       const struct u64_stats_sync *syncp;
> > +       const struct ena_stats_rx *stats;
> > +       struct ethtool_xdp_stats *iter;
> > +       u32 i;
> > +
> ...
> >  {
> > @@ -916,6 +948,8 @@ static const struct ethtool_ops 
> > ena_ethtool_ops = {
> >         .get_tunable            = ena_get_tunable,
> >         .set_tunable            = ena_set_tunable,
> >         .get_ts_info            = ethtool_op_get_ts_info,
> > +       .get_std_stats_channels = ena_get_std_stats_channels,
> > +       .get_xdp_stats          = ena_get_xdp_stats,
> >  };
> >
> >  void ena_set_ethtool_ops(struct net_device *netdev)
> 
>