i386 IPI handlers running with hardirq_count == 0

i386 IPI handlers running with hardirq_count == 0

[Keith Owens]

Macro arch/i386/kernel/entry.S::BUILD_INTERRUPT generates the code to
handle an IPI and call the corresponding smp_<name> C code.
BUILD_INTERRUPT does not update the hardirq_count for the interrupted
task, that is left to the C code.  Some of the C IPI handlers do not
call irq_enter(), so they are running in IRQ context but the
hardirq_count field does not reflect this.  For example,
smp_invalidate_interrupt does not set the hardirq count.

What is the best fix, change BUILD_INTERRUPT to adjust the hardirq
count or audit all the C handlers to ensure that they call irq_enter()?

Re: i386 IPI handlers running with hardirq_count == 0

[Andrew Morton]

On Thu, 29 Jun 2006 19:01:17 +1000
Keith Owens <kaos@ocs.com.au> wrote:

> Macro arch/i386/kernel/entry.S::BUILD_INTERRUPT generates the code to
> handle an IPI and call the corresponding smp_<name> C code.
> BUILD_INTERRUPT does not update the hardirq_count for the interrupted
> task, that is left to the C code.  Some of the C IPI handlers do not
> call irq_enter(), so they are running in IRQ context but the
> hardirq_count field does not reflect this.  For example,
> smp_invalidate_interrupt does not set the hardirq count.
> 
> What is the best fix, change BUILD_INTERRUPT to adjust the hardirq
> count or audit all the C handlers to ensure that they call irq_enter()?
> 

The IPI handlers run with IRQs disabled.  Do we need a fix?

Re: i386 IPI handlers running with hardirq_count == 0

[Andi Kleen]

Andrew Morton <akpm@osdl.org> writes:

> On Thu, 29 Jun 2006 19:01:17 +1000
> Keith Owens <kaos@ocs.com.au> wrote:
> 
> > Macro arch/i386/kernel/entry.S::BUILD_INTERRUPT generates the code to
> > handle an IPI and call the corresponding smp_<name> C code.
> > BUILD_INTERRUPT does not update the hardirq_count for the interrupted
> > task, that is left to the C code.  Some of the C IPI handlers do not
> > call irq_enter(), so they are running in IRQ context but the
> > hardirq_count field does not reflect this.  For example,
> > smp_invalidate_interrupt does not set the hardirq count.
> > 
> > What is the best fix, change BUILD_INTERRUPT to adjust the hardirq
> > count or audit all the C handlers to ensure that they call irq_enter()?
> > 
> 
> The IPI handlers run with IRQs disabled.  Do we need a fix?

They have to because if there was another interrupt it would execute
IRET and then clear the NMI flag in the hardware and allow nested NMIs 
which would cause all sorts of problems.

The only reason to change it would be complex callbacks in the
current handlers using notifier chains. Maybe if they're that complex they 
should become simpler? 

-Andi

Re: i386 IPI handlers running with hardirq_count == 0

[Keith Owens]

Andi Kleen (on 29 Jun 2006 13:25:38 +0200) wrote:
>Andrew Morton <akpm@osdl.org> writes:
>
>> On Thu, 29 Jun 2006 19:01:17 +1000
>> Keith Owens <kaos@ocs.com.au> wrote:
>> 
>> > Macro arch/i386/kernel/entry.S::BUILD_INTERRUPT generates the code to
>> > handle an IPI and call the corresponding smp_<name> C code.
>> > BUILD_INTERRUPT does not update the hardirq_count for the interrupted
>> > task, that is left to the C code.  Some of the C IPI handlers do not
>> > call irq_enter(), so they are running in IRQ context but the
>> > hardirq_count field does not reflect this.  For example,
>> > smp_invalidate_interrupt does not set the hardirq count.
>> > 
>> > What is the best fix, change BUILD_INTERRUPT to adjust the hardirq
>> > count or audit all the C handlers to ensure that they call irq_enter()?
>> > 
>> 
>> The IPI handlers run with IRQs disabled.  Do we need a fix?
>
>They have to because if there was another interrupt it would execute
>IRET and then clear the NMI flag in the hardware and allow nested NMIs 
>which would cause all sorts of problems.
>
>The only reason to change it would be complex callbacks in the
>current handlers using notifier chains. Maybe if they're that complex they 
>should become simpler? 

My question has nothing to do with NMI.  I am querying inconsistent
behaviour amongst normal IPIs, this list :-

i386 function                   irq_enter?
smp_apic_timer_interrupt           yes
smp_call_function_interrupt        yes
smp_error_interrupt                yes
smp_invalidate_interrupt           no - why
smp_reschedule_interrupt           no (does not need it)
smp_spurious_interrupt             yes
smp_thermal_interrupt              yes

x86_64 function                 irq_enter?
mce_threshold_interrupt            yes
smp_apic_timer_interrupt           yes
smp_call_function_interrupt        yes
smp_error_interrupt                yes
smp_invalidate_interrupt           no - why
smp_reschedule_interrupt           no (does not need it)
smp_spurious_interrupt             yes
smp_thermal_interrupt              yes

That is just the mach-default list, I have not checked the platforms
like voyager.

Re: i386 IPI handlers running with hardirq_count == 0

[Ingo Molnar]

* Keith Owens <kaos@ocs.com.au> wrote:

> My question has nothing to do with NMI.  I am querying inconsistent 
> behaviour amongst normal IPIs, this list :-
> 
> i386 function                   irq_enter?
> smp_apic_timer_interrupt           yes
> smp_call_function_interrupt        yes
> smp_error_interrupt                yes
> smp_invalidate_interrupt           no - why
> smp_reschedule_interrupt           no (does not need it)
> smp_spurious_interrupt             yes
> smp_thermal_interrupt              yes
> 
> x86_64 function                 irq_enter?
> mce_threshold_interrupt            yes
> smp_apic_timer_interrupt           yes
> smp_call_function_interrupt        yes
> smp_error_interrupt                yes
> smp_invalidate_interrupt           no - why
> smp_reschedule_interrupt           no (does not need it)
> smp_spurious_interrupt             yes
> smp_thermal_interrupt              yes

irq_enter() is mostly just for the purpose of in_interrupt()/in_irq() to 
work as expected, not much else. [also the timer code assumes that 
update_process_times() is called in a HARDIRQ_OFFSET elevated context, 
so the apic timer IRQ needs irq_enter() too.] The 
smp_invalidate_interrupt() and smp_reschedule_interrupt() is 
performance-critical and they dont need irq_enter()/irq_exit().

Since smp_call_function_interrupt() can be called with driver-supplied 
function vectors, it's best to keep the irq_enter()/exit there. [for 
example mm/slab.c has some in_interrupt() sanity checks.] Obviously 
do_IRQ() itself needs irq_enter()/exit() too - plus the APIC timer irq 
as mentioned above.
 
Otherwise, the rest of the SMP functions technically dont need 
irq_enter()/irq_exit(). [i.e. threshold, error, spurious and thermal] We 
could remove it from them.

	Ingo

Re: i386 IPI handlers running with hardirq_count == 0

[Andrew Morton]

Ingo Molnar <mingo@elte.hu> wrote:
>
> 
> * Keith Owens <kaos@ocs.com.au> wrote:
> 
> > My question has nothing to do with NMI.  I am querying inconsistent 
> > behaviour amongst normal IPIs, this list :-
> > 
> > i386 function                   irq_enter?
> > smp_apic_timer_interrupt           yes
> > smp_call_function_interrupt        yes
> > smp_error_interrupt                yes
> > smp_invalidate_interrupt           no - why
> > smp_reschedule_interrupt           no (does not need it)
> > smp_spurious_interrupt             yes
> > smp_thermal_interrupt              yes
> > 
> > x86_64 function                 irq_enter?
> > mce_threshold_interrupt            yes
> > smp_apic_timer_interrupt           yes
> > smp_call_function_interrupt        yes
> > smp_error_interrupt                yes
> > smp_invalidate_interrupt           no - why
> > smp_reschedule_interrupt           no (does not need it)
> > smp_spurious_interrupt             yes
> > smp_thermal_interrupt              yes
> 
> irq_enter() is mostly just for the purpose of in_interrupt()/in_irq() to 
> work as expected, not much else. [also the timer code assumes that 
> update_process_times() is called in a HARDIRQ_OFFSET elevated context, 
> so the apic timer IRQ needs irq_enter() too.] The 
> smp_invalidate_interrupt() and smp_reschedule_interrupt() is 
> performance-critical and they dont need irq_enter()/irq_exit().
> 
> Since smp_call_function_interrupt() can be called with driver-supplied 
> function vectors, it's best to keep the irq_enter()/exit there. [for 
> example mm/slab.c has some in_interrupt() sanity checks.] Obviously 
> do_IRQ() itself needs irq_enter()/exit() too - plus the APIC timer irq 
> as mentioned above.
>  
> Otherwise, the rest of the SMP functions technically dont need 
> irq_enter()/irq_exit(). [i.e. threshold, error, spurious and thermal] We 
> could remove it from them.
> 

There's a risk that spin_unlock() in an IPI handler could blow up due to it
trying to reschedule.  But preempt_schedule() explicitly checks the CPU's
interupt flag so as long as that doesn't change we're OK.

Re: i386 IPI handlers running with hardirq_count == 0

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> There's a risk that spin_unlock() in an IPI handler could blow up due 
> to it trying to reschedule.  But preempt_schedule() explicitly checks 
> the CPU's interupt flag so as long as that doesn't change we're OK.

yes. Enabling hardirqs in an IPI handler would be a quite bad idea 
anyway, they are all quite short.

	Ingo

Re: i386 IPI handlers running with hardirq_count == 0

[Keith Owens]

Andrew Morton (on Thu, 29 Jun 2006 02:18:00 -0700) wrote:
>On Thu, 29 Jun 2006 19:01:17 +1000
>Keith Owens <kaos@ocs.com.au> wrote:
>
>> Macro arch/i386/kernel/entry.S::BUILD_INTERRUPT generates the code to
>> handle an IPI and call the corresponding smp_<name> C code.
>> BUILD_INTERRUPT does not update the hardirq_count for the interrupted
>> task, that is left to the C code.  Some of the C IPI handlers do not
>> call irq_enter(), so they are running in IRQ context but the
>> hardirq_count field does not reflect this.  For example,
>> smp_invalidate_interrupt does not set the hardirq count.
>> 
>> What is the best fix, change BUILD_INTERRUPT to adjust the hardirq
>> count or audit all the C handlers to ensure that they call irq_enter()?
>> 
>
>The IPI handlers run with IRQs disabled.  Do we need a fix?

Some IPI handlers issue irq_enter() to bump hard_irq_count, some IPI
handlers do not.  It is inconsistent, with no obvious reason for doing
it either way.  All the external irqs go via do_IRQ which does issue
irq_enter().  I guess that my real question is why are some IPIs not
using irq_enter()?  The lack of consistency concerns me.