Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 19 Oct 2018 22:26:22 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     David Laight <David.Laight@ACULAB.COM>
cc:     "casey.fitzpatrick@timesys.com" <casey.fitzpatrick@timesys.com>,
        "lukas@wunner.de" <lukas@wunner.de>,
        "mingo@kernel.org" <mingo@kernel.org>,
        "m.duckeck@kunbus.de" <m.duckeck@kunbus.de>,
        "hpa@zytor.com" <hpa@zytor.com>,
        "akshay.bhat@timesys.com" <akshay.bhat@timesys.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-tip-commits@vger.kernel.org" 
        <linux-tip-commits@vger.kernel.org>
Subject: RE: [tip:irq/core] genirq: Fix race on spurious interrupt
 detection
In-Reply-To: <alpine.DEB.2.21.1810192039200.1651@nanos.tec.linutronix.de>
Message-ID: <alpine.DEB.2.21.1810192221060.1651@nanos.tec.linutronix.de>
References: <1dfd8bbd16163940648045495e3e9698e63b50ad.1539867047.git.lukas@wunner.de> <tip-746a923b863a1065ef77324e1e43f19b1a3eab5c@git.kernel.org> <2bbbe6ba27424b3b83d7f22bafea13ad@AcuMS.aculab.com>
 <alpine.DEB.2.21.1810192039200.1651@nanos.tec.linutronix.de>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

David,

On Fri, 19 Oct 2018, Thomas Gleixner wrote:
> On Fri, 19 Oct 2018, David Laight wrote:
> > From: Lukas Wunner
> > > Sent: 19 October 2018 16:34
> > > 
> > > genirq: Fix race on spurious interrupt detection
> > > 
> > > Commit 1e77d0a1ed74 ("genirq: Sanitize spurious interrupt detection of
> > > threaded irqs") made detection of spurious interrupts work for threaded
> > > handlers by:
> > > 
> > > a) incrementing a counter every time the thread returns IRQ_HANDLED, and
> > > b) checking whether that counter has increased every time the thread is
> > >    woken.
> > 
> > That seems horribly broken.
> > What is it trying to achieve?
> > 
> > There are (at least) two common cases where IRQ_HANDLED doesn't get returned.
> > (Unless the driver always returns it to avoid the message.)
> > 
> > 1) The IOW that causes the hardware to drop a level sensitive IRQ is posted
> >    on the bus (etc) and happens late enough that the IRQ line is still
> >    asserted when the iret executes.
> >    If this happens all the time you need to flush the IOW, but if only
> >    occasionally it doesn't matter and you don't want a message.
> >
> > 2) Typically an ethernet driver ISR has to enable the interrupt and then
> >    check the ring for work before returning from the interrupt.
> >    If a packet arrives at this time it might be processed by the 'old'
> >    ISR invocation but still generate another interrupt.
> >    If no more packets arrive the second ISR invocation will find no work.
> >    Again this is normal behaviour.
> >    (Deferring everything with NAPI might make this not happen - but other
> >    interrupts end up working the same way.)
> > 
> > If you are really trying to detect 'stuck' interrupts then you probably
> > want to count un-handled ones and zero the count on handled ones.
> > I'm also pretty sure you don't need an atomic counter.
> 
> Care to look at the logic which handles all of this including the
> interaction with threaded interrupt handlers?

Sorry for being dense. Wanted to postpone and hit send ...

The thing is that the spurious detector works perfectly fine for the
scenarios you described since years. It does proper handled vs. unhandled
accounting.

What Lukas is addressing is not the normal interrupt case, it's the
threaded handler case where we did the accounting check after reenabling
the interrupt, which causes spurious detection to trigger
occasionally. That's not leading to disabling the interrupt unless you
enforce it with a gazillion of printks.

The atomic counter is necessary in that case because for certain scenarios
the hard interrupt handler can race with the threaded handler legitimately.

Thanks,

	tglx