netdev - Re: [PATCH net RFT] bnxt_en: Fix TX timeout during netpoll.

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] [day] [month] [year] [list]

Message-ID: <CACKFLi=5PkvisHYkEK80kSzF-OwOCujkpL=mrnq-T+K0k4Ruzw@mail.gmail.com>
Date:   Tue, 25 Sep 2018 19:54:36 -0700
From:   Michael Chan <michael.chan@...adcom.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        Song Liu <songliubraving@...com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net RFT] bnxt_en: Fix TX timeout during netpoll.

On Tue, Sep 25, 2018 at 7:25 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Tue, Sep 25, 2018 at 7:15 PM Michael Chan <michael.chan@...adcom.com> wrote:
> >
> > On Tue, Sep 25, 2018 at 4:11 PM Michael Chan <michael.chan@...adcom.com> wrote:
> > >
> > > On Tue, Sep 25, 2018 at 3:15 PM Eric Dumazet <eric.dumazet@...il.com> wrote:
> > >
> > > >
> > > > It seems bnx2 should have a similar issue ?
> > > >
> > >
> > > Yes, I think so.  The MSIX mode in bnx2 is also auto-masking, meaning
> > > that MSIX will only assert once after it is ARMed.  If we return from
> > > ->poll() when budget of 0 is reached without ARMing, we may not get
> > > another MSIX.
> > >
> >
> > On second thought, I think bnx2 is ok.  If netpoll is polling on the
> > TX packets and reaching budget of 0 and returning, the INT_ACK_CMD
> > register is untouched.  bnx2 uses the status block for events and the
> > producers/consumers are cumulative.  So there is no need to ACK the
> > status block unless ARMing for interrupts.  If there is an IRQ about
> > to be fired, it won't be affected by the polling done by netpoll.
> >
> > In the case of bnxt, a completion ring is used for the events.  The
> > polling done by netpoll will cause the completion ring to be ACKed as
> > entries are processed.  ACKing the completion ring without ARMing may
> > cause future IRQs to be disabled for that ring.
>
> About bnxt : Are you sure it is all about IRQ problems ?

I'm pretty sure, because FB first reported TX timeouts followed by
ring reset failures when running netconsole.  These ring reset
failures are caused by IRQs no longer working on some rings.

>
> What if the whole ring buffer is is filled, then all entries
> are processed from netpoll.
>
> If cp_raw_cons becomes too high without the NIC knowing its (updated)
> value, maybe no IRQ can be generated anymore because
> of some wrapping issue (based on ring size)

Good point.  We have logic to handle that.  We will ACK the ring at
least once every tp->tx_wake_thresh TX packets.  But this logic fails
when the budget is 0, so I need to send a revised patch take care of
this one case.

>
> I guess that in order to test this, we would need something bursting
> 16000 messages while holding napi->poll_owner.
> The (single) IRQ would set/grab the SCHED bit but the cpu responsible
> to service this (soft)irq would spin for the whole test,
> and no more IRQ should be fired really.

Right, not easy to hit.  But it should be handled by my v2 patch.  Thanks.