netdev - Re: [PATCH net-next v4 2/3] net: implement threaded-able napi poll loop support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEA6p_BM_H=2bhYBtJ3LtBT0DBPBeVLyuC=BRQv=H3Ww2eecWA@mail.gmail.com>
Date:   Mon, 14 Dec 2020 09:59:21 -0800
From:   Wei Wang <weiwan@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Felix Fietkau <nbd@....name>, Hillf Danton <hdanton@...a.com>
Subject: Re: [PATCH net-next v4 2/3] net: implement threaded-able napi poll
 loop support

On Sat, Dec 12, 2020 at 2:55 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Sat, 12 Dec 2020 14:50:22 -0800 Jakub Kicinski wrote:
> > > @@ -6731,6 +6790,7 @@ void napi_disable(struct napi_struct *n)
> > >             msleep(1);
> > >
> > >     hrtimer_cancel(&n->timer);
> > > +   napi_kthread_stop(n);
> >
> > I'm surprised that we stop the thread on napi_disable() but there is no
> > start/create in napi_enable(). NAPIs can (and do get) disabled and
> > enabled again. But that'd make your code crash with many popular
> > drivers if you tried to change rings with threaded napi enabled so I
> > feel like I must be missing something..
>
> Ah, not crash, 'cause the flag gets cleared. Is it intentional that any
> changes that disable NAPIs cause us to go back to non-threaded NAPI?
> I think I had the "threaded" setting stored in struct netdevice in my
> patches, is there a reason not to do that?
>

Thanks for the comments!

The reason that I did not record it in dev is: there is a slight
chance that during creation of the kthreads, failures occur and we
flip back all NAPIs to use non-threaded mode. I am not sure the
recorded value in dev should be what user desires, or what the actual
situation is. Same as after the driver does a
napi_disabe()/napi_enable(). It might occur that the dev->threaded =
true, but the operation to re-create the kthreads fail and we flip
back to non-thread mode. This seems to get things more complicated.
What I expect is the user only enables the threaded mode after the
device is up and alive, with all NAPIs attached to dev, and enabled.
And user has to check the sysfs to make sure that the operation
succeeds.
And any operation that brings down the device, will flip this back to
default, which is non-threaded mode.

> In fact your patches may _require_ the device to be up to enable
> threaded NAPI if NAPIs are allocated in open.