[PATCH net-next 0/2] netdevsim: implement RX statistics using NETDEV_PCPU_STAT_DSTATS

[PATCH net-next 0/2] netdevsim: implement RX statistics using NETDEV_PCPU_STAT_DSTATS

[Breno Leitao]

The netdevsim driver previously lacked RX statistics support, which
prevented its use with the GenerateTraffic() test framework, as this
framework verifies traffic flow by checking RX byte counts.

This patch migrates netdevsim from its custom statistics collection to
the NETDEV_PCPU_STAT_DSTATS framework, as suggested by Jakub. This
change not only standardizes the statistics handling but also adds the
necessary RX statistics support required by the test framework.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
Breno Leitao (2):
      netdevsim: migrate to dstats stats collection
      netdevsim: collect statistics at RX side

 drivers/net/netdevsim/netdev.c    | 37 ++++++++++---------------------------
 drivers/net/netdevsim/netdevsim.h |  5 -----
 2 files changed, 10 insertions(+), 32 deletions(-)
---
base-commit: 0097c4195b1d0ca57d15979626c769c74747b5a0
change-id: 20250610-netdevsim_stat-95995921e03e

Best regards,
-- 
Breno Leitao <leitao@debian.org>

[PATCH net-next 1/2] netdevsim: migrate to dstats stats collection

[Breno Leitao]

Replace custom statistics tracking with the kernel's dstats infrastructure
to simplify code and improve consistency with other network drivers.

This change:
- Sets dev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS for automatic
  automatic allocation and deallocation.
- Removes manual stats fields and their update
- Replaces custom nsim_get_stats64() with dev_get_stats()
- Uses dev_dstats_tx_add() and dev_dstats_tx_dropped() helpers
- Eliminates the need for manual synchronization primitives

The dstats framework provides the same functionality with less code.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netdevsim/netdev.c    | 33 ++++++---------------------------
 drivers/net/netdevsim/netdevsim.h |  5 -----
 2 files changed, 6 insertions(+), 32 deletions(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index af545d42961c3..67871d31252fe 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -93,19 +93,14 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		hrtimer_start(&rq->napi_timer, us_to_ktime(5), HRTIMER_MODE_REL);
 
 	rcu_read_unlock();
-	u64_stats_update_begin(&ns->syncp);
-	ns->tx_packets++;
-	ns->tx_bytes += len;
-	u64_stats_update_end(&ns->syncp);
+	dev_dstats_tx_add(dev, skb->len);
 	return NETDEV_TX_OK;
 
 out_drop_free:
 	dev_kfree_skb(skb);
 out_drop_cnt:
 	rcu_read_unlock();
-	u64_stats_update_begin(&ns->syncp);
-	ns->tx_dropped++;
-	u64_stats_update_end(&ns->syncp);
+	dev_dstats_tx_dropped(dev);
 	return NETDEV_TX_OK;
 }
 
@@ -126,20 +121,6 @@ static int nsim_change_mtu(struct net_device *dev, int new_mtu)
 	return 0;
 }
 
-static void
-nsim_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
-{
-	struct netdevsim *ns = netdev_priv(dev);
-	unsigned int start;
-
-	do {
-		start = u64_stats_fetch_begin(&ns->syncp);
-		stats->tx_bytes = ns->tx_bytes;
-		stats->tx_packets = ns->tx_packets;
-		stats->tx_dropped = ns->tx_dropped;
-	} while (u64_stats_fetch_retry(&ns->syncp, start));
-}
-
 static int
 nsim_setup_tc_block_cb(enum tc_setup_type type, void *type_data, void *cb_priv)
 {
@@ -555,7 +536,6 @@ static const struct net_device_ops nsim_netdev_ops = {
 	.ndo_set_mac_address	= eth_mac_addr,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_change_mtu		= nsim_change_mtu,
-	.ndo_get_stats64	= nsim_get_stats64,
 	.ndo_set_vf_mac		= nsim_set_vf_mac,
 	.ndo_set_vf_vlan	= nsim_set_vf_vlan,
 	.ndo_set_vf_rate	= nsim_set_vf_rate,
@@ -579,7 +559,6 @@ static const struct net_device_ops nsim_vf_netdev_ops = {
 	.ndo_set_mac_address	= eth_mac_addr,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_change_mtu		= nsim_change_mtu,
-	.ndo_get_stats64	= nsim_get_stats64,
 	.ndo_setup_tc		= nsim_setup_tc,
 	.ndo_set_features	= nsim_set_features,
 };
@@ -593,7 +572,7 @@ static void nsim_get_queue_stats_rx(struct net_device *dev, int idx,
 	struct rtnl_link_stats64 rtstats = {};
 
 	if (!idx)
-		nsim_get_stats64(dev, &rtstats);
+		dev_get_stats(dev, &rtstats);
 
 	stats->packets = rtstats.rx_packets - !!rtstats.rx_packets;
 	stats->bytes = rtstats.rx_bytes;
@@ -605,7 +584,7 @@ static void nsim_get_queue_stats_tx(struct net_device *dev, int idx,
 	struct rtnl_link_stats64 rtstats = {};
 
 	if (!idx)
-		nsim_get_stats64(dev, &rtstats);
+		dev_get_stats(dev, &rtstats);
 
 	stats->packets = rtstats.tx_packets - !!rtstats.tx_packets;
 	stats->bytes = rtstats.tx_bytes;
@@ -617,7 +596,7 @@ static void nsim_get_base_stats(struct net_device *dev,
 {
 	struct rtnl_link_stats64 rtstats = {};
 
-	nsim_get_stats64(dev, &rtstats);
+	dev_get_stats(dev, &rtstats);
 
 	rx->packets = !!rtstats.rx_packets;
 	rx->bytes = 0;
@@ -889,6 +868,7 @@ static void nsim_setup(struct net_device *dev)
 			    NETIF_F_HW_CSUM |
 			    NETIF_F_LRO |
 			    NETIF_F_TSO;
+	dev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
 	dev->max_mtu = ETH_MAX_MTU;
 	dev->xdp_features = NETDEV_XDP_ACT_HW_OFFLOAD;
 }
@@ -1021,7 +1001,6 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
 	dev_net_set(dev, nsim_dev_net(nsim_dev));
 	ns = netdev_priv(dev);
 	ns->netdev = dev;
-	u64_stats_init(&ns->syncp);
 	ns->nsim_dev = nsim_dev;
 	ns->nsim_dev_port = nsim_dev_port;
 	ns->nsim_bus_dev = nsim_dev->nsim_bus_dev;
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index d04401f0bdf79..343b8f19dbed6 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -108,11 +108,6 @@ struct netdevsim {
 
 	int rq_reset_mode;
 
-	u64 tx_packets;
-	u64 tx_bytes;
-	u64 tx_dropped;
-	struct u64_stats_sync syncp;
-
 	struct nsim_bus_dev *nsim_bus_dev;
 
 	struct bpf_prog	*bpf_offloaded;

-- 
2.47.1

[PATCH net-next 2/2] netdevsim: collect statistics at RX side

[Breno Leitao]

When the RX side of netdevsim was added, the RX statistics were missing,
making the driver unusable for GenerateTraffic() test framework.

This patch adds proper statistics tracking on RX side, complementing the
TX path.

Signed-off-by: Breno Leitao <Leitao@debian.org>
---
 drivers/net/netdevsim/netdev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 67871d31252fe..590cb5bb0d20b 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -39,12 +39,16 @@ MODULE_IMPORT_NS("NETDEV_INTERNAL");
 
 static int nsim_napi_rx(struct nsim_rq *rq, struct sk_buff *skb)
 {
+	struct net_device *dev = rq->napi.dev;
+
 	if (skb_queue_len(&rq->skb_queue) > NSIM_RING_SIZE) {
 		dev_kfree_skb_any(skb);
+		dev_dstats_rx_dropped(dev);
 		return NET_RX_DROP;
 	}
 
 	skb_queue_tail(&rq->skb_queue, skb);
+	dev_dstats_rx_add(dev, skb->len);
 	return NET_RX_SUCCESS;
 }
 

-- 
2.47.1

Re: [PATCH net-next 1/2] netdevsim: migrate to dstats stats collection

[Joe Damato]

On Wed, Jun 11, 2025 at 08:06:19AM -0700, Breno Leitao wrote:
> Replace custom statistics tracking with the kernel's dstats infrastructure
> to simplify code and improve consistency with other network drivers.
> 
> This change:
> - Sets dev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS for automatic
>   automatic allocation and deallocation.

Ignorable minor nits: "automatic" repeated twice in the list item above and the
other items in the list below do not end with periods.

> - Removes manual stats fields and their update
> - Replaces custom nsim_get_stats64() with dev_get_stats()
> - Uses dev_dstats_tx_add() and dev_dstats_tx_dropped() helpers
> - Eliminates the need for manual synchronization primitives
> 
> The dstats framework provides the same functionality with less code.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/net/netdevsim/netdev.c    | 33 ++++++---------------------------
>  drivers/net/netdevsim/netdevsim.h |  5 -----
>  2 files changed, 6 insertions(+), 32 deletions(-)


Reviewed-by: Joe Damato <joe@dama.to>

Re: [PATCH net-next 2/2] netdevsim: collect statistics at RX side

[Joe Damato]

On Wed, Jun 11, 2025 at 08:06:20AM -0700, Breno Leitao wrote:
> When the RX side of netdevsim was added, the RX statistics were missing,
> making the driver unusable for GenerateTraffic() test framework.
> 
> This patch adds proper statistics tracking on RX side, complementing the
> TX path.
> 
> Signed-off-by: Breno Leitao <Leitao@debian.org>
> ---
>  drivers/net/netdevsim/netdev.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 67871d31252fe..590cb5bb0d20b 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -39,12 +39,16 @@ MODULE_IMPORT_NS("NETDEV_INTERNAL");
>  
>  static int nsim_napi_rx(struct nsim_rq *rq, struct sk_buff *skb)
>  {
> +	struct net_device *dev = rq->napi.dev;
> +
>  	if (skb_queue_len(&rq->skb_queue) > NSIM_RING_SIZE) {
>  		dev_kfree_skb_any(skb);
> +		dev_dstats_rx_dropped(dev);
>  		return NET_RX_DROP;
>  	}
>  
>  	skb_queue_tail(&rq->skb_queue, skb);
> +	dev_dstats_rx_add(dev, skb->len);
>  	return NET_RX_SUCCESS;
>  }

Hm, I was wondering: is it maybe better to add the stats code to nsim_poll or
nsim_rcv instead?

It "feels" like other drivers would be bumping RX stats in their NAPI poll
function (which is nsim_poll, the naming of the functions is a bit confusing
here), so it seems like netdevsim maybe should too?

Re: [PATCH net-next 2/2] netdevsim: collect statistics at RX side

[Jakub Kicinski]

On Thu, 12 Jun 2025 17:25:52 +0300 Joe Damato wrote:
> It "feels" like other drivers would be bumping RX stats in their NAPI poll
> function (which is nsim_poll, the naming of the functions is a bit confusing
> here), so it seems like netdevsim maybe should too?

Good point, and then we should probably add the queue length to
rx_dropped in nsim_queue_free() before we call purge?
-- 
pw-bot: cr

Re: [PATCH net-next 2/2] netdevsim: collect statistics at RX side

[Breno Leitao]

On Thu, Jun 12, 2025 at 07:45:03AM -0700, Jakub Kicinski wrote:
> On Thu, 12 Jun 2025 17:25:52 +0300 Joe Damato wrote:
> > It "feels" like other drivers would be bumping RX stats in their NAPI poll
> > function (which is nsim_poll, the naming of the functions is a bit confusing
> > here), so it seems like netdevsim maybe should too?
> 
> Good point, and then we should probably add the queue length to
> rx_dropped in nsim_queue_free() before we call purge?

Thanks Joe and Jakub, and after a review, I agree it might be a better
approach. I will update this patch with this new approach
and send a v2 tomorrow.

Thanks for the review,
--breno