Re: Reworking suspend-resume sequence (was: Re: PCI PM: Restore standard config registers of all devices early)

Linus Torvalds
2009-02-03T19:39:20Z

On Tue, 3 Feb 2009, Ingo Molnar wrote:
> >
> > I'm really not sure why that handle_edge_irq thing uses "ack_and_mask()"
> > instead of just "desc->chip->ack()"? I'm also totally flummoxed as to why
> > it feels it needs to go all the way out to the device to mask things,
> > instead of just masking at an apic level, which is much simpler and faster
> > (especially since masking should never happen in practice anyway).
>
> Hm, do you mean mask_ack_irq()?

Yes.

> The ->mask_ack() irqchip method is just a
> small tweak normally: if we get an irq while the irq was disabled, we can
> mark it pending and masks it for real.

No, I know why mask_ack_irq() does what it does and I agree with it. What
I was really reacting to was that handle_edge_irq() calls it at _all_.
IOW, I'm talking about this code:

handle_edge_irq(unsigned int irq, struct irq_desc *desc)
...
if (unlikely((desc->status & (IRQ_INPROGRESS | IRQ_DISABLED)) ||
!desc->action)) {
desc->status |= (IRQ_PENDING | IRQ_MASKED);
mask_ack_irq(desc, irq);
..

where the masking part seems a bit pointless. And in the case of MSI, it
causes us to go all the way out to the device, which sounds pretty
expensive too.

So if you have high enough interrupt load that you get another interrupt
on another CPU while handling one, I think the "mask_ack_irq()" likely
makes things go slower. It also causes us to have that horrible:

/*
* When another irq arrived while we were handling
* one, we could have masked the irq.
* Renable it, if it was not disabled in meantime.
*/
if (unlikely((desc->status &
(IRQ_PENDING | IRQ_MASKED | IRQ_DISABLED)) ==
(IRQ_PENDING | IRQ_MASKED))) {
desc->chip->unmask(irq);
desc->status &= ~IRQ_MASKED;
}

code there in the middle of the loop to handle the interrupt.

That function has another oddity too: it does

if (unlikely(!action)) {
desc->chip->mask(irq);
goto out_unlock;
}

but the thing is, if somebody unregistered the interrupt, then it should
have been masked to begin with. Why would be need to mask it in the
interrupt handler?

Maybe I'm missing something really subtle, but my reaction would be that
the function _should_ look just something like the appended.

But no, I'm really not going to commit this. Consider this patch to be a
question ("why does it do that?") rather than a suggestion ("we should do
this").

Linus

---
kernel/irq/chip.c | 20 +++-----------------
1 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 7de11bd..ad852ce 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -468,8 +468,8 @@ handle_edge_irq(unsigned int irq, struct irq_desc *desc)
*/
if (unlikely((desc->status & (IRQ_INPROGRESS | IRQ_DISABLED)) ||
!desc->action)) {
- desc->status |= (IRQ_PENDING | IRQ_MASKED);
- mask_ack_irq(desc, irq);
+ desc->status |= IRQ_PENDING;
+ desc->chip->ack(irq);
desc = irq_remap_to_desc(irq, desc);
goto out_unlock;
}
@@ -486,22 +486,8 @@ handle_edge_irq(unsigned int irq, struct irq_desc *desc)
struct irqaction *action = desc->action;
irqreturn_t action_ret;

- if (unlikely(!action)) {
- desc->chip->mask(irq);
+ if (unlikely(!action))
goto out_unlock;
- }
-
- /*
- * When another irq arrived while we were handling
- * one, we could have masked the irq.
- * Renable it, if it was not disabled in meantime.
- */
- if (unlikely((desc->status &
- (IRQ_PENDING | IRQ_MASKED | IRQ_DISABLED)) ==
- (IRQ_PENDING | IRQ_MASKED))) {
- desc->chip->unmask(irq);
- desc->status &= ~IRQ_MASKED;
- }

desc->status &= ~IRQ_PENDING;
spin_unlock(&desc->lock);