1. Patchew
  2. linux
  3. [PATCH net-next v2 0/4] netdevsim: implement RX statistics using NETDEV_PCPU_STAT_DSTATS
  4. View patch

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

Breno Leitao posted 4 patches 3 months, 4 weeks ago
There is a newer version of this series
[PATCH net-next v2 4/4] netdevsim: account dropped packet length in stats on queue free
Posted by Breno Leitao 3 months, 4 weeks ago 
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
Posted by Breno Leitao 3 months, 4 weeks ago 
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
Posted by Jakub Kicinski 3 months, 4 weeks ago 
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
Posted by Breno Leitao 3 months, 4 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.