[PATCH] irqchip/jcore: fix lost per-cpu interrupts

[PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

The J-Core AIC does not have separate interrupt numbers reserved for
cpu-local vs global interrupts. Instead, the driver requesting the irq
is expected to know whether its device uses per-cpu interrupts or not.
Previously it was assumed that handle_simple_irq could work for both
cases, but it intentionally drops interrupts for an irq number that
already has a handler running. This resulted in the timer interrupt
for one cpu being lost when multiple cpus' timers were set for
approximately the same expiration time, leading to stalls. In theory
the same could also happen with IPIs.

To solve the problem, instead of registering handle_simple_irq as the
handler, register a wrapper function which checks whether the irq to
be handled was requested as per-cpu or not, and passes it to
handle_simple_irq or handle_percpu_irq accordingly.

Signed-off-by: Rich Felker <dalias@libc.org>
---

Ideas for improvement are welcome -- for example the
irq_is_percpu(irq_desc_get_irq(desc)) thing looks rather silly but I
didn't see a better way without poking through abstractions -- but
overall I think this both solves the timer stall issue that I wasted
other people's time on, and addresses the concerns about the J-Core
AIC driver being oblivious to whether an irq is per-cpu.

---
 drivers/irqchip/irq-jcore-aic.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c
index 5e5e3bb..b53a8a5 100644
--- a/drivers/irqchip/irq-jcore-aic.c
+++ b/drivers/irqchip/irq-jcore-aic.c
@@ -25,12 +25,20 @@
 
 static struct irq_chip jcore_aic;
 
+static void handle_jcore_irq(struct irq_desc *desc)
+{
+	if (irq_is_percpu(irq_desc_get_irq(desc)))
+		handle_percpu_irq(desc);
+	else
+		handle_simple_irq(desc);
+}
+
 static int jcore_aic_irqdomain_map(struct irq_domain *d, unsigned int irq,
 				   irq_hw_number_t hwirq)
 {
 	struct irq_chip *aic = d->host_data;
 
-	irq_set_chip_and_handler(irq, aic, handle_simple_irq);
+	irq_set_chip_and_handler(irq, aic, handle_jcore_irq);
 
 	return 0;
 }
-- 
2.10.0

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Thomas Gleixner]

On Sun, 9 Oct 2016, Rich Felker wrote:
> Ideas for improvement are welcome -- for example the
> irq_is_percpu(irq_desc_get_irq(desc)) thing looks rather silly but I

See the other mail.

> didn't see a better way without poking through abstractions -- but
> overall I think this both solves the timer stall issue that I wasted
> other people's time on, and addresses the concerns about the J-Core
> AIC driver being oblivious to whether an irq is per-cpu.

You could put that knowledge into the device tree so you can decide at
mapping time whether it is per cpu or not.

Thanks,

	tglx

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Thomas Gleixner]

On Sun, 9 Oct 2016, Rich Felker wrote:
> Ideas for improvement are welcome -- for example the
> irq_is_percpu(irq_desc_get_irq(desc)) thing looks rather silly but I

See the other mail.

> didn't see a better way without poking through abstractions -- but
> overall I think this both solves the timer stall issue that I wasted
> other people's time on, and addresses the concerns about the J-Core
> AIC driver being oblivious to whether an irq is per-cpu.

You could put that knowledge into the device tree so you can decide at
mapping time whether it is per cpu or not.

Thanks,

	tglx

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> On Sun, 9 Oct 2016, Rich Felker wrote:
> > Ideas for improvement are welcome -- for example the
> > irq_is_percpu(irq_desc_get_irq(desc)) thing looks rather silly but I
> 
> See the other mail.
> 
> > didn't see a better way without poking through abstractions -- but
> > overall I think this both solves the timer stall issue that I wasted
> > other people's time on, and addresses the concerns about the J-Core
> > AIC driver being oblivious to whether an irq is per-cpu.
> 
> You could put that knowledge into the device tree so you can decide at
> mapping time whether it is per cpu or not.

As discussed in the previous thread(s?) on the percpu topic, I'd
really rather avoid doing this. There's both the conceptual aspect
that it's redundant information in the DT that's only there for the
sake of making the kernel irq system happier (i.e. if there were a
code path to allow it, the handler type could be set at request_irq
time with knowledge of the flags rather than requiring the irqchip
driver to already have duplicate knowledge of this), and a couple
practical issues.

One simply relates back to redundancy -- I'd like to avoid making the
DT bigger since it's likely desirable to have the DT embedded in
internal ROM in ASICs. But the other big issue is that we already have
devices in the field using the existing DT bindings.

My preference would just be to keep the branch, but with your improved
version that doesn't need a function call:

	irqd_is_per_cpu(irq_desc_get_irq_data(desc))

While there is some overhead testing this condition every time, I can
probably come up with several better places to look for a ~10 cycle
improvement in the irq code path without imposing new requirements on
the DT bindings.

If that's really not acceptable, an alternative proposal would be a
new optional DT property that would give the irqchip driver sufficient
knowledge to bind the proper handler directly at mapping time, and
then handle_jcore_irq would be needed only in the fallback case where
this property is missing.

As noted in my followup to the clocksource stall thread, there's also
a possibility that it might make sense to consider the current
behavior of having non-percpu irqs bound to a particular cpu as part
of what's required by the compatible tag, in which case
handle_percpu_irq or something similar/equivalent might be suitable
for both the percpu and non-percpu cases. I don't understand the irq
subsystem well enough to insist on that but I think it's worth
consideration since it looks like it would improve performance of
non-percpu interrupts a bit.

Rich

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> On Sun, 9 Oct 2016, Rich Felker wrote:
> > Ideas for improvement are welcome -- for example the
> > irq_is_percpu(irq_desc_get_irq(desc)) thing looks rather silly but I
> 
> See the other mail.
> 
> > didn't see a better way without poking through abstractions -- but
> > overall I think this both solves the timer stall issue that I wasted
> > other people's time on, and addresses the concerns about the J-Core
> > AIC driver being oblivious to whether an irq is per-cpu.
> 
> You could put that knowledge into the device tree so you can decide at
> mapping time whether it is per cpu or not.

As discussed in the previous thread(s?) on the percpu topic, I'd
really rather avoid doing this. There's both the conceptual aspect
that it's redundant information in the DT that's only there for the
sake of making the kernel irq system happier (i.e. if there were a
code path to allow it, the handler type could be set at request_irq
time with knowledge of the flags rather than requiring the irqchip
driver to already have duplicate knowledge of this), and a couple
practical issues.

One simply relates back to redundancy -- I'd like to avoid making the
DT bigger since it's likely desirable to have the DT embedded in
internal ROM in ASICs. But the other big issue is that we already have
devices in the field using the existing DT bindings.

My preference would just be to keep the branch, but with your improved
version that doesn't need a function call:

	irqd_is_per_cpu(irq_desc_get_irq_data(desc))

While there is some overhead testing this condition every time, I can
probably come up with several better places to look for a ~10 cycle
improvement in the irq code path without imposing new requirements on
the DT bindings.

If that's really not acceptable, an alternative proposal would be a
new optional DT property that would give the irqchip driver sufficient
knowledge to bind the proper handler directly at mapping time, and
then handle_jcore_irq would be needed only in the fallback case where
this property is missing.

As noted in my followup to the clocksource stall thread, there's also
a possibility that it might make sense to consider the current
behavior of having non-percpu irqs bound to a particular cpu as part
of what's required by the compatible tag, in which case
handle_percpu_irq or something similar/equivalent might be suitable
for both the percpu and non-percpu cases. I don't understand the irq
subsystem well enough to insist on that but I think it's worth
consideration since it looks like it would improve performance of
non-percpu interrupts a bit.

Rich

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Thomas Gleixner]

On Sun, 9 Oct 2016, Rich Felker wrote:
> On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> My preference would just be to keep the branch, but with your improved
> version that doesn't need a function call:
> 
> 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
>
> While there is some overhead testing this condition every time, I can
> probably come up with several better places to look for a ~10 cycle
> improvement in the irq code path without imposing new requirements on
> the DT bindings.

Fair enough. Your call.
 
> As noted in my followup to the clocksource stall thread, there's also
> a possibility that it might make sense to consider the current
> behavior of having non-percpu irqs bound to a particular cpu as part
> of what's required by the compatible tag, in which case
> handle_percpu_irq or something similar/equivalent might be suitable
> for both the percpu and non-percpu cases. I don't understand the irq
> subsystem well enough to insist on that but I think it's worth
> consideration since it looks like it would improve performance of
> non-percpu interrupts a bit.

Well, you can use handle_percpu_irq() for your device interrupts if you
guarantee at the hardware level that there is no reentrancy. Once you make
the hardware capable of delivering them on either core the picture changes.

Thanks,

	tglx

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Thomas Gleixner]

On Sun, 9 Oct 2016, Rich Felker wrote:
> On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> My preference would just be to keep the branch, but with your improved
> version that doesn't need a function call:
> 
> 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
>
> While there is some overhead testing this condition every time, I can
> probably come up with several better places to look for a ~10 cycle
> improvement in the irq code path without imposing new requirements on
> the DT bindings.

Fair enough. Your call.
 
> As noted in my followup to the clocksource stall thread, there's also
> a possibility that it might make sense to consider the current
> behavior of having non-percpu irqs bound to a particular cpu as part
> of what's required by the compatible tag, in which case
> handle_percpu_irq or something similar/equivalent might be suitable
> for both the percpu and non-percpu cases. I don't understand the irq
> subsystem well enough to insist on that but I think it's worth
> consideration since it looks like it would improve performance of
> non-percpu interrupts a bit.

Well, you can use handle_percpu_irq() for your device interrupts if you
guarantee at the hardware level that there is no reentrancy. Once you make
the hardware capable of delivering them on either core the picture changes.

Thanks,

	tglx

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> On Sun, 9 Oct 2016, Rich Felker wrote:
> > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > My preference would just be to keep the branch, but with your improved
> > version that doesn't need a function call:
> > 
> > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> >
> > While there is some overhead testing this condition every time, I can
> > probably come up with several better places to look for a ~10 cycle
> > improvement in the irq code path without imposing new requirements on
> > the DT bindings.
> 
> Fair enough. Your call.

Thanks.

> > As noted in my followup to the clocksource stall thread, there's also
> > a possibility that it might make sense to consider the current
> > behavior of having non-percpu irqs bound to a particular cpu as part
> > of what's required by the compatible tag, in which case
> > handle_percpu_irq or something similar/equivalent might be suitable
> > for both the percpu and non-percpu cases. I don't understand the irq
> > subsystem well enough to insist on that but I think it's worth
> > consideration since it looks like it would improve performance of
> > non-percpu interrupts a bit.
> 
> Well, you can use handle_percpu_irq() for your device interrupts if you
> guarantee at the hardware level that there is no reentrancy.

Reentrancy is possible of course if the kernel enables irqs during the
irq handler. Is not doing so a stable part of the kernel irq
subsystem? My understanding is that modern kernels keep irqs disabled
for the full duration of (hard) irq handlers.

> Once you make
> the hardware capable of delivering them on either core the picture changes.

*nod* Perhaps if/when we do that, the path of least resistence would
be to adjust the irq numbering so that percpu (i.e., hard-routed to a
particular cpu) and global irqs (deliverable on any core) are in
different ranges and the existing kernel frameworks work.

Rich

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> On Sun, 9 Oct 2016, Rich Felker wrote:
> > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > My preference would just be to keep the branch, but with your improved
> > version that doesn't need a function call:
> > 
> > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> >
> > While there is some overhead testing this condition every time, I can
> > probably come up with several better places to look for a ~10 cycle
> > improvement in the irq code path without imposing new requirements on
> > the DT bindings.
> 
> Fair enough. Your call.

Thanks.

> > As noted in my followup to the clocksource stall thread, there's also
> > a possibility that it might make sense to consider the current
> > behavior of having non-percpu irqs bound to a particular cpu as part
> > of what's required by the compatible tag, in which case
> > handle_percpu_irq or something similar/equivalent might be suitable
> > for both the percpu and non-percpu cases. I don't understand the irq
> > subsystem well enough to insist on that but I think it's worth
> > consideration since it looks like it would improve performance of
> > non-percpu interrupts a bit.
> 
> Well, you can use handle_percpu_irq() for your device interrupts if you
> guarantee at the hardware level that there is no reentrancy.

Reentrancy is possible of course if the kernel enables irqs during the
irq handler. Is not doing so a stable part of the kernel irq
subsystem? My understanding is that modern kernels keep irqs disabled
for the full duration of (hard) irq handlers.

> Once you make
> the hardware capable of delivering them on either core the picture changes.

*nod* Perhaps if/when we do that, the path of least resistence would
be to adjust the irq numbering so that percpu (i.e., hard-routed to a
particular cpu) and global irqs (deliverable on any core) are in
different ranges and the existing kernel frameworks work.

Rich

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Thomas Gleixner]

On Sun, 9 Oct 2016, Rich Felker wrote:
> On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > Well, you can use handle_percpu_irq() for your device interrupts if you
> > guarantee at the hardware level that there is no reentrancy.
> 
> Reentrancy is possible of course if the kernel enables irqs during the
> irq handler. Is not doing so a stable part of the kernel irq
> subsystem? My understanding is that modern kernels keep irqs disabled
> for the full duration of (hard) irq handlers.

If you enable lockdep then it will yell at offenders which enable irqs in
the interrupt handler. So yes, hard irq handlers are not allowed to
reenable interrupts.
 
> > Once you make
> > the hardware capable of delivering them on either core the picture changes.
> 
> *nod* Perhaps if/when we do that, the path of least resistence would
> be to adjust the irq numbering so that percpu (i.e., hard-routed to a
> particular cpu) and global irqs (deliverable on any core) are in
> different ranges and the existing kernel frameworks work.

That's very much what ARM and others do.

Thanks,

	tglx

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Thomas Gleixner]

On Sun, 9 Oct 2016, Rich Felker wrote:
> On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > Well, you can use handle_percpu_irq() for your device interrupts if you
> > guarantee at the hardware level that there is no reentrancy.
> 
> Reentrancy is possible of course if the kernel enables irqs during the
> irq handler. Is not doing so a stable part of the kernel irq
> subsystem? My understanding is that modern kernels keep irqs disabled
> for the full duration of (hard) irq handlers.

If you enable lockdep then it will yell at offenders which enable irqs in
the interrupt handler. So yes, hard irq handlers are not allowed to
reenable interrupts.
 
> > Once you make
> > the hardware capable of delivering them on either core the picture changes.
> 
> *nod* Perhaps if/when we do that, the path of least resistence would
> be to adjust the irq numbering so that percpu (i.e., hard-routed to a
> particular cpu) and global irqs (deliverable on any core) are in
> different ranges and the existing kernel frameworks work.

That's very much what ARM and others do.

Thanks,

	tglx

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> On Sun, 9 Oct 2016, Rich Felker wrote:
> > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > My preference would just be to keep the branch, but with your improved
> > version that doesn't need a function call:
> > 
> > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> >
> > While there is some overhead testing this condition every time, I can
> > probably come up with several better places to look for a ~10 cycle
> > improvement in the irq code path without imposing new requirements on
> > the DT bindings.
> 
> Fair enough. Your call.
>  
> > As noted in my followup to the clocksource stall thread, there's also
> > a possibility that it might make sense to consider the current
> > behavior of having non-percpu irqs bound to a particular cpu as part
> > of what's required by the compatible tag, in which case
> > handle_percpu_irq or something similar/equivalent might be suitable
> > for both the percpu and non-percpu cases. I don't understand the irq
> > subsystem well enough to insist on that but I think it's worth
> > consideration since it looks like it would improve performance of
> > non-percpu interrupts a bit.
> 
> Well, you can use handle_percpu_irq() for your device interrupts if you
> guarantee at the hardware level that there is no reentrancy. Once you make
> the hardware capable of delivering them on either core the picture changes.

One more concern here -- I see that handle_simple_irq is handling the
soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
that's perhaps important too. Since soft-disable is all we have
(there's no hard-disable of interrupts), is this a problem? In other
words, can drivers have an expectation of not receiving interrupts
when the irq is disabled? I would think anything compatible with irq
sharing can't have such an expectation, but perhaps the kernel needs
disabling internally for synchronization at module-unload time or
similar cases?

If you think any of these things are problems I'll switch back to the
conditional version rather than using handle_percpu_irq for
everything.

Rich

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> On Sun, 9 Oct 2016, Rich Felker wrote:
> > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > My preference would just be to keep the branch, but with your improved
> > version that doesn't need a function call:
> > 
> > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> >
> > While there is some overhead testing this condition every time, I can
> > probably come up with several better places to look for a ~10 cycle
> > improvement in the irq code path without imposing new requirements on
> > the DT bindings.
> 
> Fair enough. Your call.
>  
> > As noted in my followup to the clocksource stall thread, there's also
> > a possibility that it might make sense to consider the current
> > behavior of having non-percpu irqs bound to a particular cpu as part
> > of what's required by the compatible tag, in which case
> > handle_percpu_irq or something similar/equivalent might be suitable
> > for both the percpu and non-percpu cases. I don't understand the irq
> > subsystem well enough to insist on that but I think it's worth
> > consideration since it looks like it would improve performance of
> > non-percpu interrupts a bit.
> 
> Well, you can use handle_percpu_irq() for your device interrupts if you
> guarantee at the hardware level that there is no reentrancy. Once you make
> the hardware capable of delivering them on either core the picture changes.

One more concern here -- I see that handle_simple_irq is handling the
soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
that's perhaps important too. Since soft-disable is all we have
(there's no hard-disable of interrupts), is this a problem? In other
words, can drivers have an expectation of not receiving interrupts
when the irq is disabled? I would think anything compatible with irq
sharing can't have such an expectation, but perhaps the kernel needs
disabling internally for synchronization at module-unload time or
similar cases?

If you think any of these things are problems I'll switch back to the
conditional version rather than using handle_percpu_irq for
everything.

Rich

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Thomas Gleixner]

On Tue, 11 Oct 2016, Rich Felker wrote:
> On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > On Sun, 9 Oct 2016, Rich Felker wrote:
> > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > > My preference would just be to keep the branch, but with your improved
> > > version that doesn't need a function call:
> > > 
> > > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> > >
> > > While there is some overhead testing this condition every time, I can
> > > probably come up with several better places to look for a ~10 cycle
> > > improvement in the irq code path without imposing new requirements on
> > > the DT bindings.
> > 
> > Fair enough. Your call.
> >  
> > > As noted in my followup to the clocksource stall thread, there's also
> > > a possibility that it might make sense to consider the current
> > > behavior of having non-percpu irqs bound to a particular cpu as part
> > > of what's required by the compatible tag, in which case
> > > handle_percpu_irq or something similar/equivalent might be suitable
> > > for both the percpu and non-percpu cases. I don't understand the irq
> > > subsystem well enough to insist on that but I think it's worth
> > > consideration since it looks like it would improve performance of
> > > non-percpu interrupts a bit.
> > 
> > Well, you can use handle_percpu_irq() for your device interrupts if you
> > guarantee at the hardware level that there is no reentrancy. Once you make
> > the hardware capable of delivering them on either core the picture changes.
> 
> One more concern here -- I see that handle_simple_irq is handling the
> soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
> that's perhaps important too. Since soft-disable is all we have
> (there's no hard-disable of interrupts), is this a problem? In other
> words, can drivers have an expectation of not receiving interrupts
> when the irq is disabled? I would think anything compatible with irq
> sharing can't have such an expectation, but perhaps the kernel needs
> disabling internally for synchronization at module-unload time or
> similar cases?

Sure. A driver would be surprised getting an interrupt when it is disabled,
but with your exceptionally well thought out interrupt controller a pending
(level) interrupt which is not handled will be reraised forever and just
hard lock the machine.

> If you think any of these things are problems I'll switch back to the
> conditional version rather than using handle_percpu_irq for
> everything.

It might be the approach of least surprise, but it won't make a difference
for the situation described above.

Thanks,

	tglx

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Thomas Gleixner]

On Tue, 11 Oct 2016, Rich Felker wrote:
> On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > On Sun, 9 Oct 2016, Rich Felker wrote:
> > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > > My preference would just be to keep the branch, but with your improved
> > > version that doesn't need a function call:
> > > 
> > > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> > >
> > > While there is some overhead testing this condition every time, I can
> > > probably come up with several better places to look for a ~10 cycle
> > > improvement in the irq code path without imposing new requirements on
> > > the DT bindings.
> > 
> > Fair enough. Your call.
> >  
> > > As noted in my followup to the clocksource stall thread, there's also
> > > a possibility that it might make sense to consider the current
> > > behavior of having non-percpu irqs bound to a particular cpu as part
> > > of what's required by the compatible tag, in which case
> > > handle_percpu_irq or something similar/equivalent might be suitable
> > > for both the percpu and non-percpu cases. I don't understand the irq
> > > subsystem well enough to insist on that but I think it's worth
> > > consideration since it looks like it would improve performance of
> > > non-percpu interrupts a bit.
> > 
> > Well, you can use handle_percpu_irq() for your device interrupts if you
> > guarantee at the hardware level that there is no reentrancy. Once you make
> > the hardware capable of delivering them on either core the picture changes.
> 
> One more concern here -- I see that handle_simple_irq is handling the
> soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
> that's perhaps important too. Since soft-disable is all we have
> (there's no hard-disable of interrupts), is this a problem? In other
> words, can drivers have an expectation of not receiving interrupts
> when the irq is disabled? I would think anything compatible with irq
> sharing can't have such an expectation, but perhaps the kernel needs
> disabling internally for synchronization at module-unload time or
> similar cases?

Sure. A driver would be surprised getting an interrupt when it is disabled,
but with your exceptionally well thought out interrupt controller a pending
(level) interrupt which is not handled will be reraised forever and just
hard lock the machine.

> If you think any of these things are problems I'll switch back to the
> conditional version rather than using handle_percpu_irq for
> everything.

It might be the approach of least surprise, but it won't make a difference
for the situation described above.

Thanks,

	tglx

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote:
> On Tue, 11 Oct 2016, Rich Felker wrote:
> > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > > On Sun, 9 Oct 2016, Rich Felker wrote:
> > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > > > My preference would just be to keep the branch, but with your improved
> > > > version that doesn't need a function call:
> > > > 
> > > > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> > > >
> > > > While there is some overhead testing this condition every time, I can
> > > > probably come up with several better places to look for a ~10 cycle
> > > > improvement in the irq code path without imposing new requirements on
> > > > the DT bindings.
> > > 
> > > Fair enough. Your call.
> > >  
> > > > As noted in my followup to the clocksource stall thread, there's also
> > > > a possibility that it might make sense to consider the current
> > > > behavior of having non-percpu irqs bound to a particular cpu as part
> > > > of what's required by the compatible tag, in which case
> > > > handle_percpu_irq or something similar/equivalent might be suitable
> > > > for both the percpu and non-percpu cases. I don't understand the irq
> > > > subsystem well enough to insist on that but I think it's worth
> > > > consideration since it looks like it would improve performance of
> > > > non-percpu interrupts a bit.
> > > 
> > > Well, you can use handle_percpu_irq() for your device interrupts if you
> > > guarantee at the hardware level that there is no reentrancy. Once you make
> > > the hardware capable of delivering them on either core the picture changes.
> > 
> > One more concern here -- I see that handle_simple_irq is handling the
> > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
> > that's perhaps important too. Since soft-disable is all we have
> > (there's no hard-disable of interrupts), is this a problem? In other
> > words, can drivers have an expectation of not receiving interrupts
> > when the irq is disabled? I would think anything compatible with irq
> > sharing can't have such an expectation, but perhaps the kernel needs
> > disabling internally for synchronization at module-unload time or
> > similar cases?
> 
> Sure. A driver would be surprised getting an interrupt when it is disabled,
> but with your exceptionally well thought out interrupt controller a pending
> (level) interrupt which is not handled will be reraised forever and just
> hard lock the machine.

If you want to criticize the interrupt controller design (not my work
or under my control) for limitations in the type of hardware that can
be hooked up to it, that's okay -- this kind of input will actually be
useful for designing the next iteration of it -- but I don't think
this specific possibility is a concern. The controller is designed for
SoC-internal use with devices that behave well, and not for level
interrupts that require device-specific action to clear (the clearing
of pending status is non-device-specific and takes place at the time
the interrupt is accepted by the cpu). It might end up being that it
makes sense to keep the AIC2 as-is but attach a separate, more
general-purpose global interrupt cuntroller that can route to any
cpu's AIC and that's friendlier to diverse external hardware (like the
type of level interrupts you described) but the hardware team would
know better than me.

> > If you think any of these things are problems I'll switch back to the
> > conditional version rather than using handle_percpu_irq for
> > everything.
> 
> It might be the approach of least surprise, but it won't make a difference
> for the situation described above.

I'm not seeing any easily measurable performance difference with the
version using the conditional, so I'm going to submit that as a v3.
Whether or not there's actually a safety concern, I'm not sure, but
I'd rather use the functions the way they were intended to be used so
we don't have to worry about unexpected bugs or regressions if the
internals change.

Rich

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote:
> On Tue, 11 Oct 2016, Rich Felker wrote:
> > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > > On Sun, 9 Oct 2016, Rich Felker wrote:
> > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > > > My preference would just be to keep the branch, but with your improved
> > > > version that doesn't need a function call:
> > > > 
> > > > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> > > >
> > > > While there is some overhead testing this condition every time, I can
> > > > probably come up with several better places to look for a ~10 cycle
> > > > improvement in the irq code path without imposing new requirements on
> > > > the DT bindings.
> > > 
> > > Fair enough. Your call.
> > >  
> > > > As noted in my followup to the clocksource stall thread, there's also
> > > > a possibility that it might make sense to consider the current
> > > > behavior of having non-percpu irqs bound to a particular cpu as part
> > > > of what's required by the compatible tag, in which case
> > > > handle_percpu_irq or something similar/equivalent might be suitable
> > > > for both the percpu and non-percpu cases. I don't understand the irq
> > > > subsystem well enough to insist on that but I think it's worth
> > > > consideration since it looks like it would improve performance of
> > > > non-percpu interrupts a bit.
> > > 
> > > Well, you can use handle_percpu_irq() for your device interrupts if you
> > > guarantee at the hardware level that there is no reentrancy. Once you make
> > > the hardware capable of delivering them on either core the picture changes.
> > 
> > One more concern here -- I see that handle_simple_irq is handling the
> > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
> > that's perhaps important too. Since soft-disable is all we have
> > (there's no hard-disable of interrupts), is this a problem? In other
> > words, can drivers have an expectation of not receiving interrupts
> > when the irq is disabled? I would think anything compatible with irq
> > sharing can't have such an expectation, but perhaps the kernel needs
> > disabling internally for synchronization at module-unload time or
> > similar cases?
> 
> Sure. A driver would be surprised getting an interrupt when it is disabled,
> but with your exceptionally well thought out interrupt controller a pending
> (level) interrupt which is not handled will be reraised forever and just
> hard lock the machine.

If you want to criticize the interrupt controller design (not my work
or under my control) for limitations in the type of hardware that can
be hooked up to it, that's okay -- this kind of input will actually be
useful for designing the next iteration of it -- but I don't think
this specific possibility is a concern. The controller is designed for
SoC-internal use with devices that behave well, and not for level
interrupts that require device-specific action to clear (the clearing
of pending status is non-device-specific and takes place at the time
the interrupt is accepted by the cpu). It might end up being that it
makes sense to keep the AIC2 as-is but attach a separate, more
general-purpose global interrupt cuntroller that can route to any
cpu's AIC and that's friendlier to diverse external hardware (like the
type of level interrupts you described) but the hardware team would
know better than me.

> > If you think any of these things are problems I'll switch back to the
> > conditional version rather than using handle_percpu_irq for
> > everything.
> 
> It might be the approach of least surprise, but it won't make a difference
> for the situation described above.

I'm not seeing any easily measurable performance difference with the
version using the conditional, so I'm going to submit that as a v3.
Whether or not there's actually a safety concern, I'm not sure, but
I'd rather use the functions the way they were intended to be used so
we don't have to worry about unexpected bugs or regressions if the
internals change.

Rich

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Paul E. McKenney]

On Wed, Oct 12, 2016 at 12:35:43PM -0400, Rich Felker wrote:
> On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote:
> > On Tue, 11 Oct 2016, Rich Felker wrote:
> > > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > > > On Sun, 9 Oct 2016, Rich Felker wrote:
> > > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > > > > My preference would just be to keep the branch, but with your improved
> > > > > version that doesn't need a function call:
> > > > > 
> > > > > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> > > > >
> > > > > While there is some overhead testing this condition every time, I can
> > > > > probably come up with several better places to look for a ~10 cycle
> > > > > improvement in the irq code path without imposing new requirements on
> > > > > the DT bindings.
> > > > 
> > > > Fair enough. Your call.
> > > >  
> > > > > As noted in my followup to the clocksource stall thread, there's also
> > > > > a possibility that it might make sense to consider the current
> > > > > behavior of having non-percpu irqs bound to a particular cpu as part
> > > > > of what's required by the compatible tag, in which case
> > > > > handle_percpu_irq or something similar/equivalent might be suitable
> > > > > for both the percpu and non-percpu cases. I don't understand the irq
> > > > > subsystem well enough to insist on that but I think it's worth
> > > > > consideration since it looks like it would improve performance of
> > > > > non-percpu interrupts a bit.
> > > > 
> > > > Well, you can use handle_percpu_irq() for your device interrupts if you
> > > > guarantee at the hardware level that there is no reentrancy. Once you make
> > > > the hardware capable of delivering them on either core the picture changes.
> > > 
> > > One more concern here -- I see that handle_simple_irq is handling the
> > > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
> > > that's perhaps important too. Since soft-disable is all we have
> > > (there's no hard-disable of interrupts), is this a problem? In other
> > > words, can drivers have an expectation of not receiving interrupts
> > > when the irq is disabled? I would think anything compatible with irq
> > > sharing can't have such an expectation, but perhaps the kernel needs
> > > disabling internally for synchronization at module-unload time or
> > > similar cases?
> > 
> > Sure. A driver would be surprised getting an interrupt when it is disabled,
> > but with your exceptionally well thought out interrupt controller a pending
> > (level) interrupt which is not handled will be reraised forever and just
> > hard lock the machine.
> 
> If you want to criticize the interrupt controller design (not my work
> or under my control) for limitations in the type of hardware that can
> be hooked up to it, that's okay -- this kind of input will actually be
> useful for designing the next iteration of it -- but I don't think
> this specific possibility is a concern.

Well, if this scenario does happen, the machine will likely either lock
up silently and hard, give you RCU CPU stall warning messages, or give
you soft-lockup messages.

							Thanx, Paul

>                                         The controller is designed for
> SoC-internal use with devices that behave well, and not for level
> interrupts that require device-specific action to clear (the clearing
> of pending status is non-device-specific and takes place at the time
> the interrupt is accepted by the cpu). It might end up being that it
> makes sense to keep the AIC2 as-is but attach a separate, more
> general-purpose global interrupt cuntroller that can route to any
> cpu's AIC and that's friendlier to diverse external hardware (like the
> type of level interrupts you described) but the hardware team would
> know better than me.
> 
> > > If you think any of these things are problems I'll switch back to the
> > > conditional version rather than using handle_percpu_irq for
> > > everything.
> > 
> > It might be the approach of least surprise, but it won't make a difference
> > for the situation described above.
> 
> I'm not seeing any easily measurable performance difference with the
> version using the conditional, so I'm going to submit that as a v3.
> Whether or not there's actually a safety concern, I'm not sure, but
> I'd rather use the functions the way they were intended to be used so
> we don't have to worry about unexpected bugs or regressions if the
> internals change.
> 
> Rich
> 

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Paul E. McKenney]

On Wed, Oct 12, 2016 at 12:35:43PM -0400, Rich Felker wrote:
> On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote:
> > On Tue, 11 Oct 2016, Rich Felker wrote:
> > > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > > > On Sun, 9 Oct 2016, Rich Felker wrote:
> > > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > > > > My preference would just be to keep the branch, but with your improved
> > > > > version that doesn't need a function call:
> > > > > 
> > > > > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> > > > >
> > > > > While there is some overhead testing this condition every time, I can
> > > > > probably come up with several better places to look for a ~10 cycle
> > > > > improvement in the irq code path without imposing new requirements on
> > > > > the DT bindings.
> > > > 
> > > > Fair enough. Your call.
> > > >  
> > > > > As noted in my followup to the clocksource stall thread, there's also
> > > > > a possibility that it might make sense to consider the current
> > > > > behavior of having non-percpu irqs bound to a particular cpu as part
> > > > > of what's required by the compatible tag, in which case
> > > > > handle_percpu_irq or something similar/equivalent might be suitable
> > > > > for both the percpu and non-percpu cases. I don't understand the irq
> > > > > subsystem well enough to insist on that but I think it's worth
> > > > > consideration since it looks like it would improve performance of
> > > > > non-percpu interrupts a bit.
> > > > 
> > > > Well, you can use handle_percpu_irq() for your device interrupts if you
> > > > guarantee at the hardware level that there is no reentrancy. Once you make
> > > > the hardware capable of delivering them on either core the picture changes.
> > > 
> > > One more concern here -- I see that handle_simple_irq is handling the
> > > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
> > > that's perhaps important too. Since soft-disable is all we have
> > > (there's no hard-disable of interrupts), is this a problem? In other
> > > words, can drivers have an expectation of not receiving interrupts
> > > when the irq is disabled? I would think anything compatible with irq
> > > sharing can't have such an expectation, but perhaps the kernel needs
> > > disabling internally for synchronization at module-unload time or
> > > similar cases?
> > 
> > Sure. A driver would be surprised getting an interrupt when it is disabled,
> > but with your exceptionally well thought out interrupt controller a pending
> > (level) interrupt which is not handled will be reraised forever and just
> > hard lock the machine.
> 
> If you want to criticize the interrupt controller design (not my work
> or under my control) for limitations in the type of hardware that can
> be hooked up to it, that's okay -- this kind of input will actually be
> useful for designing the next iteration of it -- but I don't think
> this specific possibility is a concern.

Well, if this scenario does happen, the machine will likely either lock
up silently and hard, give you RCU CPU stall warning messages, or give
you soft-lockup messages.

							Thanx, Paul

>                                         The controller is designed for
> SoC-internal use with devices that behave well, and not for level
> interrupts that require device-specific action to clear (the clearing
> of pending status is non-device-specific and takes place at the time
> the interrupt is accepted by the cpu). It might end up being that it
> makes sense to keep the AIC2 as-is but attach a separate, more
> general-purpose global interrupt cuntroller that can route to any
> cpu's AIC and that's friendlier to diverse external hardware (like the
> type of level interrupts you described) but the hardware team would
> know better than me.
> 
> > > If you think any of these things are problems I'll switch back to the
> > > conditional version rather than using handle_percpu_irq for
> > > everything.
> > 
> > It might be the approach of least surprise, but it won't make a difference
> > for the situation described above.
> 
> I'm not seeing any easily measurable performance difference with the
> version using the conditional, so I'm going to submit that as a v3.
> Whether or not there's actually a safety concern, I'm not sure, but
> I'd rather use the functions the way they were intended to be used so
> we don't have to worry about unexpected bugs or regressions if the
> internals change.
> 
> Rich
> 

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

On Wed, Oct 12, 2016 at 01:34:17PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 12, 2016 at 12:35:43PM -0400, Rich Felker wrote:
> > On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote:
> > > On Tue, 11 Oct 2016, Rich Felker wrote:
> > > > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > > > > On Sun, 9 Oct 2016, Rich Felker wrote:
> > > > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > > > > > My preference would just be to keep the branch, but with your improved
> > > > > > version that doesn't need a function call:
> > > > > > 
> > > > > > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> > > > > >
> > > > > > While there is some overhead testing this condition every time, I can
> > > > > > probably come up with several better places to look for a ~10 cycle
> > > > > > improvement in the irq code path without imposing new requirements on
> > > > > > the DT bindings.
> > > > > 
> > > > > Fair enough. Your call.
> > > > >  
> > > > > > As noted in my followup to the clocksource stall thread, there's also
> > > > > > a possibility that it might make sense to consider the current
> > > > > > behavior of having non-percpu irqs bound to a particular cpu as part
> > > > > > of what's required by the compatible tag, in which case
> > > > > > handle_percpu_irq or something similar/equivalent might be suitable
> > > > > > for both the percpu and non-percpu cases. I don't understand the irq
> > > > > > subsystem well enough to insist on that but I think it's worth
> > > > > > consideration since it looks like it would improve performance of
> > > > > > non-percpu interrupts a bit.
> > > > > 
> > > > > Well, you can use handle_percpu_irq() for your device interrupts if you
> > > > > guarantee at the hardware level that there is no reentrancy. Once you make
> > > > > the hardware capable of delivering them on either core the picture changes.
> > > > 
> > > > One more concern here -- I see that handle_simple_irq is handling the
> > > > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
> > > > that's perhaps important too. Since soft-disable is all we have
> > > > (there's no hard-disable of interrupts), is this a problem? In other
> > > > words, can drivers have an expectation of not receiving interrupts
> > > > when the irq is disabled? I would think anything compatible with irq
> > > > sharing can't have such an expectation, but perhaps the kernel needs
> > > > disabling internally for synchronization at module-unload time or
> > > > similar cases?
> > > 
> > > Sure. A driver would be surprised getting an interrupt when it is disabled,
> > > but with your exceptionally well thought out interrupt controller a pending
> > > (level) interrupt which is not handled will be reraised forever and just
> > > hard lock the machine.
> > 
> > If you want to criticize the interrupt controller design (not my work
> > or under my control) for limitations in the type of hardware that can
> > be hooked up to it, that's okay -- this kind of input will actually be
> > useful for designing the next iteration of it -- but I don't think
> > this specific possibility is a concern.
> 
> Well, if this scenario does happen, the machine will likely either lock
> up silently and hard, give you RCU CPU stall warning messages, or give
> you soft-lockup messages.

The same situation can happen with badly-behaved hardware under
software interrupt control too if it keeps generating interrupts
rapidly (more quickly than the cpu can handle them), unless the kernel
has some kind of framework for disabling the interrupt and only
reenabling it later via a timer. It's equivalent to a realtime-prio
process failing to block/sleep to give lower-priority processes a
chance to run.

Rich

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Rich Felker]

On Wed, Oct 12, 2016 at 01:34:17PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 12, 2016 at 12:35:43PM -0400, Rich Felker wrote:
> > On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote:
> > > On Tue, 11 Oct 2016, Rich Felker wrote:
> > > > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > > > > On Sun, 9 Oct 2016, Rich Felker wrote:
> > > > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > > > > > My preference would just be to keep the branch, but with your improved
> > > > > > version that doesn't need a function call:
> > > > > > 
> > > > > > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> > > > > >
> > > > > > While there is some overhead testing this condition every time, I can
> > > > > > probably come up with several better places to look for a ~10 cycle
> > > > > > improvement in the irq code path without imposing new requirements on
> > > > > > the DT bindings.
> > > > > 
> > > > > Fair enough. Your call.
> > > > >  
> > > > > > As noted in my followup to the clocksource stall thread, there's also
> > > > > > a possibility that it might make sense to consider the current
> > > > > > behavior of having non-percpu irqs bound to a particular cpu as part
> > > > > > of what's required by the compatible tag, in which case
> > > > > > handle_percpu_irq or something similar/equivalent might be suitable
> > > > > > for both the percpu and non-percpu cases. I don't understand the irq
> > > > > > subsystem well enough to insist on that but I think it's worth
> > > > > > consideration since it looks like it would improve performance of
> > > > > > non-percpu interrupts a bit.
> > > > > 
> > > > > Well, you can use handle_percpu_irq() for your device interrupts if you
> > > > > guarantee at the hardware level that there is no reentrancy. Once you make
> > > > > the hardware capable of delivering them on either core the picture changes.
> > > > 
> > > > One more concern here -- I see that handle_simple_irq is handling the
> > > > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
> > > > that's perhaps important too. Since soft-disable is all we have
> > > > (there's no hard-disable of interrupts), is this a problem? In other
> > > > words, can drivers have an expectation of not receiving interrupts
> > > > when the irq is disabled? I would think anything compatible with irq
> > > > sharing can't have such an expectation, but perhaps the kernel needs
> > > > disabling internally for synchronization at module-unload time or
> > > > similar cases?
> > > 
> > > Sure. A driver would be surprised getting an interrupt when it is disabled,
> > > but with your exceptionally well thought out interrupt controller a pending
> > > (level) interrupt which is not handled will be reraised forever and just
> > > hard lock the machine.
> > 
> > If you want to criticize the interrupt controller design (not my work
> > or under my control) for limitations in the type of hardware that can
> > be hooked up to it, that's okay -- this kind of input will actually be
> > useful for designing the next iteration of it -- but I don't think
> > this specific possibility is a concern.
> 
> Well, if this scenario does happen, the machine will likely either lock
> up silently and hard, give you RCU CPU stall warning messages, or give
> you soft-lockup messages.

The same situation can happen with badly-behaved hardware under
software interrupt control too if it keeps generating interrupts
rapidly (more quickly than the cpu can handle them), unless the kernel
has some kind of framework for disabling the interrupt and only
reenabling it later via a timer. It's equivalent to a realtime-prio
process failing to block/sleep to give lower-priority processes a
chance to run.

Rich

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Paul E. McKenney]

On Wed, Oct 12, 2016 at 06:19:27PM -0400, Rich Felker wrote:
> On Wed, Oct 12, 2016 at 01:34:17PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 12, 2016 at 12:35:43PM -0400, Rich Felker wrote:
> > > On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote:
> > > > On Tue, 11 Oct 2016, Rich Felker wrote:
> > > > > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > > > > > On Sun, 9 Oct 2016, Rich Felker wrote:
> > > > > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > > > > > > My preference would just be to keep the branch, but with your improved
> > > > > > > version that doesn't need a function call:
> > > > > > > 
> > > > > > > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> > > > > > >
> > > > > > > While there is some overhead testing this condition every time, I can
> > > > > > > probably come up with several better places to look for a ~10 cycle
> > > > > > > improvement in the irq code path without imposing new requirements on
> > > > > > > the DT bindings.
> > > > > > 
> > > > > > Fair enough. Your call.
> > > > > >  
> > > > > > > As noted in my followup to the clocksource stall thread, there's also
> > > > > > > a possibility that it might make sense to consider the current
> > > > > > > behavior of having non-percpu irqs bound to a particular cpu as part
> > > > > > > of what's required by the compatible tag, in which case
> > > > > > > handle_percpu_irq or something similar/equivalent might be suitable
> > > > > > > for both the percpu and non-percpu cases. I don't understand the irq
> > > > > > > subsystem well enough to insist on that but I think it's worth
> > > > > > > consideration since it looks like it would improve performance of
> > > > > > > non-percpu interrupts a bit.
> > > > > > 
> > > > > > Well, you can use handle_percpu_irq() for your device interrupts if you
> > > > > > guarantee at the hardware level that there is no reentrancy. Once you make
> > > > > > the hardware capable of delivering them on either core the picture changes.
> > > > > 
> > > > > One more concern here -- I see that handle_simple_irq is handling the
> > > > > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
> > > > > that's perhaps important too. Since soft-disable is all we have
> > > > > (there's no hard-disable of interrupts), is this a problem? In other
> > > > > words, can drivers have an expectation of not receiving interrupts
> > > > > when the irq is disabled? I would think anything compatible with irq
> > > > > sharing can't have such an expectation, but perhaps the kernel needs
> > > > > disabling internally for synchronization at module-unload time or
> > > > > similar cases?
> > > > 
> > > > Sure. A driver would be surprised getting an interrupt when it is disabled,
> > > > but with your exceptionally well thought out interrupt controller a pending
> > > > (level) interrupt which is not handled will be reraised forever and just
> > > > hard lock the machine.
> > > 
> > > If you want to criticize the interrupt controller design (not my work
> > > or under my control) for limitations in the type of hardware that can
> > > be hooked up to it, that's okay -- this kind of input will actually be
> > > useful for designing the next iteration of it -- but I don't think
> > > this specific possibility is a concern.
> > 
> > Well, if this scenario does happen, the machine will likely either lock
> > up silently and hard, give you RCU CPU stall warning messages, or give
> > you soft-lockup messages.
> 
> The same situation can happen with badly-behaved hardware under
> software interrupt control too if it keeps generating interrupts
> rapidly (more quickly than the cpu can handle them), unless the kernel
> has some kind of framework for disabling the interrupt and only
> reenabling it later via a timer. It's equivalent to a realtime-prio
> process failing to block/sleep to give lower-priority processes a
> chance to run.

Indeed, there are quite a few scenarios that can lead to silent hard
lockups, RCU CPU stall warning messages, or soft-lockup messages.

							Thanx, Paul

Re: [PATCH] irqchip/jcore: fix lost per-cpu interrupts

[Paul E. McKenney]

On Wed, Oct 12, 2016 at 06:19:27PM -0400, Rich Felker wrote:
> On Wed, Oct 12, 2016 at 01:34:17PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 12, 2016 at 12:35:43PM -0400, Rich Felker wrote:
> > > On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote:
> > > > On Tue, 11 Oct 2016, Rich Felker wrote:
> > > > > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote:
> > > > > > On Sun, 9 Oct 2016, Rich Felker wrote:
> > > > > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote:
> > > > > > > My preference would just be to keep the branch, but with your improved
> > > > > > > version that doesn't need a function call:
> > > > > > > 
> > > > > > > 	irqd_is_per_cpu(irq_desc_get_irq_data(desc))
> > > > > > >
> > > > > > > While there is some overhead testing this condition every time, I can
> > > > > > > probably come up with several better places to look for a ~10 cycle
> > > > > > > improvement in the irq code path without imposing new requirements on
> > > > > > > the DT bindings.
> > > > > > 
> > > > > > Fair enough. Your call.
> > > > > >  
> > > > > > > As noted in my followup to the clocksource stall thread, there's also
> > > > > > > a possibility that it might make sense to consider the current
> > > > > > > behavior of having non-percpu irqs bound to a particular cpu as part
> > > > > > > of what's required by the compatible tag, in which case
> > > > > > > handle_percpu_irq or something similar/equivalent might be suitable
> > > > > > > for both the percpu and non-percpu cases. I don't understand the irq
> > > > > > > subsystem well enough to insist on that but I think it's worth
> > > > > > > consideration since it looks like it would improve performance of
> > > > > > > non-percpu interrupts a bit.
> > > > > > 
> > > > > > Well, you can use handle_percpu_irq() for your device interrupts if you
> > > > > > guarantee at the hardware level that there is no reentrancy. Once you make
> > > > > > the hardware capable of delivering them on either core the picture changes.
> > > > > 
> > > > > One more concern here -- I see that handle_simple_irq is handling the
> > > > > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff
> > > > > that's perhaps important too. Since soft-disable is all we have
> > > > > (there's no hard-disable of interrupts), is this a problem? In other
> > > > > words, can drivers have an expectation of not receiving interrupts
> > > > > when the irq is disabled? I would think anything compatible with irq
> > > > > sharing can't have such an expectation, but perhaps the kernel needs
> > > > > disabling internally for synchronization at module-unload time or
> > > > > similar cases?
> > > > 
> > > > Sure. A driver would be surprised getting an interrupt when it is disabled,
> > > > but with your exceptionally well thought out interrupt controller a pending
> > > > (level) interrupt which is not handled will be reraised forever and just
> > > > hard lock the machine.
> > > 
> > > If you want to criticize the interrupt controller design (not my work
> > > or under my control) for limitations in the type of hardware that can
> > > be hooked up to it, that's okay -- this kind of input will actually be
> > > useful for designing the next iteration of it -- but I don't think
> > > this specific possibility is a concern.
> > 
> > Well, if this scenario does happen, the machine will likely either lock
> > up silently and hard, give you RCU CPU stall warning messages, or give
> > you soft-lockup messages.
> 
> The same situation can happen with badly-behaved hardware under
> software interrupt control too if it keeps generating interrupts
> rapidly (more quickly than the cpu can handle them), unless the kernel
> has some kind of framework for disabling the interrupt and only
> reenabling it later via a timer. It's equivalent to a realtime-prio
> process failing to block/sleep to give lower-priority processes a
> chance to run.

Indeed, there are quite a few scenarios that can lead to silent hard
lockups, RCU CPU stall warning messages, or soft-lockup messages.

							Thanx, Paul