netdev - Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 25 Sep 2018 07:43:11 -0700
From:   Michael Chan <michael.chan@...adcom.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Song Liu <songliubraving@...com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Ariel Elior <ariel.elior@...ium.com>,
        Eric Dumazet <eric.dumazet@...il.com>, tariqt@...lanox.com,
        Saeed Mahameed <saeedm@...lanox.com>,
        jeffrey.t.kirsher@...el.com,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        j.vosburgh@...il.com, vfalico@...il.com,
        Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

On Tue, Sep 25, 2018 at 7:20 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Tue, Sep 25, 2018 at 7:02 AM Michael Chan <michael.chan@...adcom.com> wrote:
> >
> > On Mon, Sep 24, 2018 at 2:18 PM Song Liu <songliubraving@...com> wrote:
> > >
> > >
> > >
> > > > On Sep 24, 2018, at 2:05 PM, Eric Dumazet <edumazet@...gle.com> wrote:
> > > >
> > > >>
> > > >> Interesting, maybe a bnxt specific issue.
> > > >>
> > > >> It seems their model is to process TX/RX notification in the same queue,
> > > >> they throw away RX events if budget == 0
> > > >>
> > > >> It means commit e7b9569102995ebc26821789628eef45bd9840d8 is wrong and
> > > >> must be reverted.
> > > >>
> > > >> Otherwise, we have a possibility of blocking a queue under netpoll pressure.
> > > >
> > > > Hmm, actually a revert might not be enough, since code at lines 2030-2031
> > > > would fire and we might not call napi_complete_done() anyway.
> > > >
> > > > Unfortunately this driver logic is quite complex.
> > > >
> > > > Could you test on other NIC eventually ?
> > > >
> > >
> > > It actually runs OK on ixgbe.
> > >
> > > @Michael, could you please help us with this?
> > >
> > I've taken a quick look using today's net tree plus Eric's
> > poll_one_napi() patch.  The problem I'm seeing is that netpoll calls
> > bnxt_poll() with budget 0.  And since work_done >= budget of 0, we
> > return without calling napi_complete_done() and without arming the
> > interrupt.  netpoll doesn't always call us back until we call
> > napi_complete_done(), right?  So I think if there are in-flight TX
> > completions, we'll miss those.
>
> That's the whole point of netpoll :
>
>  We drain the TX queues, without interrupts being involved at all,
> by calling ->napi() with a zero budget.
>
> napi_complete(), even if called from ->napi() while budget was zero,
> should do nothing but return early.
>
> budget==0 means that ->napi() should process all TX completions.

All TX completions that we can see.  We cannot see the in-flight ones.

If budget is exceeded, I think the assumption is that poll will always
be called again.

>
> So it looks like bnxt has a bug, that is showing up after the latest
> poll_one_napi() patch.
> This latest patch is needed otherwise the cpu attempting the
> netpoll-TX-drain might drain nothing at all,
> since it does not anymore call ndo_poll_controller() that was grabbing
> SCHED bits on all queues (napi_schedule() like calls)

I think the latest patch is preventing the normal interrupt -> NAPI
path from coming in and cleaning the remaining TX completions and
arming the interrupt.