RE: [PATCH net] net: fix race between napi kthread mode and busy poll

From: Alexander Duyck <alexanderduyck@fb.com>
To: Jakub Kicinski <kuba@kernel.org>, Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>,
	"David S . Miller" <davem@davemloft.net>,
	netdev <netdev@vger.kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	"Hannes Frederic Sowa" <hannes@stressinduktion.org>,
	Martin Zaharinov <micron10@gmail.com>
Subject: RE: [PATCH net] net: fix race between napi kthread mode and busy poll
Date: Thu, 25 Feb 2021 00:11:34 +0000	[thread overview]
Message-ID: <BN8PR15MB2787694425A1369CA563FCFFBD9E9@BN8PR15MB2787.namprd15.prod.outlook.com> (raw)
In-Reply-To: <20210224160723.4786a256@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, February 24, 2021 4:07 PM
> To: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>; David S . Miller
> <davem@davemloft.net>; netdev <netdev@vger.kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Hannes Frederic Sowa
> <hannes@stressinduktion.org>; Alexander Duyck
> <alexanderduyck@fb.com>; Martin Zaharinov <micron10@gmail.com>
> Subject: Re: [PATCH net] net: fix race between napi kthread mode and busy
> poll
> 
> On Thu, 25 Feb 2021 00:59:25 +0100 Eric Dumazet wrote:
> > On Thu, Feb 25, 2021 at 12:52 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > Interesting, vger seems to be CCed but it isn't appearing on the ML.
> > > Perhaps just a vger delay :S
> > >
> > > Not really upsetting. I'm just trying to share what I learned
> > > devising more advanced pollers. The bits get really messy really quickly.
> > > Especially that the proposed fix adds a bit for a poor bystander
> > > (busy
> > > poll) while it's the threaded IRQ that is incorrectly not preserving
> > > its ownership.
> > >
> > > > Additional 16 bytes here, possibly in a shared cache line, [1] I
> > > > prefer using a bit in hot n->state, we have plenty of them available.
> > >
> > > Right, presumably the location of the new member could be optimized.
> > > I typed this proposal up in a couple of minutes.
> > >
> > > > We worked hours with Alexander, Wei, I am sorry you think we did a
> poor job.
> > > > I really thought we instead solved the issue at hand.
> > > >
> > > > May I suggest you defer your idea of redesigning the NAPI model
> > > > for net-next ?
> > >
> > > Seems like you decided on this solution off list and now the fact
> > > that there is a discussion on the list is upsetting you. May I
> > > suggest that discussions should be conducted on list to avoid such
> situations?
> >
> > We were trying to not pollute the list (with about 40 different emails
> > so far)
> >
> > (Note this was not something I initiated, I only hit Reply all button)
> >
> > OK, I will shut up, since you seem to take over this matter, and it is
> > 1am here in France.
> 
> Are you okay with adding a SCHED_THREADED bit for threaded NAPI to be
> set in addition to SCHED? At least that way the bit is associated with it's user.
> IIUC since the extra clear_bit() in busy poll was okay so should be a new
> set_bit()?

The problem with adding a bit for SCHED_THREADED is that you would have to heavily modify napi_schedule_prep so that it would add the bit. That is the reason for going with adding the bit to the busy poll logic because it added no additional overhead. Adding another atomic bit setting operation or heavily modifying the existing one would add considerable overhead as it is either adding a complicated conditional check to all NAPI calls, or adding an atomic operation to the path for the threaded NAPI.