diff mbox series

[net-next,v2,5/7] netdevsim: report stats by default, like a real device

Message ID 20240403023426.1762996-6-kuba@kernel.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series selftests: net: groundwork for YNL-based tests | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; GEN HAS DIFF 2 files changed, 15 insertions(+);
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 943 this patch: 943
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 954 this patch: 954
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 954 this patch: 954
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 92 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-03--09-00 (tests: 950)

Commit Message

Jakub Kicinski April 3, 2024, 2:34 a.m. UTC
Real devices should implement qstats. Devices which support
pause or FEC configuration should also report the relevant stats.

nsim was missing FEC stats completely, some of the qstats
and pause stats required toggling a debugfs knob.

Note that the tests which used pause always initialize the setting
so they shouldn't be affected by the different starting value.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/ethtool.c | 11 ++++++++
 drivers/net/netdevsim/netdev.c  | 45 +++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

Comments

Petr Machata April 4, 2024, 10:40 a.m. UTC | #1
Jakub Kicinski <kuba@kernel.org> writes:

> Real devices should implement qstats. Devices which support
> pause or FEC configuration should also report the relevant stats.
>
> nsim was missing FEC stats completely, some of the qstats
> and pause stats required toggling a debugfs knob.
>
> Note that the tests which used pause always initialize the setting
> so they shouldn't be affected by the different starting value.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Petr Machata <petrm@nvidia.com>

Just:

> @@ -330,6 +331,49 @@ static const struct net_device_ops nsim_vf_netdev_ops = {
>  	.ndo_set_features	= nsim_set_features,
>  };
>  
> +/* We don't have true par-queue stats, yet, so do some random fakery here. */

per

> +static void nsim_get_queue_stats_rx(struct net_device *dev, int idx,
> +				    struct netdev_queue_stats_rx *stats)
> +{
> +	struct rtnl_link_stats64 rtstats = {};
> +
> +	nsim_get_stats64(dev, &rtstats);
> +
> +	stats->packets = rtstats.rx_packets - !!rtstats.rx_packets;

This is just to make sure that per-queue stats are lower than the
overall rtstats I presume?

> +	stats->bytes = rtstats.rx_bytes;
> +}
> +
> +static void nsim_get_queue_stats_tx(struct net_device *dev, int idx,
> +				    struct netdev_queue_stats_tx *stats)
> +{
> +	struct rtnl_link_stats64 rtstats = {};
> +
> +	nsim_get_stats64(dev, &rtstats);
> +
> +	stats->packets = rtstats.tx_packets - !!rtstats.tx_packets;
> +	stats->bytes = rtstats.tx_bytes;
> +}
Paolo Abeni April 4, 2024, 1:40 p.m. UTC | #2
On Tue, 2024-04-02 at 19:34 -0700, Jakub Kicinski wrote:
> Real devices should implement qstats. Devices which support
> pause or FEC configuration should also report the relevant stats.
> 
> nsim was missing FEC stats completely, some of the qstats
> and pause stats required toggling a debugfs knob.
> 
> Note that the tests which used pause always initialize the setting
> so they shouldn't be affected by the different starting value.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  drivers/net/netdevsim/ethtool.c | 11 ++++++++
>  drivers/net/netdevsim/netdev.c  | 45 +++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
> index bd546d4d26c6..3f9c9327f149 100644
> --- a/drivers/net/netdevsim/ethtool.c
> +++ b/drivers/net/netdevsim/ethtool.c
> @@ -140,6 +140,13 @@ nsim_set_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam)
>  	return 0;
>  }
>  
> +static void
> +nsim_get_fec_stats(struct net_device *dev, struct ethtool_fec_stats *fec_stats)
> +{
> +	fec_stats->corrected_blocks.total = 123;
> +	fec_stats->uncorrectable_blocks.total = 4;
> +}
> +
>  static int nsim_get_ts_info(struct net_device *dev,
>  			    struct ethtool_ts_info *info)
>  {
> @@ -163,6 +170,7 @@ static const struct ethtool_ops nsim_ethtool_ops = {
>  	.set_channels			= nsim_set_channels,
>  	.get_fecparam			= nsim_get_fecparam,
>  	.set_fecparam			= nsim_set_fecparam,
> +	.get_fec_stats			= nsim_get_fec_stats,
>  	.get_ts_info			= nsim_get_ts_info,
>  };
>  
> @@ -182,6 +190,9 @@ void nsim_ethtool_init(struct netdevsim *ns)
>  
>  	nsim_ethtool_ring_init(ns);
>  
> +	ns->ethtool.pauseparam.report_stats_rx = true;
> +	ns->ethtool.pauseparam.report_stats_tx = true;
> +
>  	ns->ethtool.fec.fec = ETHTOOL_FEC_NONE;
>  	ns->ethtool.fec.active_fec = ETHTOOL_FEC_NONE;
>  
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 8330bc0bcb7e..096ac0abbc02 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -19,6 +19,7 @@
>  #include <linux/module.h>
>  #include <linux/netdevice.h>
>  #include <linux/slab.h>
> +#include <net/netdev_queues.h>
>  #include <net/netlink.h>
>  #include <net/pkt_cls.h>
>  #include <net/rtnetlink.h>
> @@ -330,6 +331,49 @@ static const struct net_device_ops nsim_vf_netdev_ops = {
>  	.ndo_set_features	= nsim_set_features,
>  };
>  
> +/* We don't have true par-queue stats, yet, so do some random fakery here. */
> +static void nsim_get_queue_stats_rx(struct net_device *dev, int idx,
> +				    struct netdev_queue_stats_rx *stats)
> +{
> +	struct rtnl_link_stats64 rtstats = {};
> +
> +	nsim_get_stats64(dev, &rtstats);
> +
> +	stats->packets = rtstats.rx_packets - !!rtstats.rx_packets;
> +	stats->bytes = rtstats.rx_bytes;
> +}
> +
> +static void nsim_get_queue_stats_tx(struct net_device *dev, int idx,
> +				    struct netdev_queue_stats_tx *stats)
> +{
> +	struct rtnl_link_stats64 rtstats = {};
> +
> +	nsim_get_stats64(dev, &rtstats);
> +
> +	stats->packets = rtstats.tx_packets - !!rtstats.tx_packets;
> +	stats->bytes = rtstats.tx_bytes;

It looks the stats will not be self-consistent with multiple queues
enabled. 

What about zeroing 'stats' when idx > 0 ?

/P
Jakub Kicinski April 4, 2024, 1:52 p.m. UTC | #3
On Thu, 4 Apr 2024 12:40:04 +0200 Petr Machata wrote:
> > +	stats->packets = rtstats.rx_packets - !!rtstats.rx_packets;  
> 
> This is just to make sure that per-queue stats are lower than the
> overall rtstats I presume?

Yes, to fake some "non-queue" stats.
Jakub Kicinski April 4, 2024, 1:53 p.m. UTC | #4
On Thu, 04 Apr 2024 15:40:33 +0200 Paolo Abeni wrote:
> > +	nsim_get_stats64(dev, &rtstats);
> > +
> > +	stats->packets = rtstats.tx_packets - !!rtstats.tx_packets;
> > +	stats->bytes = rtstats.tx_bytes;  
> 
> It looks the stats will not be self-consistent with multiple queues
> enabled. 
> 
> What about zeroing 'stats' when idx > 0 ?

Ah, good catch. We do allow allocating multiple queues, even tho 
we don't do anything with them on the driver side, yet..
diff mbox series

Patch

diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index bd546d4d26c6..3f9c9327f149 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -140,6 +140,13 @@  nsim_set_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam)
 	return 0;
 }
 
+static void
+nsim_get_fec_stats(struct net_device *dev, struct ethtool_fec_stats *fec_stats)
+{
+	fec_stats->corrected_blocks.total = 123;
+	fec_stats->uncorrectable_blocks.total = 4;
+}
+
 static int nsim_get_ts_info(struct net_device *dev,
 			    struct ethtool_ts_info *info)
 {
@@ -163,6 +170,7 @@  static const struct ethtool_ops nsim_ethtool_ops = {
 	.set_channels			= nsim_set_channels,
 	.get_fecparam			= nsim_get_fecparam,
 	.set_fecparam			= nsim_set_fecparam,
+	.get_fec_stats			= nsim_get_fec_stats,
 	.get_ts_info			= nsim_get_ts_info,
 };
 
@@ -182,6 +190,9 @@  void nsim_ethtool_init(struct netdevsim *ns)
 
 	nsim_ethtool_ring_init(ns);
 
+	ns->ethtool.pauseparam.report_stats_rx = true;
+	ns->ethtool.pauseparam.report_stats_tx = true;
+
 	ns->ethtool.fec.fec = ETHTOOL_FEC_NONE;
 	ns->ethtool.fec.active_fec = ETHTOOL_FEC_NONE;
 
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 8330bc0bcb7e..096ac0abbc02 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -19,6 +19,7 @@ 
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/slab.h>
+#include <net/netdev_queues.h>
 #include <net/netlink.h>
 #include <net/pkt_cls.h>
 #include <net/rtnetlink.h>
@@ -330,6 +331,49 @@  static const struct net_device_ops nsim_vf_netdev_ops = {
 	.ndo_set_features	= nsim_set_features,
 };
 
+/* We don't have true par-queue stats, yet, so do some random fakery here. */
+static void nsim_get_queue_stats_rx(struct net_device *dev, int idx,
+				    struct netdev_queue_stats_rx *stats)
+{
+	struct rtnl_link_stats64 rtstats = {};
+
+	nsim_get_stats64(dev, &rtstats);
+
+	stats->packets = rtstats.rx_packets - !!rtstats.rx_packets;
+	stats->bytes = rtstats.rx_bytes;
+}
+
+static void nsim_get_queue_stats_tx(struct net_device *dev, int idx,
+				    struct netdev_queue_stats_tx *stats)
+{
+	struct rtnl_link_stats64 rtstats = {};
+
+	nsim_get_stats64(dev, &rtstats);
+
+	stats->packets = rtstats.tx_packets - !!rtstats.tx_packets;
+	stats->bytes = rtstats.tx_bytes;
+}
+
+static void nsim_get_base_stats(struct net_device *dev,
+				struct netdev_queue_stats_rx *rx,
+				struct netdev_queue_stats_tx *tx)
+{
+	struct rtnl_link_stats64 rtstats = {};
+
+	nsim_get_stats64(dev, &rtstats);
+
+	rx->packets = !!rtstats.rx_packets;
+	rx->bytes = 0;
+	tx->packets = !!rtstats.tx_packets;
+	tx->bytes = 0;
+}
+
+static const struct netdev_stat_ops nsim_stat_ops = {
+	.get_queue_stats_tx	= nsim_get_queue_stats_tx,
+	.get_queue_stats_rx	= nsim_get_queue_stats_rx,
+	.get_base_stats		= nsim_get_base_stats,
+};
+
 static void nsim_setup(struct net_device *dev)
 {
 	ether_setup(dev);
@@ -360,6 +404,7 @@  static int nsim_init_netdevsim(struct netdevsim *ns)
 
 	ns->phc = phc;
 	ns->netdev->netdev_ops = &nsim_netdev_ops;
+	ns->netdev->stat_ops = &nsim_stat_ops;
 
 	err = nsim_udp_tunnels_info_create(ns->nsim_dev, ns->netdev);
 	if (err)