Re: [PATCH net-next v2 2/3] veth: link RX queues to their NAPI instances

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In drivers/net/veth.c, veth_napi_enable_range() still creates
  the per-queue NAPI with plain netif_napi_add(dev, &rq->xdp_napi,…

--

veth: link RX queues to their NAPI instances

This patch links each veth RX queue to its NAPI instance with
netif_queue_set_napi(). The link is set after napi_enable() and cleared
before napi_disable(). With it, queue-get reports a napi-id, and AF_XDP
copy-mode sockets bound to a veth queue get a NAPI ID for busy polling.

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index f5097696df15e..c042771864051 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -1062,6 +1062,8 @@ static int veth_napi_enable_range(struct net_device *dev, int start, int end)
>  		netif_napi_add(dev, &rq->xdp_napi, veth_poll);
>  		rcu_assign_pointer(rq->xdp_prog, priv->_xdp_prog);
>  		napi_enable(&rq->xdp_napi);
> +		netif_queue_set_napi(dev, i, NETDEV_QUEUE_TYPE_RX,
> +				     &rq->xdp_napi);

[Severity: Medium]
Should this be netif_napi_add_config(dev, &rq->xdp_napi, veth_poll, i)
rather than netif_napi_add()?

As written, the queue-to-NAPI link only holds until the next
reconfiguration. veth_napi_del_range() deletes the NAPI completely, via
napi_disable() and then __netif_napi_del(). On the next enable,
napi_enable_locked() finds no n->config and allocates a new ID:

net/core/dev.c:napi_enable_locked() {
    ...
	if (n->config)
		napi_restore_config(n);
	else
		napi_hash_add(n);
    ...
}

So the same queue gets a new napi-id after each of these:

  - ip link set down/up (veth_close()/veth_open())
  - XDP detach and re-attach (veth_xdp_set())
  - a GRO toggle without XDP (veth_set_features())
  - ethtool -L (veth_disable_range_safe()/veth_enable_range_safe())

Any per-NAPI settings applied with napi-set are dropped silently on each
of these paths. That covers defer-hard-irqs, gro-flush-timeout,
irq-suspend-timeout and threaded.

An AF_XDP copy-mode socket is affected the same way. xsk_bind() records
the ID only once:

net/xdp/xsk.c:xsk_bind() {
    ...
		if (rxq->napi)
			__sk_mark_napi_id_once(sk, rxq->napi->napi_id);
    ...
}

The xsk notifier only unbinds on NETDEV_UNREGISTER, so the socket stays
bound across a down/up and keeps the old ID. Would sk_busy_loop() then
call napi_busy_loop() with an ID that no longer resolves, so busy polling
stops without any error?

The commit message gives napi-set and AF_XDP busy polling as the
motivation. Documentation/networking/napi.rst ("Persistent NAPI config")
says "Drivers should try to use netif_napi_add_config() whenever
possible." The queue index i is already available here, and
dev->napi_config has max(num_tx_queues, num_rx_queues) entries.

The selftest added later in this series ("selftests: net: veth: test RX
queue NAPI links") checks that the IDs match napi-get after each step.
It does not check that they stay the same across these reconfigurations.

>  		rcu_assign_pointer(priv->rq[i].napi, &priv->rq[i].xdp_napi);
>  	}
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111015922.67745-1-tianyi%40cloudflare.com