Message-ID: <CAAywjhTjBzU+6XqHWx=JjA89KxmaxPSuoQj7CrxQRTNGwE1vug@mail.gmail.com>
Date: Fri, 25 Apr 2025 15:52:30 -0700
From: Samiullah Khawaja <skhawaja@...gle.com>
To: Joe Damato <jdamato@...tly.com>, Samiullah Khawaja <skhawaja@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, almasrymina@...gle.com, 
	willemb@...gle.com, mkarsten@...terloo.ca, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v5] Add support to set napi threaded for
 individual napi

On Fri, Apr 25, 2025 at 3:24 PM Joe Damato <jdamato@...tly.com> wrote:
>
> On Fri, Apr 25, 2025 at 11:28:11AM -0700, Samiullah Khawaja wrote:
> > On Thu, Apr 24, 2025 at 4:13 PM Joe Damato <jdamato@...tly.com> wrote:
> > >
> > > On Wed, Apr 23, 2025 at 08:14:13PM +0000, Samiullah Khawaja wrote:
>
> [...]
>
> > > Thanks; I think this is a good change on its own separate from the
> > > rest of the series and, IMO, I think it makes it easier to get
> > > reviewed and merged.
> > Thanks for the review and suggestion to split this out.
>
> Sure. Up to you if you want to split it out or not.
I meant splitting it from the main series about busypolling.
>
> [...]
>
> > > > +
> > > >  int dev_set_threaded(struct net_device *dev, bool threaded)
> > > >  {
> > > >       struct napi_struct *napi;
> > > > @@ -7144,6 +7165,8 @@ static void napi_restore_config(struct napi_struct *n)
> > > >               napi_hash_add(n);
> > > >               n->config->napi_id = n->napi_id;
> > > >       }
> > > > +
> > > > +     napi_set_threaded(n, n->config->threaded);
> > >
> > > It makes sense to me that when restoring the config, the kthread is
> > > kicked off again (assuming config->thread > 0), but does the
> > > napi_save_config path need to stop the thread?
> >
> > >
> > > Not sure if kthread_stop is hit via some other path when
> > > napi_disable is called? Can you clarify?
> > NAPI kthread are not stopped when napi is disabled. When napis are
> > disabled, the NAPI_STATE_DISABLED state is set on them and the
> > associated thread goes to sleep after unsetting the STATE_SCHED. The
> > kthread_stop is only called on them when NAPI is deleted. This is the
> > existing behaviour. Please see napi_disable implementation for
> > reference. Also napi_enable doesn't create a new kthread and just sets
> > the napi STATE appropriately and once the NAPI is scheduled again the
> > thread is woken up. Please seem implementation of napi_schedule for
> > reference.
>
> Yea but seems:
>   - Weird from a user perspective, and
>   - Keeps the pid around as shown below even if threaded NAPI is
>     inactive, which seems weird, too.
>
> > > I ran the test and it passes for me.
> > >
> > > That said, the test is incomplete or buggy because I've manually
> > > identified 1 case that is incorrect which we discussed in the v4 and
> > > a second case that seems buggy from a user perspective.
> > >
> > > Case 1 (we discussed this in the v4, but seems like it was missed
> > > here?):
> > >
> > > Threaded set to 1 and then to 0 at the device level
> > >
> > >   echo 1 > /sys/class/net/eni28160np1/threaded
> > >   echo 0 > /sys/class/net/eni28160np1/threaded
> > >
> > > Check the setting:
> > >
> > >   cat /sys/class/net/eni28160np1/threaded
> > >   0
> > >
> > > Dump the settings for netdevsim, noting that threaded is 0, but pid
> > > is set (again, should it be?):
> > >
> > >   ./tools/net/ynl/pyynl/cli.py \
> > >        --spec Documentation/netlink/specs/netdev.yaml \
> > >        --dump napi-get --json='{"ifindex": 20}'
> > >
> > >   [{'defer-hard-irqs': 0,
> > >     'gro-flush-timeout': 0,
> > >     'id': 612,
> > >     'ifindex': 20,
> > >     'irq-suspend-timeout': 0,
> > >     'pid': 15728,
> > >     'threaded': 0},
> > >    {'defer-hard-irqs': 0,
> > >     'gro-flush-timeout': 0,
> > >     'id': 611,
> > >     'ifindex': 20,
> > >     'irq-suspend-timeout': 0,
> > >     'pid': 15729,
> > >     'threaded': 0}]
> > As explained in the comment earlier, since the kthread is still valid
> > and associated with the napi, the PID is valid. I just verified that
> > this is the existing behaviour. Not sure whether the pid should be
> > hidden if the threaded state is not enabled? Do you think we should
> > change this behaviour?
>
> I don't know, but I do think it's pretty weird from the user
> perspective.
>
> Probably need a maintainer to weigh-in on what the preferred
> behavior is. Maybe there's a reason the thread isn't killed.
+1

I think the reason behind it not being killed is because the user
might have already done some configuration using the PID and if the
kthread was removed, the user would have to do that configuration
again after enable/disable. But I am just speculating. I will let the
maintainers weigh-in as you suggested.
>
> Overall, it feels weird to me that disabling threaded NAPI leaves
> the pid in the netlink output and also keeps the thread running
> (despite being blocked).
>
> I think:
>   - If the thread is kept running then the pid probably should be
>     left in the output. Because if its hidden but still running (and
>     thus still visible in ps or top or whatever), that's even
>     stranger/more confusing?
>
>   - If the thread is killed when threaded NAPI is disabled, then the
>     pid should be wiped.
I agree. I think in the selftest I will also add PID checks just to
explicitly verify the existing behaviour is preserved.
>
> My personal preference is for the second option because it seems
> more sensible from the user's point of view, but maybe there's a
> reason it works the way it does that I don't know or understand.

Powered by blists - more mailing lists