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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7fd250f7-30e9-4392-ae1e-c3664222f9de@davidwei.uk>
Date: Tue, 2 Apr 2024 19:51:01 -0700
From: David Wei <dw@...idwei.uk>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
 shuah@...nel.org, sdf@...gle.com, donald.hunter@...il.com,
 linux-kselftest@...r.kernel.org, petrm@...dia.com
Subject: Re: [PATCH net-next 5/7] netdevsim: report stats by default, like a
 real device

On 2024-04-01 18:05, 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@...nel.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. */

nit: per-queue

> +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;

Why subtract !!rtstats.rx_packets? This evaluates to 0 if rx_packets is
0 and 1 if rx_packets is non-zero.

> +	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)