[patch 0/3] [patch 0/3] genirq: Suppress resend of level interrupts

[patch 0/3] [patch 0/3] genirq: Suppress resend of level interrupts

[Thomas Gleixner]

The delayed irq disable functionality introduced a bug due to
retriggering level type interrupts in case of the delayed disable. The
resulting problem was discussed and debugged here:
http://marc.info/?l=linux-kernel&m=118202978609968&w=2

The resend of a level type interrupt is unnessecary and adds extra
noise to the system. Level type interrupts are resent by hardware when
they are still active at irq_enable(). The hard-/soft-ware resend is
only useful for edge type interrupts.

The following patch series addresses the problem and fixes an
unnoticed mismerge which affects the same area as well.

	tglx

-- 

[patch 1/3] genirq: cleanup mismerge artifact

[Thomas Gleixner]

[-- Attachment #1: genirq-do-not-resend-level-irqs.patch --]
[-- Type: text/plain, Size: 1105 bytes --]

commit 5a43a066b11ac2fe84cf67307f20b83bea390f83
genirq: Allow fasteoi handler to retrigger disabled interrupts

was erroneously applied to handle_level_irq(). This adds the irq
retrigger / resend functionality to the level irq handler.

Revert the offending bits.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/irq/chip.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Index: linux-2.6/kernel/irq/chip.c
===================================================================
--- linux-2.6.orig/kernel/irq/chip.c	2007-08-12 17:18:04.000000000 +0200
+++ linux-2.6/kernel/irq/chip.c	2007-08-12 17:18:04.000000000 +0200
@@ -352,13 +352,10 @@ handle_level_irq(unsigned int irq, struc
 	 * keep it masked and get out of here
 	 */
 	action = desc->action;
-	if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
-		desc->status |= IRQ_PENDING;
+	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
 		goto out_unlock;
-	}
 
 	desc->status |= IRQ_INPROGRESS;
-	desc->status &= ~IRQ_PENDING;
 	spin_unlock(&desc->lock);
 
 	action_ret = handle_IRQ_event(irq, action);

-- 

[patch 2/3] genirq: suppress resend of level interrupts

[Thomas Gleixner]

[-- Attachment #1: genirq-do-suppress-resend-of-level-type-interrupts.patch --]
[-- Type: text/plain, Size: 1041 bytes --]

Level type interrupts are resent by the interrupt hardware when
they are still active at irq_enable().

Suppress the resend mechanism for interrupts marked as level.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/irq/resend.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: linux-2.6/kernel/irq/resend.c
===================================================================
--- linux-2.6.orig/kernel/irq/resend.c	2007-08-12 17:18:04.000000000 +0200
+++ linux-2.6/kernel/irq/resend.c	2007-08-12 17:18:04.000000000 +0200
@@ -62,7 +62,12 @@ void check_irq_resend(struct irq_desc *d
 	 */
 	desc->chip->enable(irq);
 
-	if ((status & (IRQ_PENDING | IRQ_REPLAY)) == IRQ_PENDING) {
+	/*
+	 * We do not resend level type interrupts. Level type
+	 * interrupts are resent by hardware when they are still
+	 * active.
+	 */
+	if ((status & (IRQ_LEVEL | IRQ_PENDING | IRQ_REPLAY)) == IRQ_PENDING) {
 		desc->status = (status & ~IRQ_PENDING) | IRQ_REPLAY;
 
 		if (!desc->chip || !desc->chip->retrigger ||

-- 

[patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Thomas Gleixner]

[-- Attachment #1: i386-x86-64-mark-fasteio-interrupts-level-triggered.patch --]
[-- Type: text/plain, Size: 2087 bytes --]

Level type interrupts do not need to be resent. It was also found that
some chipsets get confused in case of the resend. 

Mark the ioapic level type interrupts as such to avoid the resend
functionality in the generic irq code.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/i386/kernel/io_apic.c   |    7 +++++--
 arch/x86_64/kernel/io_apic.c |    7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

Index: linux-2.6/arch/i386/kernel/io_apic.c
===================================================================
--- linux-2.6.orig/arch/i386/kernel/io_apic.c	2007-08-11 11:09:11.000000000 +0200
+++ linux-2.6/arch/i386/kernel/io_apic.c	2007-08-11 11:09:12.000000000 +0200
@@ -1256,12 +1256,15 @@ static struct irq_chip ioapic_chip;
 static void ioapic_register_intr(int irq, int vector, unsigned long trigger)
 {
 	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
-			trigger == IOAPIC_LEVEL)
+	    trigger == IOAPIC_LEVEL) {
+		irq_desc[irq].status |= IRQ_LEVEL;
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
 					 handle_fasteoi_irq, "fasteoi");
-	else
+	} else {
+		irq_desc[irq].status &= ~IRQ_LEVEL;
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
 					 handle_edge_irq, "edge");
+	}
 	set_intr_gate(vector, interrupt[irq]);
 }
 
Index: linux-2.6/arch/x86_64/kernel/io_apic.c
===================================================================
--- linux-2.6.orig/arch/x86_64/kernel/io_apic.c	2007-08-11 11:09:11.000000000 +0200
+++ linux-2.6/arch/x86_64/kernel/io_apic.c	2007-08-11 11:09:12.000000000 +0200
@@ -800,12 +800,15 @@ static struct irq_chip ioapic_chip;
 
 static void ioapic_register_intr(int irq, unsigned long trigger)
 {
-	if (trigger)
+	if (trigger) {
+		irq_desc[irq].status |= IRQ_LEVEL;
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
 					      handle_fasteoi_irq, "fasteoi");
-	else
+	} else {
+		irq_desc[irq].status &= ~IRQ_LEVEL;
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
 					      handle_edge_irq, "edge");
+	}
 }
 
 static void setup_IO_APIC_irq(int apic, int pin, unsigned int irq,

-- 

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Jarek Poplawski]

On Sun, Aug 12, 2007 at 03:46:36PM -0000, Thomas Gleixner wrote:
> Level type interrupts do not need to be resent. It was also found that
> some chipsets get confused in case of the resend. 

IMHO, it's still not proven the chipsets are to blame here
(plus read below).

> 
> Mark the ioapic level type interrupts as such to avoid the resend
> functionality in the generic irq code.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> ---
>  arch/i386/kernel/io_apic.c   |    7 +++++--
>  arch/x86_64/kernel/io_apic.c |    7 +++++--
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> Index: linux-2.6/arch/i386/kernel/io_apic.c
> ===================================================================
> --- linux-2.6.orig/arch/i386/kernel/io_apic.c	2007-08-11 11:09:11.000000000 +0200
> +++ linux-2.6/arch/i386/kernel/io_apic.c	2007-08-11 11:09:12.000000000 +0200
> @@ -1256,12 +1256,15 @@ static struct irq_chip ioapic_chip;
>  static void ioapic_register_intr(int irq, int vector, unsigned long trigger)
>  {
>  	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
> -			trigger == IOAPIC_LEVEL)
> +	    trigger == IOAPIC_LEVEL) {
> +		irq_desc[irq].status |= IRQ_LEVEL;
>  		set_irq_chip_and_handler_name(irq, &ioapic_chip,
>  					 handle_fasteoi_irq, "fasteoi");
> -	else
> +	} else {
> +		irq_desc[irq].status &= ~IRQ_LEVEL;
>  		set_irq_chip_and_handler_name(irq, &ioapic_chip,
>  					 handle_edge_irq, "edge");
> +	}
>  	set_intr_gate(vector, interrupt[irq]);
>  }
>  
> Index: linux-2.6/arch/x86_64/kernel/io_apic.c
> ===================================================================
> --- linux-2.6.orig/arch/x86_64/kernel/io_apic.c	2007-08-11 11:09:11.000000000 +0200
> +++ linux-2.6/arch/x86_64/kernel/io_apic.c	2007-08-11 11:09:12.000000000 +0200
> @@ -800,12 +800,15 @@ static struct irq_chip ioapic_chip;
>  
>  static void ioapic_register_intr(int irq, unsigned long trigger)
>  {
> -	if (trigger)
> +	if (trigger) {
> +		irq_desc[irq].status |= IRQ_LEVEL;
>  		set_irq_chip_and_handler_name(irq, &ioapic_chip,
>  					      handle_fasteoi_irq, "fasteoi");
> -	else
> +	} else {
> +		irq_desc[irq].status &= ~IRQ_LEVEL;
>  		set_irq_chip_and_handler_name(irq, &ioapic_chip,
>  					      handle_edge_irq, "edge");
> +	}
>  }
>  
>  static void setup_IO_APIC_irq(int apic, int pin, unsigned int irq,
> 
> -- 
> 

Maybe I miss something, but:
- why it's not done in other places with handle_level_irq or
handle_fasteoi_irq. Are we sure they can never get IRQ_PENDING flag
or we take the risk?
- what about e.g. handle_simple_irq: do we think it needs resending
or simply going to wait for good bug reports?
- is this .status cleared enough on free_irq or during request_irq?

BTW, of course something like this set of patches was needed here,
but I still think this shouldn't be done this way: the bug wasn't
diagnosed nor tested enough, some chips are suspected, and
nevertheless the change is done for everybody, whithout any
possibility to set this back for people who had no problems with
2.6.21 and 2.6.22 (at least for some transition time). Maybe some
other chips get confused now, and we get some notice of this maybe
only in 2.6.25, if we'are lucky enough to have somebody like Marcin
around to do the next git bisection?

Regards,
Jarek P.

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Thomas Gleixner]

On Mon, 2007-08-13 at 13:28 +0200, Jarek Poplawski wrote:
> Maybe I miss something, but:
> - why it's not done in other places with handle_level_irq or

I removed the PENDING bit magic in handle_level_irq, which was put there
by a mismerge.

> handle_fasteoi_irq. Are we sure they can never get IRQ_PENDING flag
> or we take the risk?

handle_fasteoi_irq is special. It is used by ioapic and weird Powerpc
hardware. We marked the ioapic level interrupts IRQ_LEVEL, so we wont
get a resend on them (see kernel/irq/resend.c)

> - what about e.g. handle_simple_irq: do we think it needs resending
> or simply going to wait for good bug reports?

Oops. I missed that one. I'll have a look at that as well.

> - is this .status cleared enough on free_irq or during request_irq?

AFAICT, there is no danger.

> BTW, of course something like this set of patches was needed here,
> but I still think this shouldn't be done this way: the bug wasn't
> diagnosed nor tested enough, some chips are suspected, and
> nevertheless the change is done for everybody, whithout any
> possibility to set this back for people who had no problems with
> 2.6.21 and 2.6.22 (at least for some transition time). 

It is quite clear what happened:

1. The retrigger/resend mechanism was never meant to be used for level
type interrupts and it makes absolutely no sense. When a level type
interrupt is masked while active it is resent by hardware on unmask when
it is still active. The resend / retrigger mechanism is only useful for
edge type interrupts, because we would lose an interrupt when we mask it
delayed without triggering a resend on unmask.

2. I found a box similar to Marcins which gets confused when level type
interrupts are resent.

> Maybe some
> other chips get confused now, and we get some notice of this maybe
> only in 2.6.25, if we'are lucky enough to have somebody like Marcin
> around to do the next git bisection?

There is nothing to confuse anymore. The resend for level type
interrupts is not happening, which is the same behavior as we had before
the delayed disable. The edge type interrupts resend is there since we
did the big genirq changes (2.6.18) and it was tested on various boxen
(including the above one) quite extensive.

The delayed disable was added later and unfortunately introduced the
problems we've seen.

	tglx

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Jarek Poplawski]

On Mon, Aug 13, 2007 at 02:07:15PM +0200, Thomas Gleixner wrote:
> On Mon, 2007-08-13 at 13:28 +0200, Jarek Poplawski wrote:
> > Maybe I miss something, but:
> > - why it's not done in other places with handle_level_irq or
> 
> I removed the PENDING bit magic in handle_level_irq, which was put there
> by a mismerge.

But, this flag is used in many "strange" places like e.g. autoprobe,
so, on any problems with this, we have to check them all...

> 
> > handle_fasteoi_irq. Are we sure they can never get IRQ_PENDING flag
> > or we take the risk?
> 
> handle_fasteoi_irq is special. It is used by ioapic and weird Powerpc
> hardware. We marked the ioapic level interrupts IRQ_LEVEL, so we wont
> get a resend on them (see kernel/irq/resend.c)

I've thought e.g. about "fasteoi" for this "Virtual Wire" timer for
i386: I hope it's OK, but since is it any problem to add some comment
here, why it's OK with resending here (with POWERPC it's easier to
think it's something special, but here we have to similar things in
the same file)?

> 
> > - what about e.g. handle_simple_irq: do we think it needs resending
> > or simply going to wait for good bug reports?
> 
> Oops. I missed that one. I'll have a look at that as well.
> 
> > - is this .status cleared enough on free_irq or during request_irq?
> 
> AFAICT, there is no danger.

Maybe no reason to risk, too.

> 
> > BTW, of course something like this set of patches was needed here,
> > but I still think this shouldn't be done this way: the bug wasn't
> > diagnosed nor tested enough, some chips are suspected, and
> > nevertheless the change is done for everybody, whithout any
> > possibility to set this back for people who had no problems with
> > 2.6.21 and 2.6.22 (at least for some transition time). 
> 
> It is quite clear what happened:
> 
> 1. The retrigger/resend mechanism was never meant to be used for level
> type interrupts and it makes absolutely no sense. When a level type
> interrupt is masked while active it is resent by hardware on unmask when
> it is still active. The resend / retrigger mechanism is only useful for
> edge type interrupts, because we would lose an interrupt when we mask it
> delayed without triggering a resend on unmask.

Of course, I think you are right with this, but:
- this patch was active for quite a long time and, if it was so wrong
there was astonishingly small number of similar problems;
- there where many changes in drivers done around similar problems,
so how can you guarantee, they will behave resonably after such serious
change again;
- this new way with levels is different from 2.6.21/22 and 2.6.20 too;
it was tested only on 2 x86_64 boxes mainly for network; it's really
not much considering the number of various quirks.

> 
> 2. I found a box similar to Marcins which gets confused when level type
> interrupts are resent.

So, we can only be sure boxes similar to Marcin's are safe now...

> 
> > Maybe some
> > other chips get confused now, and we get some notice of this maybe
> > only in 2.6.25, if we'are lucky enough to have somebody like Marcin
> > around to do the next git bisection?
> 
> There is nothing to confuse anymore. The resend for level type
> interrupts is not happening, which is the same behavior as we had before
> the delayed disable. The edge type interrupts resend is there since we
> did the big genirq changes (2.6.18) and it was tested on various boxen
> (including the above one) quite extensive.
> 
> The delayed disable was added later and unfortunately introduced the
> problems we've seen.

See my argument above... This is not 100% guarantee, and I think,
there is no reason to endanger even a small number of users/admins
for stresses like this, done to Marcin or Jean-Baptiste, when it's
possible to do this safer without much changes.

As a matter of fact I think about still another possibility, which
wasn't tested at all: let's say there is this "new" (retriggered)
interrupt possible instantly after enable_irq in a place, which
earlier wasn't affected with this. So, e.g. network _xmit or
_timeout code for some drivers can be interrupted in a place which
was always "wrong" for this, but for some reasons, not very probable
to hit (before this resending patch). Then, maybe it's all wrong
only because this level resending works OK here, but uncovers weak,
not irq-safe  places? In such a case skipping resend and software
resend should be OK: sw resend could be blocked here by softirqs off.
Of course, this can be false idea as well, but since it wasn't checked
yet (plus maybe several other possibilities) - we should be rather
cautious here.

Cheers,
Jarek P.

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Jarek Poplawski]

On Mon, Aug 13, 2007 at 03:53:11PM +0200, Jarek Poplawski wrote:
...
> I've thought e.g. about "fasteoi" for this "Virtual Wire" timer for
> i386: I hope it's OK, but since is it any problem to add some comment
> here, why it's OK with resending here (with POWERPC it's easier to
> think it's something special, but here we have to similar things in
> the same file)?
...
Sorry for these misspellings etc. (a bit in hurry...) Should be:

I've thought e.g. about "fasteoi" for this "Virtual Wire" timer for
i386: I hope it's OK, but is this any problem to add some comment
here: why it's OK with resending here (with POWERPC it's easier to
think it's something special, but here we have to similar things in
the same file)?

Jarek P.

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Thomas Gleixner]

On Mon, 2007-08-13 at 15:53 +0200, Jarek Poplawski wrote:
> On Mon, Aug 13, 2007 at 02:07:15PM +0200, Thomas Gleixner wrote:
> > On Mon, 2007-08-13 at 13:28 +0200, Jarek Poplawski wrote:
> > > Maybe I miss something, but:
> > > - why it's not done in other places with handle_level_irq or
> > 
> > I removed the PENDING bit magic in handle_level_irq, which was put there
> > by a mismerge.
> 
> But, this flag is used in many "strange" places like e.g. autoprobe,
> so, on any problems with this, we have to check them all...

No, the point is that the resend is suppressed for all interrupts which
are marked with IRQ_LEVEL:

        /*
         * We do not resend level type interrupts. Level type
         * interrupts are resent by hardware when they are still
         * active.
         */
        if ((status & (IRQ_LEVEL | IRQ_PENDING | IRQ_REPLAY)) == IRQ_PENDING) {
	....

This is not witchcraft, this is how the hardware works.

> > It is quite clear what happened:
> > 
> > 1. The retrigger/resend mechanism was never meant to be used for level
> > type interrupts and it makes absolutely no sense. When a level type
> > interrupt is masked while active it is resent by hardware on unmask when
> > it is still active. The resend / retrigger mechanism is only useful for
> > edge type interrupts, because we would lose an interrupt when we mask it
> > delayed without triggering a resend on unmask.
> 
> Of course, I think you are right with this, but:
> - this patch was active for quite a long time and, if it was so wrong
> there was astonishingly small number of similar problems;
> - there where many changes in drivers done around similar problems,
> so how can you guarantee, they will behave resonably after such serious
> change again;
> - this new way with levels is different from 2.6.21/22 and 2.6.20 too;
> it was tested only on 2 x86_64 boxes mainly for network; it's really
> not much considering the number of various quirks.

But it is not different from the original implementation before we
introduced the delayed disable, simply because before the delayed
disable change only edge type interrupts were ever resent.

> > There is nothing to confuse anymore. The resend for level type
> > interrupts is not happening, which is the same behavior as we had before
> > the delayed disable. The edge type interrupts resend is there since we
> > did the big genirq changes (2.6.18) and it was tested on various boxen
> > (including the above one) quite extensive.
> > 
> > The delayed disable was added later and unfortunately introduced the
> > problems we've seen.
> 
> See my argument above... This is not 100% guarantee, and I think,

There is no 100% guarantee for any of the proposed changes.

> there is no reason to endanger even a small number of users/admins
> for stresses like this, done to Marcin or Jean-Baptiste, when it's
> possible to do this safer without much changes.

Safer in what way ? 

The alternative patches (including my first debug patch, which got
merged during my vacation) are just pampering over the real problem
instead of fixing it.

> As a matter of fact I think about still another possibility, which
> wasn't tested at all: let's say there is this "new" (retriggered)
> interrupt possible instantly after enable_irq in a place, which
> earlier wasn't affected with this. So, e.g. network _xmit or
> _timeout code for some drivers can be interrupted in a place which
> was always "wrong" for this, but for some reasons, not very probable
> to hit (before this resending patch). Then, maybe it's all wrong
> only because this level resending works OK here, but uncovers weak,
> not irq-safe  places? In such a case skipping resend and software
> resend should be OK: sw resend could be blocked here by softirqs off.
> Of course, this can be false idea as well, but since it wasn't checked
> yet (plus maybe several other possibilities) - we should be rather
> cautious here.

If there is code which gets affected by the removal of the useless
resend of level type interrupts, then this code needs to be fixed and
not a wrong behavior preserved for no good reason.

	tglx

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Benjamin Herrenschmidt]

> handle_fasteoi_irq is special. It is used by ioapic and weird Powerpc
> hardware. We marked the ioapic level interrupts IRQ_LEVEL, so we wont
> get a resend on them (see kernel/irq/resend.c)

s/weird/sane :-)

More specifically, it's used by "intelligent" PICs that automatically do
the masking of the source (or internally handle priority levels) and
only need an "eoi" when the OS is finished processing the latest
interrupt. (On those PICs, the ack is generally an implicit part of
fetching the next pending interrupt, and the eoi consist of bringing the
current processor priority back to what it was before the interrupt
occured).

I also suspect that any hypervisor using a different method than this
one for paravirt irqs is broken :-)

Ben.

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Benjamin Herrenschmidt]

On Mon, 2007-08-13 at 15:53 +0200, Jarek Poplawski wrote:
> 
> I've thought e.g. about "fasteoi" for this "Virtual Wire" timer for
> i386: I hope it's OK, but since is it any problem to add some comment
> here, why it's OK with resending here (with POWERPC it's easier to
> think it's something special, but here we have to similar things in
> the same file)?

To be totally fair, there shouldn't be a problem with spurrious
re-sends, I'm not sure what problems some chipsets are having there,
though it may be related to the method used for re-sending. On ppc, we
use the soft method I think everywhere anyway.

I use re-sending on cell with fasteoi for the on-die top-level PIC
because while it does have a HW priority handling, it also doesn't have
a HW mask. Thus I just "drop" IRQs that are masked and resend (it's edge
messages).

Ben.

Re: [patch 0/3] [patch 0/3] genirq: Suppress resend of level interrupts

[Marcin Ślusarz]

2007/8/12, Thomas Gleixner <tglx@linutronix.de>:
> The delayed irq disable functionality introduced a bug due to
> retriggering level type interrupts in case of the delayed disable. The
> resulting problem was discussed and debugged here:
> http://marc.info/?l=linux-kernel&m=118202978609968&w=2
>
> The resend of a level type interrupt is unnessecary and adds extra
> noise to the system. Level type interrupts are resent by hardware when
> they are still active at irq_enable(). The hard-/soft-ware resend is
> only useful for edge type interrupts.
>
> The following patch series addresses the problem and fixes an
> unnoticed mismerge which affects the same area as well.
>
>         tglx
>
Works fine for me (tested on 2.6.22.1). I uploaded 6 GB of data and my
network card didn't time out. (it usually did after 1-100 MB)
Is this the final version?

Regards,
Marcin Slusarz

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Jarek Poplawski]

On Mon, Aug 13, 2007 at 06:30:20PM +0200, Thomas Gleixner wrote:
> On Mon, 2007-08-13 at 15:53 +0200, Jarek Poplawski wrote:
> > On Mon, Aug 13, 2007 at 02:07:15PM +0200, Thomas Gleixner wrote:
> > > On Mon, 2007-08-13 at 13:28 +0200, Jarek Poplawski wrote:
> > > > Maybe I miss something, but:
> > > > - why it's not done in other places with handle_level_irq or
> > > 
> > > I removed the PENDING bit magic in handle_level_irq, which was put there
> > > by a mismerge.
> > 
> > But, this flag is used in many "strange" places like e.g. autoprobe,
> > so, on any problems with this, we have to check them all...
> 
> No, the point is that the resend is suppressed for all interrupts which
> are marked with IRQ_LEVEL:
> 
>         /*
>          * We do not resend level type interrupts. Level type
>          * interrupts are resent by hardware when they are still
>          * active.
>          */
>         if ((status & (IRQ_LEVEL | IRQ_PENDING | IRQ_REPLAY)) == IRQ_PENDING) {
> 	....
> 
> This is not witchcraft, this is how the hardware works.

Sorry! It's probably something with my English: I like this flag very
much! But I simply can't find where this flag is set for any irq using
handle_level_irq, or, otherwise, can't understand why it's not set
(because in this case I don't think not setting IRQ_PENDING by the
handler should be enough).

> 
> > > It is quite clear what happened:
> > > 
> > > 1. The retrigger/resend mechanism was never meant to be used for level
> > > type interrupts and it makes absolutely no sense. When a level type
> > > interrupt is masked while active it is resent by hardware on unmask when
> > > it is still active. The resend / retrigger mechanism is only useful for
> > > edge type interrupts, because we would lose an interrupt when we mask it
> > > delayed without triggering a resend on unmask.
> > 
> > Of course, I think you are right with this, but:
> > - this patch was active for quite a long time and, if it was so wrong
> > there was astonishingly small number of similar problems;
> > - there where many changes in drivers done around similar problems,
> > so how can you guarantee, they will behave resonably after such serious
> > change again;
> > - this new way with levels is different from 2.6.21/22 and 2.6.20 too;
> > it was tested only on 2 x86_64 boxes mainly for network; it's really
> > not much considering the number of various quirks.
> 
> But it is not different from the original implementation before we
> introduced the delayed disable, simply because before the delayed
> disable change only edge type interrupts were ever resent.

It's different because e.g. for x86_64 fasteoi level type irqs were
masked during disable_irq, so there was very small probability any
irq were skipped, plus the state of io_apic was different from this
point (regarding this irq). Now it's for sure many interrupts could
be 'missing'.

BTW, of course, my knowledge of this is very limited, but I wonder
about these level type irqs used e.g. by apics. 'Normal' chips hold
some data until it's read by a driver, so there is something more
needed than an ack by io_apic. But isn't there any possibility
some level type irqs possible for IPIs or local interrupts (82489DX?)
could be missing here? Are we sure there is no hardware using level
type irqs in a similar way (drop after acking)?

> 
> > > There is nothing to confuse anymore. The resend for level type
> > > interrupts is not happening, which is the same behavior as we had before
> > > the delayed disable. The edge type interrupts resend is there since we
> > > did the big genirq changes (2.6.18) and it was tested on various boxen
> > > (including the above one) quite extensive.
> > > 
> > > The delayed disable was added later and unfortunately introduced the
> > > problems we've seen.
> > 
> > See my argument above... This is not 100% guarantee, and I think,
> 
> There is no 100% guarantee for any of the proposed changes.

That's why, sometimes, with such hard to predict changes there are
added some safety options or at least some debugging.

> 
> > there is no reason to endanger even a small number of users/admins
> > for stresses like this, done to Marcin or Jean-Baptiste, when it's
> > possible to do this safer without much changes.
> 
> Safer in what way ? 

Because, if there were a visible config option or kernel parameter
e.g.  with a comment like "legacy level irq handling - obsolete",
some people would be happy when they find it's useful for them, and
you would know about the problem much sooner, as well.

> 
> The alternative patches (including my first debug patch, which got
> merged during my vacation) are just pampering over the real problem
> instead of fixing it.
> 
> > As a matter of fact I think about still another possibility, which
> > wasn't tested at all: let's say there is this "new" (retriggered)
> > interrupt possible instantly after enable_irq in a place, which
> > earlier wasn't affected with this. So, e.g. network _xmit or
> > _timeout code for some drivers can be interrupted in a place which
> > was always "wrong" for this, but for some reasons, not very probable
> > to hit (before this resending patch). Then, maybe it's all wrong
> > only because this level resending works OK here, but uncovers weak,
> > not irq-safe  places? In such a case skipping resend and software
> > resend should be OK: sw resend could be blocked here by softirqs off.
> > Of course, this can be false idea as well, but since it wasn't checked
> > yet (plus maybe several other possibilities) - we should be rather
> > cautious here.
> 
> If there is code which gets affected by the removal of the useless
> resend of level type interrupts, then this code needs to be fixed and
> not a wrong behavior preserved for no good reason.

100% right! But this is only another example to show there could be
very complex and hard to debug problems around, even after doing
completely resonable changes in such a place.

Regards,
Jarek P.

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Jarek Poplawski]

On Mon, Aug 13, 2007 at 02:07:37PM -0500, Benjamin Herrenschmidt wrote:
> On Mon, 2007-08-13 at 15:53 +0200, Jarek Poplawski wrote:
> > 
> > I've thought e.g. about "fasteoi" for this "Virtual Wire" timer for
> > i386: I hope it's OK, but since is it any problem to add some comment
> > here, why it's OK with resending here (with POWERPC it's easier to
> > think it's something special, but here we have to similar things in
> > the same file)?

Sorry! Of course, there is something about edge in a lapic_chip
declaration, so this (plus some knowledge of the subject) should be
enough, but I was a little surprised that x86_64 uses handle_edge_irq
for probably(?) the same thing.

> 
> To be totally fair, there shouldn't be a problem with spurrious
> re-sends, I'm not sure what problems some chipsets are having there,
> though it may be related to the method used for re-sending. On ppc, we
> use the soft method I think everywhere anyway.

IMHO, this method is very doubtful if the whole system isn't adapted
for this: e.g. in networking code softirqs are blocked very often, so
such resended irq could be really late.

> 
> I use re-sending on cell with fasteoi for the on-die top-level PIC
> because while it does have a HW priority handling, it also doesn't have
> a HW mask. Thus I just "drop" IRQs that are masked and resend (it's edge
> messages).

So, if I got this right, it looks a bit easier, since you can resend with
the same type as not masked ones (fasteoi-edge?!), I guess.

Regards,
Jarek P.

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Thomas Gleixner]

On Tue, 2007-08-14 at 09:12 +0200, Jarek Poplawski wrote:
> > No, the point is that the resend is suppressed for all interrupts which
> > are marked with IRQ_LEVEL:
> > 
> >         /*
> >          * We do not resend level type interrupts. Level type
> >          * interrupts are resent by hardware when they are still
> >          * active.
> >          */
> >         if ((status & (IRQ_LEVEL | IRQ_PENDING | IRQ_REPLAY)) == IRQ_PENDING) {
> > 	....
> > 
> > This is not witchcraft, this is how the hardware works.
> 
> Sorry! It's probably something with my English: I like this flag very
> much! But I simply can't find where this flag is set for any irq using
> handle_level_irq, or, otherwise, can't understand why it's not set
> (because in this case I don't think not setting IRQ_PENDING by the
> handler should be enough).

handle_level_irq() does not set the PENDING bit on delayed disable.

> > 
> It's different because e.g. for x86_64 fasteoi level type irqs were
> masked during disable_irq, so there was very small probability any
> irq were skipped, plus the state of io_apic was different from this
> point (regarding this irq). Now it's for sure many interrupts could
> be 'missing'.

No. Let me explain:

Before delayed disable:

	irq_disable(); /* Mask in hardware */
	....
	-> Interrupt line is asserted. No interrupt due to hardware mask
	....
	irq_enable();  /* Unmask */
	
	-> When interrupt line is still active, then the interrupt is
	   invoked. Otherwise nothing happens

Delayed disable (with level fix):

	irq_disable(); /* Do not mask in hardware */
	....
	-> Interrupt line is asserted.
	---> interrupt handler is invoked: interrupt is masked in
	     hardware
	....
	irq_enable();  /* Unmask */
	
	-> When interrupt line is still active, then the interrupt is
	   invoked. Otherwise nothing happens

Can we agree, that this is the same ?

> BTW, of course, my knowledge of this is very limited, but I wonder
> about these level type irqs used e.g. by apics. 'Normal' chips hold
> some data until it's read by a driver, so there is something more
> needed than an ack by io_apic. But isn't there any possibility
> some level type irqs possible for IPIs or local interrupts (82489DX?)
> could be missing here? Are we sure there is no hardware using level
> type irqs in a similar way (drop after acking)?

Level type interrupts _are_ active as long as the hardware pin of the
interrupt line is driven by a device to the active level. 

When the hardware pin is kept at the active level by a device, the ACK
of the interrupt controller does not change the interrupt line of the
device. The interrupt comes back again immediately.

Edge type interrupts are different:

The interrupt is only triggered on the transition of the hardware pin
from inactive to active level. So the above scheme looks like:

Before delayed disable:

	irq_disable(); /* Mask in hardware */
	....
	-> Interrupt line is asserted. No interrupt due to hardware mask
	....
	irq_enable();  /* Unmask */
	
	-> The now unmasked interrupt becomes active and the interrupt
	   handler is invoked.

Delayed disable:

	irq_disable(); /* Do not mask in hardware */
	....
	-> Interrupt line is asserted.
	---> interrupt handler is invoked: interrupt is masked in
	     hardware, acknowledged and marked PENDING
	....
	irq_enable();  /* Unmask */
	
	-> On unmask the hardware does not invoke the interrupt, because
	   we acknowledged it above. Here we need to use the
	   hard-/soft-ware resend mechanism, otherwise the interrupt
	   would be lost

> > > there is no reason to endanger even a small number of users/admins
> > > for stresses like this, done to Marcin or Jean-Baptiste, when it's
> > > possible to do this safer without much changes.
> > 
> > Safer in what way ? 
> 
> Because, if there were a visible config option or kernel parameter
> e.g.  with a comment like "legacy level irq handling - obsolete",
> some people would be happy when they find it's useful for them, and
> you would know about the problem much sooner, as well.

Well, there is nothing legacy. level type interrupts do not need the
resend mechanism at all. This misfeature was introduced with the delayed
disable and went unnoticed until now.

	tglx

Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Jarek Poplawski]

On Tue, Aug 14, 2007 at 10:10:32AM +0200, Thomas Gleixner wrote:
> On Tue, 2007-08-14 at 09:12 +0200, Jarek Poplawski wrote:
> > > No, the point is that the resend is suppressed for all interrupts which
> > > are marked with IRQ_LEVEL:
> > > 
> > >         /*
> > >          * We do not resend level type interrupts. Level type
> > >          * interrupts are resent by hardware when they are still
> > >          * active.
> > >          */
> > >         if ((status & (IRQ_LEVEL | IRQ_PENDING | IRQ_REPLAY)) == IRQ_PENDING) {
> > > 	....
> > > 
> > > This is not witchcraft, this is how the hardware works.
> > 
> > Sorry! It's probably something with my English: I like this flag very
> > much! But I simply can't find where this flag is set for any irq using
> > handle_level_irq, or, otherwise, can't understand why it's not set
> > (because in this case I don't think not setting IRQ_PENDING by the
> > handler should be enough).
> 
> handle_level_irq() does not set the PENDING bit on delayed disable.
> 
> > > 
> > It's different because e.g. for x86_64 fasteoi level type irqs were
> > masked during disable_irq, so there was very small probability any
> > irq were skipped, plus the state of io_apic was different from this
> > point (regarding this irq). Now it's for sure many interrupts could
> > be 'missing'.
> 
> No. Let me explain:
> 
> Before delayed disable:
> 
> 	irq_disable(); /* Mask in hardware */
> 	....
> 	-> Interrupt line is asserted. No interrupt due to hardware mask
> 	....
> 	irq_enable();  /* Unmask */
> 	
> 	-> When interrupt line is still active, then the interrupt is
> 	   invoked. Otherwise nothing happens
> 
> Delayed disable (with level fix):
> 
> 	irq_disable(); /* Do not mask in hardware */
> 	....
> 	-> Interrupt line is asserted.
> 	---> interrupt handler is invoked: interrupt is masked in
> 	     hardware
> 	....
> 	irq_enable();  /* Unmask */
> 	
> 	-> When interrupt line is still active, then the interrupt is
> 	   invoked. Otherwise nothing happens
> 
> Can we agree, that this is the same ?

Of course, not! IMHO, far less changes can broke some drivers.

> 
> > BTW, of course, my knowledge of this is very limited, but I wonder
> > about these level type irqs used e.g. by apics. 'Normal' chips hold
> > some data until it's read by a driver, so there is something more
> > needed than an ack by io_apic. But isn't there any possibility
> > some level type irqs possible for IPIs or local interrupts (82489DX?)
> > could be missing here? Are we sure there is no hardware using level
> > type irqs in a similar way (drop after acking)?
> 
> Level type interrupts _are_ active as long as the hardware pin of the
> interrupt line is driven by a device to the active level. 
> 
> When the hardware pin is kept at the active level by a device, the ACK
> of the interrupt controller does not change the interrupt line of the
> device. The interrupt comes back again immediately.
> 
> Edge type interrupts are different:
...
Thanks for explanation. But, I'd be really glad if you could hint me
too about this level type possible sometimes e.g. for IPIs: I've
read the message needs ack (EOI) only, and hard to believe IPI is
repeated on and on after this. So, if such "thing" is level type
and hits IRQ_PENDING (or even IRQ_INPROGRESS) in handle_fasteoi_irq,
then is acked - isn't it dumped?

> > > > there is no reason to endanger even a small number of users/admins
> > > > for stresses like this, done to Marcin or Jean-Baptiste, when it's
> > > > possible to do this safer without much changes.
> > > 
> > > Safer in what way ? 
> > 
> > Because, if there were a visible config option or kernel parameter
> > e.g.  with a comment like "legacy level irq handling - obsolete",
> > some people would be happy when they find it's useful for them, and
> > you would know about the problem much sooner, as well.
> 
> Well, there is nothing legacy. level type interrupts do not need the
> resend mechanism at all. This misfeature was introduced with the delayed
> disable and went unnoticed until now.

http://dictionary.reference.com/browse/legacy
"2. anything handed down from the past [...]"

I hope, you are right, really! On the other hand we wouldn't have this
discussion at all, if right opinions were always enough.

Thanks,
Jarek P.