Re: [PATCH net-next v2 1/3] veth: refactor NAPI enable paths

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch changes the order of NAPI setup in
  veth_napi_enable_range() (drivers/net/veth.c).

--

veth: refactor NAPI enable paths

This patch stops passing rq->xdp_napi.napi_id to xdp_rxq_info_reg(). It
moves netif_napi_add() out of veth_enable_xdp_range() and the old
veth_napi_enable_range() wrapper and into the loop in
__veth_napi_enable_range() that enables each NAPI. The trivial wrappers
are removed, and the double-underscore helpers take over their names.

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 0539b3661793d..f5097696df15e 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c

[ ... ]

> @@ -1059,6 +1059,7 @@ static int __veth_napi_enable_range(struct net_device *dev, int start, int end)
>  	for (i = start; i < end; i++) {
>  		struct veth_rq *rq = &priv->rq[i];
>  
> +		netif_napi_add(dev, &rq->xdp_napi, veth_poll);
>  		rcu_assign_pointer(rq->xdp_prog, priv->_xdp_prog);
>  		napi_enable(&rq->xdp_napi);

[Severity: Low]
Can this leave a veth device with threaded NAPI half on and half off?

Before this patch, every caller added all NAPIs in the range before it
enabled any of them. The core seems to depend on that order.
netif_napi_add_weight_locked() in net/core/dev.c says:

	/* Create kthread for this napi if dev->threaded is set.
	 * Clear dev->threaded if kthread creation failed so that
	 * threaded mode will not be enabled in napi_enable().
	 */
	if (napi_get_threaded_config(dev, napi))
		if (napi_kthread_create(napi))
			dev->threaded = NETDEV_NAPI_THREADED_DISABLED;

napi_enable_locked() reads dev->threaded at the time it runs:

	if (n->dev->threaded && n->thread)
		new |= NAPIF_STATE_THREADED;

With add and enable now in the same loop, suppose threaded mode is on and
napi_kthread_create() fails for some queue k > start. Queues start..k-1
have already been enabled with NAPIF_STATE_THREADED. Queue k and the
queues after it come up non-threaded, and dev->threaded is cleared.

The device then reports threaded mode as disabled through sysfs and
netlink, while some of its NAPIs are still polled by kthreads. This can
happen in any path that reaches veth_napi_enable_range():
veth_set_features() toggling GRO, veth_set_channels() calling
veth_enable_range_safe(), veth_open(), and veth_enable_xdp(). The
kthread_run() failure can come from ENOMEM or from a pending fatal
signal.

netif_set_threaded() avoids this state by reverting all NAPIs when
kthread creation fails, but this path doesn't do that.

Could netif_napi_add() stay in its own loop over the whole range? It
could sit after the page_pool and ptr_ring allocations that can fail and
before the napi_enable() loop. That would keep the new error handling
and the old ordering.

[ ... ]

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