> Linus Torvalds <torvalds@transmeta.com> writes:
>
> > On 26 Apr 1998, Claus-Justus Heine wrote:
> > >
> > > Will check tomorrow, just too tired. For ftape it will most probably
> > > work fine. Only problem could be for dunno what driver/hardware
> > > combination that disable_irq() really discards interrupts.
> >
> > Yes. There's some #ifdef code there that should fix this, but it is
>
> Yes, I saw it.
>
> Mmmh. I'm writing this with a 2.1.98 + your irq patches running. So at
> least it seems not to be completely broken.
>
> Recompilation of the kernel tree with "make -j" completed successfully.
>
> > currently disabled because there seem to be some problems with it.
THERE ARE BIG PROBLEMS with your patch. Ok, the machine doesn't
hang. But you have broken a major feature (or bug?) of the SMP irq
handling. Previously, do_ioapic_IRQ() made sure that a handler never
was reentered. This was so because do_ioapic_IRQ() didn't actually
handle the IRQ down to handle_IRQ_event() if irq_events[irq] was
different from zero, and because do_ioapic_IRQ() checked for new
pending IRQs after handle_IRQ_event() had completed (this was the
"goto again;" loop in do_ioapic_IRQ())
Now, my machine doesn't deadlock with the new IRQ code, but the
network layer is (nearly) dead with a "eth0: Re-entering the interrupt
handler" message.
One probably could change the low level handlers accordingly ... As
far as I understand it interrupts are disabled locally when an
interrupt occurs. This means the those "Re-entering interrupt"
messages are caused by an IRQ event that occurs one of the other CPUs
(ok THE other CPU in my case). So one could use a spin-locks in the
specific low level handlers.
However, the previous scheme, where do_ioapic_IRQ() repeatedly called
the IRQ handler until there were no more interrupts pending, and where
do_ioapic_IRQ() wouldn't re-enter the interrupt handler 'cause it knew
there was already another CPU busy with handling this IRQ if
irq_events[irq] was non-zero --- I think that this previous scheme was
better for performance. A spinlock would block the second CPU until
the first one would have completed with the interrupt handler. In the
previous scheme the second CPU would only mark another interrupt as
pending and exit the interrupt level, afterwards being able to either
serve other interrupts or to execute user level tasks.
So, I'd propose to not abandom the previous scheme, but -- of course
-- get rid of counting pending interrupts during a disable_irq() is
active. Simply mark one as pending and send it with the IPI, or, if
this really can't be done, abandon new interrupts for a disabled IRQ.
I'd therefore propose the following patch. Note the "#if 0" around the
send_IPI().
Could somebody shine some light on the problems with resending a
previously disabled interrupts with send_IPI()?
BTW, I have tested the following patch with and without the
send_IPI(). Basically, seems to work. But without the send_IPI() some
interrupts get lost; the latest hacker version of ftape uses
enable_irq()/disable_irq() to delay some interrupts without having to
disable interrupts globally or with a spin-lock. This _IS_ a
problem. However, I could easily change this, but would be much
happier if disable_irq()/enable_irq() would simply cause the IRQ to be
handled delayed, but be handled.
Thanks
Claus
P.s.: The patch is relative to a plain 2.1.98. If not, complain.
--- linux-2.1/arch/i386/kernel/irq.c.old Sun Apr 26 19:14:08 1998
+++ linux-2.1/arch/i386/kernel/irq.c Sun Apr 26 19:14:02 1998
@@ -673,16 +673,6 @@
set_8259A_irq_mask(irq);
}
-#ifdef __SMP__
-static void disable_ioapic_irq(unsigned int irq)
-{
- disabled_irq[irq] = 1;
- /*
- * We do not disable IO-APIC irqs in hardware ...
- */
-}
-#endif
-
void enable_8259A_irq (unsigned int irq)
{
unsigned long flags;
@@ -693,29 +683,50 @@
}
#ifdef __SMP__
+/*
+ * In the SMP+IOAPIC case it might happen that there are an unspecified
+ * number of pending IRQ events unhandled. These cases are very rare,
+ * so we 'resend' these IRQs via IPIs, to the same CPU. It's much
+ * better to do it this way as thus we dont have to be aware of
+ * 'pending' interrupts in the IRQ path, except at this point.
+ *
+ * This code appears still badly broken on some machines, and thus the
+ * send_IPI() is disabled.
+ *
+ * Note: we need to reset irq_events[irq] to zero, otherwise the IRQ
+ * _WILL_ be disabled permanently as do_ioapic_IRQ() only handles IRQs
+ * with irq_events[irq] == 0 */
+static inline void trigger_pending_irqs(unsigned int irq)
+{
+ if (irq_events[irq]) {
+ if (!ipi_pending[irq]) {
+ ipi_pending[irq] = 1;
+ irq_events[irq] = 0;
+#if 0
+ send_IPI(smp_processor_id(), IO_APIC_VECTOR(irq));
+#endif
+ }
+ }
+}
+
void enable_ioapic_irq (unsigned int irq)
{
- unsigned long flags, should_handle_irq;
- int cpu = smp_processor_id();
+ unsigned long flags;
spin_lock_irqsave(&irq_controller_lock, flags);
disabled_irq[irq] = 0;
+ trigger_pending_irqs(irq);
+
+ spin_unlock_irqrestore(&irq_controller_lock, flags);
+}
+
+static void disable_ioapic_irq(unsigned int irq)
+{
+ disabled_irq[irq] = 1;
/*
- * In the SMP+IOAPIC case it might happen that there are an unspecified
- * number of pending IRQ events unhandled. These cases are very rare,
- * so we 'resend' these IRQs via IPIs, to the same CPU. It's much
- * better to do it this way as thus we dont have to be aware of
- * 'pending' interrupts in the IRQ path, except at this point.
+ * We do not disable IO-APIC irqs in hardware ...
*/
- if (irq_events[irq]) {
- if (!ipi_pending[irq]) {
- ipi_pending[irq] = 1;
- --irq_events[irq];
- send_IPI(cpu,IO_APIC_VECTOR(irq));
- }
- }
- spin_unlock_irqrestore(&irq_controller_lock, flags);
}
#endif
@@ -778,9 +789,7 @@
ack_APIC_irq();
spin_lock(&irq_controller_lock);
- if (ipi_pending[irq])
- ipi_pending[irq] = 0;
-
+ ipi_pending[irq] = 0;
if (!irq_events[irq]++ && !disabled_irq[irq])
should_handle_irq = 1;
hardirq_enter(cpu);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu