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

[PATCH net-next v2 0/4] 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>
---
Changes in v2:
- Changed the RX collection place from nsim_napi_rx() to nsim_rcv (Joe
  Damato)
- Collect RX dropped packets statistic in nsim_queue_free() (Jakub)
- Added a helper in dstat to add values to RX dropped packets
- Link to v1: https://lore.kernel.org/r/20250611-netdevsim_stat-v1-0-c11b657d96bf@debian.org

---
Breno Leitao (4):
      netdevsim: migrate to dstats stats collection
      netdevsim: collect statistics at RX side
      net: add dev_dstats_rx_dropped_add() helper
      netdevsim: account dropped packet length in stats on queue free

 drivers/net/netdevsim/netdev.c    | 48 ++++++++++++++++-----------------------
 drivers/net/netdevsim/netdevsim.h |  5 ----
 include/linux/netdevice.h         | 10 ++++++++
 3 files changed, 29 insertions(+), 34 deletions(-)
---
base-commit: 6d4e01d29d87356924f1521ca6df7a364e948f13
change-id: 20250610-netdevsim_stat-95995921e03e

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

[PATCH net-next v2 1/4] 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>
Reviewed-by: Joe Damato <joe@dama.to>
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 fa5fbd97ad69e..5010d8eefc854 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)
 {
@@ -556,7 +537,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,
@@ -580,7 +560,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,
 };
@@ -594,7 +573,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;
@@ -606,7 +585,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;
@@ -618,7 +597,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;
@@ -890,6 +869,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;
 }
@@ -1022,7 +1002,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 v2 2/4] 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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 5010d8eefc854..de309ff69e43e 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -331,16 +331,24 @@ static int nsim_get_iflink(const struct net_device *dev)
 
 static int nsim_rcv(struct nsim_rq *rq, int budget)
 {
+	struct net_device *dev = rq->napi.dev;
 	struct sk_buff *skb;
-	int i;
+	unsigned int skblen;
+	int i, ret;
 
 	for (i = 0; i < budget; i++) {
 		if (skb_queue_empty(&rq->skb_queue))
 			break;
 
 		skb = skb_dequeue(&rq->skb_queue);
+		/* skb might be discard at netif_receive_skb, save the len */
+		skblen =  skb->len;
 		skb_mark_napi_id(skb, &rq->napi);
-		netif_receive_skb(skb);
+		ret = netif_receive_skb(skb);
+		if (ret == NET_RX_SUCCESS)
+			dev_dstats_rx_add(dev, skblen);
+		else
+			dev_dstats_rx_dropped(dev);
 	}
 
 	return i;

-- 
2.47.1

[PATCH net-next v2 3/4] net: add dev_dstats_rx_dropped_add() helper

[Breno Leitao]

Introduce the dev_dstats_rx_dropped_add() helper to allow incrementing
the rx_drops per-CPU statistic by an arbitrary value, rather than just
one. This is useful for drivers or code paths that need to account for
multiple dropped packets at once, such as when dropping entire queues.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 include/linux/netdevice.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index adb14db257984..2e9100468e1af 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3016,6 +3016,16 @@ static inline void dev_dstats_rx_dropped(struct net_device *dev)
 	u64_stats_update_end(&dstats->syncp);
 }
 
+static inline void dev_dstats_rx_dropped_add(struct net_device *dev,
+					     unsigned int packets)
+{
+	struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
+
+	u64_stats_update_begin(&dstats->syncp);
+	u64_stats_add(&dstats->rx_drops, packets);
+	u64_stats_update_end(&dstats->syncp);
+}
+
 static inline void dev_dstats_tx_add(struct net_device *dev,
 				     unsigned int len)
 {

-- 
2.47.1

[PATCH net-next v2 4/4] netdevsim: account dropped packet length in stats on queue free

[Breno Leitao]

Add a call to dev_dstats_rx_dropped_add() in nsim_queue_free() to
account for the number of packets dropped when purging the skb queue.

This improves the accuracy of RX drop statistics reported by
netdevsim.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netdevsim/netdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index de309ff69e43e..6e8fb8922ace2 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -634,7 +634,10 @@ static struct nsim_rq *nsim_queue_alloc(void)
 
 static void nsim_queue_free(struct nsim_rq *rq)
 {
+	struct net_device *dev = rq->napi.dev;
+
 	hrtimer_cancel(&rq->napi_timer);
+	dev_dstats_rx_dropped_add(dev, rq->skb_queue.qlen);
 	skb_queue_purge_reason(&rq->skb_queue, SKB_DROP_REASON_QUEUE_PURGE);
 	kfree(rq);
 }

-- 
2.47.1

Re: [PATCH net-next v2 4/4] netdevsim: account dropped packet length in stats on queue free

[Breno Leitao]

On Fri, Jun 13, 2025 at 03:40:01AM -0700, Breno Leitao wrote:
> Add a call to dev_dstats_rx_dropped_add() in nsim_queue_free() to
> account for the number of packets dropped when purging the skb queue.
> 
> This improves the accuracy of RX drop statistics reported by
> netdevsim.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>

> ---
>  drivers/net/netdevsim/netdev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index de309ff69e43e..6e8fb8922ace2 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -634,7 +634,10 @@ static struct nsim_rq *nsim_queue_alloc(void)
>  
>  static void nsim_queue_free(struct nsim_rq *rq)
>  {
> +	struct net_device *dev = rq->napi.dev;
> +
>  	hrtimer_cancel(&rq->napi_timer);
> +	dev_dstats_rx_dropped_add(dev, rq->skb_queue.qlen);

This is wrong and it will cause the kernel to crash in some cases, given
we can get here with dev == NULL, in the following path:

	nsim_destroy (drivers/net/netdevsim/netdev.c:1067) netdevsim
	nsim_dev_reload_destroy (drivers/net/netdevsim/dev.c:426 drivers/net/netdevsim/dev.c:1429 drivers/net/netdevsim/dev.c:1440 drivers/net/netdevsim/dev.c:1661) netdevsim
	nsim_dev_reload_down (drivers/net/netdevsim/dev.c:?) netdevsim
	devlink_reload (net/devlink/dev.c:462)
	? lock_acquire (kernel/locking/lockdep.c:5871)
	? devlink_remote_reload_actions_performed (net/devlink/dev.c:446)
	devlink_nl_reload_doit (net/devlink/dev.c:?)
	? devlink_reload (net/devlink/dev.c:520)
	? __nla_parse (lib/nlattr.c:732)
	genl_family_rcv_msg_doit (net/netlink/genetlink.c:1115)
	? genl_family_rcv_msg_dumpit (net/netlink/genetlink.c:1088)
	genl_rcv_msg (net/netlink/genetlink.c:? net/netlink/genetlink.c:1210)
	? genl_release (net/netlink/genetlink.c:1201)
	? devlink_nl_pre_doit_port (net/devlink/netlink.c:257)
	? devlink_reload (net/devlink/dev.c:520)
	? devlink_nl_post_doit (net/devlink/netlink.c:288)
	? __lock_acquire (kernel/locking/lockdep.c:?)
	netlink_rcv_skb (net/netlink/af_netlink.c:2534)
	? genl_release (net/netlink/genetlink.c:1201)
	? netlink_ack_tlv_fill (net/netlink/af_netlink.c:2511)
	? down_read (./arch/x86/include/asm/atomic64_64.h:20 ./include/linux/atomic/atomic-arch-fallback.h:2629 ./include/linux/atomic/atomic-long.h:79 ./include/linux/atomic/atomic-instrumented.h:3224 kernel/locking/rwsem.c:176 kernel/locking/rwsem.c:181 kernel/locking/rwsem.c:256 kernel/locking/rwsem.c:1247 kernel/locking/rwsem.c:1261 kernel/locking/rwsem.c:1526)
	genl_rcv (net/netlink/genetlink.c:1220)
	netlink_unicast (net/netlink/af_netlink.c:1314 net/netlink/af_netlink.c:1339)
	netlink_sendmsg (net/netlink/af_netlink.c:1883)
	? netlink_getsockopt (net/netlink/af_netlink.c:1802)
	? __might_fault (mm/memory.c:6991)
	__sys_sendto (net/socket.c:715 net/socket.c:727 net/socket.c:2180)
	? __ia32_sys_getpeername (net/socket.c:2147)
	? __might_fault (mm/memory.c:6991)
	? __bpf_trace_rseq_ip_fixup (kernel/rseq.c:425)
	__x64_sys_sendto (net/socket.c:2187 net/socket.c:2183 net/socket.c:2183)
	? entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
	do_syscall_64 (arch/x86/entry/syscall_64.c:?)
	? exc_page_fault (arch/x86/mm/fault.c:1536)
	entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)

I am wondering if this additional patch is too ugly:

	diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
	index 6e8fb8922ace2..328c7e83f2823 100644
	--- a/drivers/net/netdevsim/netdev.c
	+++ b/drivers/net/netdevsim/netdev.c
	@@ -637,7 +637,8 @@ static void nsim_queue_free(struct nsim_rq *rq)
		struct net_device *dev = rq->napi.dev;

		hrtimer_cancel(&rq->napi_timer);
	-       dev_dstats_rx_dropped_add(dev, rq->skb_queue.qlen);
	+       if (dev)
	+               dev_dstats_rx_dropped_add(dev, rq->skb_queue.qlen);
		skb_queue_purge_reason(&rq->skb_queue, SKB_DROP_REASON_QUEUE_PURGE);
		kfree(rq);
	}

Re: [PATCH net-next v2 4/4] netdevsim: account dropped packet length in stats on queue free

[Jakub Kicinski]

On Fri, 13 Jun 2025 07:47:11 -0700 Breno Leitao wrote:
> >  static void nsim_queue_free(struct nsim_rq *rq)
> >  {
> > +	struct net_device *dev = rq->napi.dev;
> > +
> >  	hrtimer_cancel(&rq->napi_timer);
> > +	dev_dstats_rx_dropped_add(dev, rq->skb_queue.qlen);  
> 
> This is wrong and it will cause the kernel to crash in some cases, given
> we can get here with dev == NULL, in the following path:

It's probably because NAPI wasn't registered yet in some paths.
You can pass dev in from the callers, it always exists, and all 
callers have it.
-- 
pw-bot: cr

Re: [PATCH net-next v2 4/4] netdevsim: account dropped packet length in stats on queue free

[Breno Leitao]

On Fri, Jun 13, 2025 at 07:55:07AM -0700, Jakub Kicinski wrote:
> On Fri, 13 Jun 2025 07:47:11 -0700 Breno Leitao wrote:
> > >  static void nsim_queue_free(struct nsim_rq *rq)
> > >  {
> > > +	struct net_device *dev = rq->napi.dev;
> > > +
> > >  	hrtimer_cancel(&rq->napi_timer);
> > > +	dev_dstats_rx_dropped_add(dev, rq->skb_queue.qlen);  
> > 
> > This is wrong and it will cause the kernel to crash in some cases, given
> > we can get here with dev == NULL, in the following path:
> 
> It's probably because NAPI wasn't registered yet in some paths.
> You can pass dev in from the callers, it always exists, and all 
> callers have it.

Right, that is a better approach.

I will update and send another version soon,
--breno