diff mbox series

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

Message ID 20210803163641.3743-8-alexandr.lobakin@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series ethtool, stats: introduce and use standard XDP stats | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 22 of 22 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 64 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Alexander Lobakin Aug. 3, 2021, 4:36 p.m. UTC 
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(-)

Comments

Shay Agroskin Aug. 4, 2021, 1:04 p.m. UTC | #1 
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

thanks,
Shay

>  };
>
>  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)
Alexander Lobakin Aug. 4, 2021, 3:24 p.m. UTC | #2 
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)
> 
>
diff mbox series

Patch

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),
 };
 
 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;
+
+	for (i = 0; i < adapter->num_io_queues; i++) {
+		stats = &adapter->rx_ring[i].rx_stats;
+		syncp = &adapter->rx_ring[i].syncp;
+		iter = xdp_stats + i;
+
+		ena_safe_update_stat(&stats->xdp_drop, &iter->drop, syncp);
+		ena_safe_update_stat(&stats->xdp_pass, &iter->pass, syncp);
+		ena_safe_update_stat(&stats->xdp_tx, &iter->tx, syncp);
+		ena_safe_update_stat(&stats->xdp_redirect, &iter->redirect,
+				     syncp);
+		ena_safe_update_stat(&stats->xdp_aborted, &iter->aborted,
+				     syncp);
+		ena_safe_update_stat(&stats->xdp_invalid, &iter->invalid,
+				     syncp);
+	}
+}
+
 static int ena_get_link_ksettings(struct net_device *netdev,
 				  struct ethtool_link_ksettings *link_ksettings)
 {
@@ -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)