* Eric W. Biederman <[EMAIL PROTECTED]> wrote:
> When making the interrupt vectors per cpu I failed to handle a case
> during irq migration. If the same interrupt comes in while we are
> servicing the irq but before we migrate it the pending bit in the
> local apic IRR register will be set for that irq.
hm. I think this needs more work. For example this part of the fix looks
quite ugly to me:
> +static unsigned apic_in_service_vector(void)
> +{
> + unsigned isr, vector;
> + /* Figure out which vector we are servicing */
> + for (vector = FIRST_EXTERNAL_VECTOR; vector < FIRST_SYSTEM_VECTOR;
> vector += 32) {
> + isr = apic_read(APIC_ISR + ((vector/32) * 0x10));
> + if (isr)
> + break;
> + }
> + /* Find the low bits of the vector we are servicing */
> + vector += __ffs(isr);
> + return vector;
so we read the hardware to figure out what the hell we are doing. The
problem is - why doesnt the kernel know at this point what it is doing?
It knows the CPU and it should know all the vector numbers. It also has
an irq number.
> + /* If the irq we are servicing has moved and is not pending
> + * free it's vector.
> + */
> + irr = apic_read(APIC_IRR + ((vector/32) * 0x10));
the IRR is quite fragile. It's a rarely used hardware field and it has
erratums attached to it. Again, it seems like this function too tries to
recover information it /should/ already have.
> @@ -1400,19 +1439,24 @@ static int ioapic_retrigger_irq(unsigned int irq)
>
> static void ack_apic_edge(unsigned int irq)
> {
> - move_native_irq(irq);
> + if (unlikely(irq_desc[irq].status & IRQ_MOVE_PENDING)) {
> + move_native_irq(irq);
> + apic_handle_pending_vector(apic_in_service_vector());
> + }
> ack_APIC_irq();
this looks a bit ugly and a bit fragile. We had a simple
'move_native_irq()' call for IRQ balancing, which is straightforward,
but now we've got this complex condition open coded.
If then this should be done in some sort of helper - but even then, the
whole approach looks a bit erroneous.
To me the cleanest way to migrate an IRQ between two different vectors
on two different CPUs would be to first mask the IRQ source in the PIC,
then to do the migration /atomically/ (no intermediary state), and then
unmask. If the PIC loses edges while masked then that's a problem of the
irq chip implementation of the PIC: its ->set_affinity() method should
refuse to migrate edge-triggered IRQs if it can lose edges while
unmasked!
Ingo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
