in_interrupt race

in_interrupt race

[Paul Mackerras]

Rusty made the suggestion to me the other day that it would be
interesting to put some stuff in smp_processor_id() to BUG() if
preemption isn't disabled, and then see where the kernel breaks.  The
idea is that the result of smp_processor_id() is useless if preemption
is enabled, since you could move to another processor before you get
to use the result.

With that in mind, while I was poking around I noticed that
in_interrupt() uses smp_processor_id().  (This is true on all
architectures except ia64, which uses a local_cpu_data pointer
instead, and x86-64, which uses a read_pda function).

Thus if we have CONFIG_SMP and CONFIG_PREEMPT, there is a small but
non-zero probability that in_interrupt() will give the wrong answer if
it is called with preemption enabled.  If the process gets scheduled
from cpu A to cpu B between calling smp_processor_id() and evaluating
local_irq_count(cpu) or local_bh_count(), and cpu A then happens to be
in interrupt context at the point where the process resumes on cpu B,
then in_interrupt() will incorrectly return 1.

One idea I had is to use a couple of bits in
current_thread_info()->flags to indicate whether local_irq_count and
local_bh_count are non-zero for the current cpu.  These bits could be
tested safely without having to disable preemption.

In fact almost all uses of local_irq_count() and local_bh_count() are
for the current cpu; the exceptions are the irqs_running() function
and some debug printks.  Maybe the irq and bh counters themselves
could be put into the thread_info struct, if irqs_running could be
implemented another way.

Paul.

Re: in_interrupt race

[Robert Love]

On Sat, 2002-04-20 at 06:27, Paul Mackerras wrote:

> Thus if we have CONFIG_SMP and CONFIG_PREEMPT, there is a small but
> non-zero probability that in_interrupt() will give the wrong answer if
> it is called with preemption enabled.  If the process gets scheduled
> from cpu A to cpu B between calling smp_processor_id() and evaluating
> local_irq_count(cpu) or local_bh_count(), and cpu A then happens to be
> in interrupt context at the point where the process resumes on cpu B,
> then in_interrupt() will incorrectly return 1.

Looks like you are probably right ...

> One idea I had is to use a couple of bits in
> current_thread_info()->flags to indicate whether local_irq_count and
> local_bh_count are non-zero for the current cpu.  These bits could be
> tested safely without having to disable preemption.

For now we can just do this,

--- linux-2.5.8/include/asm-i386/hardirq.h	Sun Apr 14 15:18:55 2002
+++ linux/include/asm-i386/hardirq.h	Mon Apr 22 14:56:29 2002
@@ -21,8 +21,10 @@
  * Are we in an interrupt context? Either doing bottom half
  * or hardware interrupt processing?
  */
-#define in_interrupt() ({ int __cpu = smp_processor_id(); \
-	(local_irq_count(__cpu) + local_bh_count(__cpu) != 0); })
+#define in_interrupt() ({ int __cpu; preempt_disable(); \
+	__cpu = smp_processor_id(); \
+	(local_irq_count(__cpu) + local_bh_count(__cpu) != 0); \
+	preempt_enable(); })
 
 #define in_irq() (local_irq_count(smp_processor_id()) != 0)
 

Or perhaps leave the code as-is but make the rule preemption needs to be
disabled before calling (either implicitly or explicitly).  I.e., via a
call to preempt_disable or because interrupts are disabled, a lock is
held, etc ...

> In fact almost all uses of local_irq_count() and local_bh_count() are
> for the current cpu; the exceptions are the irqs_running() function
> and some debug printks.  Maybe the irq and bh counters themselves
> could be put into the thread_info struct, if irqs_running could be
> implemented another way.

One thing Linus, DaveM, and I discussed a while back was actually
getting rid of the irq and bh counts completely and folding them into
preempt_count.  I am interested in this...

	Robert Love

Re: in_interrupt race

[george anzinger]

Robert Love wrote:
> 
> On Sat, 2002-04-20 at 06:27, Paul Mackerras wrote:
> 
> > Thus if we have CONFIG_SMP and CONFIG_PREEMPT, there is a small but
> > non-zero probability that in_interrupt() will give the wrong answer if
> > it is called with preemption enabled.  If the process gets scheduled
> > from cpu A to cpu B between calling smp_processor_id() and evaluating
> > local_irq_count(cpu) or local_bh_count(), and cpu A then happens to be
> > in interrupt context at the point where the process resumes on cpu B,
> > then in_interrupt() will incorrectly return 1.
> 
> Looks like you are probably right ...
> 
> > One idea I had is to use a couple of bits in
> > current_thread_info()->flags to indicate whether local_irq_count and
> > local_bh_count are non-zero for the current cpu.  These bits could be
> > tested safely without having to disable preemption.

Preemption lock is implied by either of these being != 0, so this seems
consistant, but why not the whole counter?
> 
> For now we can just do this,
> 
> --- linux-2.5.8/include/asm-i386/hardirq.h      Sun Apr 14 15:18:55 2002
> +++ linux/include/asm-i386/hardirq.h    Mon Apr 22 14:56:29 2002
> @@ -21,8 +21,10 @@
>   * Are we in an interrupt context? Either doing bottom half
>   * or hardware interrupt processing?
>   */
> -#define in_interrupt() ({ int __cpu = smp_processor_id(); \
> -       (local_irq_count(__cpu) + local_bh_count(__cpu) != 0); })
> +#define in_interrupt() ({ int __cpu; preempt_disable(); \
> +       __cpu = smp_processor_id(); \
> +       (local_irq_count(__cpu) + local_bh_count(__cpu) != 0); \
> +       preempt_enable(); })
> 
>  #define in_irq() (local_irq_count(smp_processor_id()) != 0)
> 
> 
> Or perhaps leave the code as-is but make the rule preemption needs to be
> disabled before calling (either implicitly or explicitly).  I.e., via a
> call to preempt_disable or because interrupts are disabled, a lock is
> held, etc ...

Right, getting a consistant flag is not much use if it isn't used within
the same context.
> 
> > In fact almost all uses of local_irq_count() and local_bh_count() are
> > for the current cpu; the exceptions are the irqs_running() function
> > and some debug printks.  Maybe the irq and bh counters themselves
> > could be put into the thread_info struct, if irqs_running could be
> > implemented another way.
> 
> One thing Linus, DaveM, and I discussed a while back was actually
> getting rid of the irq and bh counts completely and folding them into
> preempt_count.  I am interested in this...

Yes.

> 
>         Robert Love
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: in_interrupt race

[Robert Love]

On Mon, 2002-04-22 at 17:39, george anzinger wrote: 

> Robert Love wrote:
> > Or perhaps leave the code as-is but make the rule preemption needs to be
> > disabled before calling (either implicitly or explicitly).  I.e., via a
> > call to preempt_disable or because interrupts are disabled, a lock is
> > held, etc ...
> 
> Right, getting a consistant flag is not much use if it isn't used within
> the same context.

Oh, wait ... someone clue me in to what I am missing, but the context
does not matter - in fact, we do not need even my fix. 

We have two cases: 

(a) we are in an interrupt or softirq, 
(b) we are not in an interrupt or softirq. 

If (a), we are not preemptible and thus do _not_ need explicit
preemption disabling.

If (b) we are preemptible, and then it does not matter what happens
during this check, since we are not preemptible and the check won't
return a false true.

Now, if we are actually using in_interrupt() as "is this exact CPU
processing an interrupt?" we may not get what we want (because we could
end up on CPU#2 from #1, and now #1 is indeed in an interrupt).  But
that is rarely (if ever?) the point of the call.

The majority, if not all, uses of in_interrupt is to see if you entered
the current code from an interrupt.  Like in schedule, "did we enter
this function off an interrupt?"  Thus, with or without preemption:

	if (in_interrupt())
		/* yep! */

will _always_ return false if your current CPU is not in an interrupt. 
This says nothing of the CPU you may of been on, but then who cares
about it?

	Robert Love

Re: in_interrupt race

[Paul Mackerras]

Robert Love writes:

> We have two cases: 
> 
> (a) we are in an interrupt or softirq, 
> (b) we are not in an interrupt or softirq. 
> 
> If (a), we are not preemptible and thus do _not_ need explicit
> preemption disabling.

True.

> If (b) we are preemptible, and then it does not matter what happens
> during this check, since we are not preemptible and the check won't
> return a false true.

Huh?  First you say we are preemptible, then you say we are not, in
the same sentence?

> Now, if we are actually using in_interrupt() as "is this exact CPU
> processing an interrupt?" we may not get what we want (because we could
> end up on CPU#2 from #1, and now #1 is indeed in an interrupt).  But
> that is rarely (if ever?) the point of the call.
> 
> The majority, if not all, uses of in_interrupt is to see if you entered
> the current code from an interrupt.  Like in schedule, "did we enter
> this function off an interrupt?"  Thus, with or without preemption:
> 
> 	if (in_interrupt())
> 		/* yep! */
> 
> will _always_ return false if your current CPU is not in an interrupt. 

No.  The point is that in_interrupt() asks two separate questions:
(1) which cpu are we on?  (2) is that cpu in interrupt context?
If we switch cpus between (1) and (2) then we can get a false positive
from in_interrupt().

> This says nothing of the CPU you may of been on, but then who cares
> about it?

We don't care about any cpu, what we want to know is whether the
current thread of execution is in process context or not.  Which is
why it is bogus for in_interrupt to need to ask which cpu we are on,
and why the local_bh_count and local_irq_count should go in the
thread_info struct IMHO.  I am working on that now. :)

Paul.

Re: in_interrupt race

[Robert Love]

On Mon, 2002-04-22 at 19:06, Paul Mackerras wrote:

> > If (b) we are preemptible, and then it does not matter what happens
> > during this check, since we are not preemptible and the check won't
> > return a false true.
> 
> Huh?  First you say we are preemptible, then you say we are not, in
> the same sentence?

Sorry - yes, we are preemptible.  I meant to say even if we do preempt,
we still end up returning false, which is all we care about.

> No.  The point is that in_interrupt() asks two separate questions:
> (1) which cpu are we on?  (2) is that cpu in interrupt context?
> If we switch cpus between (1) and (2) then we can get a false positive
> from in_interrupt().

How can we get a false positive?  Positive => we are in an interrupt. 
If we are in an interrupt, we can't preempt, so this is not an issue.

Negative => we are not in an interrupt.  Even if we do preempt, we
preempt to a CPU where we are _also_ not in an interrupt, so we get the
same answer.  In other words, if we preempt, no matter where we were or
where we end up, in_interrupt is (correctly) false.

> > This says nothing of the CPU you may of been on, but then who cares
> > about it?
> 
> We don't care about any cpu, what we want to know is whether the
> current thread of execution is in process context or not.  Which is
> why it is bogus for in_interrupt to need to ask which cpu we are on,
> and why the local_bh_count and local_irq_count should go in the
> thread_info struct IMHO.  I am working on that now. :)

Right, but I don't see a flaw (as noted previously) in the current
scheme... if preemption is a problem, then we are certainly in process
context so it is a non-issue!

	Robert Love

Re: in_interrupt race

[Paul Mackerras]

george anzinger writes:

> > On Sat, 2002-04-20 at 06:27, Paul Mackerras wrote:
> > > One idea I had is to use a couple of bits in
> > > current_thread_info()->flags to indicate whether local_irq_count and
> > > local_bh_count are non-zero for the current cpu.  These bits could be
> > > tested safely without having to disable preemption.
> 
> Preemption lock is implied by either of these being != 0, so this seems
> consistant, but why not the whole counter?

Putting local_bh_count in the thread_info struct is easy and I have
done that locally here.

Putting local_irq_count in the thread_info struct means we would have
to do irqs_running() differently.  There is a brlock allocated for
BR_GLOBALIRQ_LOCK, which is used on some architectures.  Using that
would probably help.

Paul.

Re: in_interrupt race

[Rusty Russell]

On Tue, 23 Apr 2002 09:06:31 +1000 (EST)
Paul Mackerras <paulus@samba.org> wrote:
> No.  The point is that in_interrupt() asks two separate questions:
> (1) which cpu are we on?  (2) is that cpu in interrupt context?
> If we switch cpus between (1) and (2) then we can get a false positive
> from in_interrupt().

Yes: the old CPU happens to be processing an interrupt now.
The neat solution is to follow Linus' original instinct and make
PREEMPT an option only for UP: I only like preempt because it brings
UP into line with SMP, effectively enlarging the SMP userbase to reasonable
size.

diff -urN -I \$.*\$ --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.9/arch/i386/config.in working-2.5.9-preempt/arch/i386/config.in
--- linux-2.5.9/arch/i386/config.in	Tue Apr 23 11:39:32 2002
+++ working-2.5.9-preempt/arch/i386/config.in	Tue Apr 23 13:19:58 2002
@@ -184,7 +184,6 @@
 bool 'Math emulation' CONFIG_MATH_EMULATION
 bool 'MTRR (Memory Type Range Register) support' CONFIG_MTRR
 bool 'Symmetric multi-processing support' CONFIG_SMP
-bool 'Preemptible Kernel' CONFIG_PREEMPT
 if [ "$CONFIG_SMP" != "y" ]; then
    bool 'Local APIC support on uniprocessors' CONFIG_X86_UP_APIC
    dep_bool 'IO-APIC support on uniprocessors' CONFIG_X86_UP_IOAPIC $CONFIG_X86_UP_APIC
@@ -195,6 +194,7 @@
       define_bool CONFIG_X86_IO_APIC y
    fi
 else
+   bool 'Preemptible Kernel' CONFIG_PREEMPT 
    bool 'Multiquad NUMA system' CONFIG_MULTIQUAD
 fi
 
diff -urN -I \$.*\$ --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.9/arch/sparc64/config.in working-2.5.9-preempt/arch/sparc64/config.in
--- linux-2.5.9/arch/sparc64/config.in	Thu Mar 21 14:14:42 2002
+++ working-2.5.9-preempt/arch/sparc64/config.in	Tue Apr 23 13:20:48 2002
@@ -15,7 +15,7 @@
 define_bool CONFIG_VT_CONSOLE y
 
 bool 'Symmetric multi-processing support' CONFIG_SMP
-bool 'Preemptible kernel' CONFIG_PREEMPT
+dep_bool 'Preemptible kernel' CONFIG_PREEMPT $CONFIG_SMP
 
 # Identify this as a Sparc64 build
 define_bool CONFIG_SPARC64 y
diff -urN -I \$.*\$ --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.9/arch/x86_64/config.in working-2.5.9-preempt/arch/x86_64/config.in
--- linux-2.5.9/arch/x86_64/config.in	Tue Apr 23 11:39:33 2002
+++ working-2.5.9-preempt/arch/x86_64/config.in	Tue Apr 23 13:20:59 2002
@@ -42,7 +42,7 @@
 #currently broken:
 #bool 'MTRR (Memory Type Range Register) support' CONFIG_MTRR
 bool 'Symmetric multi-processing support' CONFIG_SMP
-bool 'Preemptible Kernel' CONFIG_PREEMPT
+dep_bool 'Preemptible Kernel' CONFIG_PREEMPT $CONFIG_SMP
 if [ "$CONFIG_SMP" = "y" -a "$CONFIG_X86_CMPXCHG" = "y" ]; then
     define_bool CONFIG_HAVE_DEC_LOCK y
 fi

-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Re: in_interrupt race

[Russell King]

On Tue, Apr 23, 2002 at 01:25:24PM +1000, Rusty Russell wrote:
> Yes: the old CPU happens to be processing an interrupt now.
> The neat solution is to follow Linus' original instinct and make
> PREEMPT an option only for UP: I only like preempt because it brings
> UP into line with SMP, effectively enlarging the SMP userbase to reasonable
> size.

> -bool 'Preemptible kernel' CONFIG_PREEMPT
> +dep_bool 'Preemptible kernel' CONFIG_PREEMPT $CONFIG_SMP
> -bool 'Preemptible Kernel' CONFIG_PREEMPT
> +dep_bool 'Preemptible Kernel' CONFIG_PREEMPT $CONFIG_SMP

Do you really mean that CONFIG_PREEMPT is only available if CONFIG_SMP is
'y' or undefined?

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: in_interrupt race

[Rusty Russell]

In message <20020423093151.A17302@flint.arm.linux.org.uk> you write:
> On Tue, Apr 23, 2002 at 01:25:24PM +1000, Rusty Russell wrote:
> > Yes: the old CPU happens to be processing an interrupt now.
> > The neat solution is to follow Linus' original instinct and make
> > PREEMPT an option only for UP: I only like preempt because it brings
> > UP into line with SMP, effectively enlarging the SMP userbase to reasonable
> > size.
> 
> > -bool 'Preemptible kernel' CONFIG_PREEMPT
> > +dep_bool 'Preemptible kernel' CONFIG_PREEMPT $CONFIG_SMP
> > -bool 'Preemptible Kernel' CONFIG_PREEMPT
> > +dep_bool 'Preemptible Kernel' CONFIG_PREEMPT $CONFIG_SMP
> 
> Do you really mean that CONFIG_PREEMPT is only available if CONFIG_SMP is
> 'y' or undefined?

<sigh>... Of course that should be reversed.
if [ "$CONFIG_SMP" != y ]; then
   bool 'Preemptible Kernel' CONFIG_PREEMPT
fi

Thanks,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.