Message-ID: <CAAywjhQGFP-A2=F=x4ZGjcKKqTBZNJVcV1jyPYZG+1vwr1k4zw@mail.gmail.com>
Date: Thu, 31 Jul 2025 16:39:15 -0700
From: Samiullah Khawaja <skhawaja@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, willemb@...gle.com, almasrymina@...gle.com, 
	netdev@...r.kernel.org, Joe Damato <joe@...a.to>
Subject: Re: [PATCH net-next v2] net: Update threaded state in napi config in netif_set_threaded

On Wed, Jul 30, 2025 at 5:55 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 30 Jul 2025 18:25:11 +0000 Samiullah Khawaja wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 1c6e755841ce..1abba4fc1eec 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -7023,6 +7023,9 @@ int netif_set_threaded(struct net_device *dev,
> >        * This should not cause hiccups/stalls to the live traffic.
> >        */
> >       list_for_each_entry(napi, &dev->napi_list, dev_list) {
> > +             if (napi->config)
> > +                     napi->config->threaded = threaded;
> > +
> >               if (!threaded && napi->thread)
> >                       napi_stop_kthread(napi);
> >               else
>
> Could we possibly just call napi_set_threaded() instead of having the
> body of this loop? The first "if (threaded)" in napi_set_threaded()
> should do nothing.. and the rest is pretty much copy & paste.
> The barrier is not a concern, it's a control path. WDYT?
+1
>
> The test needs to be added to a Makefile, a few more comments on the
> contents..
>
> > +def enable_dev_threaded_disable_napi_threaded(cfg, nl) -> None:
> > +    """
> > +    Test that when napi threaded is enabled at device level and
> > +    then disabled at napi level for one napi, the threaded state
> > +    of all napis is preserved after a change in number of queues.
> > +    """
> > +
> > +    ip(f"link set dev {cfg.ifname} up")
>
> Why the up? Env should ifup the netdevsim for you..
Will remove
>
> > +    napis = nl.napi_get({'ifindex': cfg.ifindex}, dump=True)
> > +    ksft_eq(len(napis), 2)
>
> So.. the tests under drivers/net are supposed to be run on HW devices
> and netdevsim (both must work). Not sure if running this on real HW
> adds much value TBH, up to you if you care about that.
The addition of a test to drivers/net was suggested by you in the last
review and I sort of prefer that also as different drivers handle the
netdev resize in a different way, so having this test in drivers/net/
gives more coverage. For example the issue this patch fixes is not
reproducible with NetDevSim.
>
> If you don't move the test to net/, and use NetdevSimDev() directly.
>
> If you do let's allow any number of queues >= 2, so ksft_ge() here.
> Real drivers are unlikely to have exactly 2 queues.
>
> > +    napi0_id = napis[0]['id']
> > +    napi1_id = napis[1]['id']
> > +
> > +    threaded = cmd(f"cat /sys/class/net/{cfg.ifname}/threaded").stdout
> > +    defer(_set_threaded_state, cfg, threaded)
> > +
> > +    # set threaded
> > +    _set_threaded_state(cfg, 1)
> > +
> > +    # check napi threaded is set for both napis
> > +    _assert_napi_threaded_enabled(nl, napi0_id)
> > +    _assert_napi_threaded_enabled(nl, napi1_id)
> > +
> > +    # disable threaded for napi1
> > +    nl.napi_set({'id': napi1_id, 'threaded': 'disabled'})
> > +
> > +    cmd(f"ethtool -L {cfg.ifname} combined 1")
> > +    cmd(f"ethtool -L {cfg.ifname} combined 2")
>
> And if you decide to keep this in drivers/net it would be good to defer
> resetting the queue count to original value too. You can do (untested):
>
>         cur = ethtool(f"-l {cfg.ifname}", json=True).get("combined")
>
> or simply assume there is as many queues as there was napis earlier.
>
> And then a defer(ethtool..
+1 to restore the channel count.


>
> > +    _assert_napi_threaded_enabled(nl, napi0_id)
> > +    _assert_napi_threaded_disabled(nl, napi1_id)
> --
> pw-bot: cr

Powered by blists - more mailing lists