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 <k...@kernel.org>
> Signed-off-by: Breno Leitao <lei...@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);
}
