Re: [patch] softirq-2.4.5-B0

Re: [patch] softirq-2.4.5-B0

[Andi Kleen]

"David S. Miller" <davem@redhat.com> writes:

>  > the attached softirq-2.4.5-B0 patch fixes this problem by calling
>  > do_softirq()  from local_bh_enable() [if the bh count is 0, to avoid
>  > recursion].
> 
> Yikes!  I do not like this fix.
> 
> I'd rather local_bh_enable() not become a more heavy primitive.
> 
> I know, in one respect it makes sense because it parallels how
> hardware interrupts work, but not this thing is a function call
> instead of a counter bump :-(
> 
> Any other ideas how to zap this?

I think the right way to fix it is to do a final atomic check for softirqs 
when the kernel is exited. To be atomic this check neededs to be done with 
interrupts off until the kernel exited. I just implemented a very similar way 
into the x86-64 kernel (which needs to turn off interrupts for kernel anyways 
for other reasons, this just extends the cli area a bit). The same atomic
check is needed for race free check of need_schedule (which is still racy
on plain i386). 

-Andi

Re: [patch] softirq-2.4.5-B0

[Ingo Molnar]

On 27 May 2001, Andi Kleen wrote:

> I think the right way to fix it is to do a final atomic check for
> softirqs when the kernel is exited. To be atomic this check neededs to
> be done with interrupts off until the kernel exited. [...]

check out my first softirq patch, it does exactly this. (but has the wrong
explanation.)

> [...] The same atomic check is needed for race free check of
> need_schedule (which is still racy on plain i386).

right, this need_resched race is fixed in the lowlatency patchset.

	Ingo

Re: [patch] severe softirq handling performance bug, fix, 2.4.5

[David S. Miller]

Ingo Molnar writes:
 > (unlike bottom halves, soft-IRQs do not preempt kernel code.)
 ...

Since when do we have this rule? :-)

 > the two error cases are:
 > 
 >  #1 hard-IRQ interrupts user-space code, activates softirq, and returns to
 >     user-space code
 > 
 >  #2 hard-IRQ interrupts the idle task, activates softirq and returns to
 >     the idle task.
 > 
 > category #1 is easy to fix, in entry.S we have to check active softirqs
 > not only the exception and ret-from-syscall cases, but also in the
 > IRQ-ret-to-userspace case.
 > 
 > category #2 is subtle, because the idle process is kernel code, so
 > returning to it we do not execute active softirqs. The main two types of
 > idle handlers both had a window do 'miss' softirq execution:

Ingo, I don't think this is the fix.

You should check Softirqs on return from every single IRQ.
In do_softirq() it will make sure that we won't run softirqs
while already doing so or being already nested in a hard-IRQ.

Every port works this way, I don't know where you got this "soft-IRQs
cannot run when returning to kernel code" rule, it simply doesn't
exist.

And looking at the x86 code, I don't even understand how your fixes
can make a difference, what about the do_softirq() call in
arch/i386/kernel/irq.c:do_IRQ()???  That should be taking care of all
these "error cases" right?

Later,
David S. Miller
davem@redhat.com

[patch] softirq-2.4.5-B0

[Ingo Molnar]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1531 bytes --]


On Sat, 26 May 2001, David S. Miller wrote:

> And looking at the x86 code, I don't even understand how your fixes
> can make a difference, what about the do_softirq() call in
> arch/i386/kernel/irq.c:do_IRQ()??? [...]

[you are right, it's a brain fart on my part. doh. i guess i was too happy
having fixed the longstanding latency problem.]

the TCP latency issues and the missed softirq execution bug is still
there, but for a slightly different reason.

the bug/misbehavior causing bad latencies turned out to be the following:
if a hardirq triggers a softirq, but syscall-level code on the same CPU
disabled local bhs via local_bh_disable(), then we 'miss' the execution of
the softirq, until the next IRQ. (or next direct call to do_softirq()).

the attached softirq-2.4.5-B0 patch fixes this problem by calling
do_softirq()  from local_bh_enable() [if the bh count is 0, to avoid
recursion]. This slightly changes local_bh_enable() semantics: calling
do_softirq() has the side-effect of disabling/enabling interrupts, so code
that used local_bh_enable while interrupts are disabled (and depended on
them staying disabled) will break. I checked all code that uses
local_bh_enable() via a debugging check, and the only (harmless) violation
of this new rule is machine_restart() in the x86 tree.

Yesterday's patches fix this problem too, but only as a lucky side-effect,
and only in the idle-poll case. 2.4.5 + softirq-2.4.5-B0 TCP latency is
down from a fluctuating 300-400 microseconds to a stable 109 microseconds.

	Ingo

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1617 bytes --]

--- linux/kernel/softirq.c.orig	Sun May 27 20:57:36 2001
+++ linux/kernel/softirq.c	Sun May 27 20:57:52 2001
@@ -87,7 +87,7 @@
 			goto retry;
 	}
 
-	local_bh_enable();
+	__local_bh_enable();
 
 	/* Leave with locally disabled hard irqs. It is critical to close
 	 * window for infinite recursion, while we help local bh count,
--- linux/include/asm-i386/softirq.h.orig	Sun May 27 20:56:58 2001
+++ linux/include/asm-i386/softirq.h	Sun May 27 20:58:15 2001
@@ -5,10 +5,12 @@
 #include <asm/hardirq.h>
 
 #define cpu_bh_disable(cpu)	do { local_bh_count(cpu)++; barrier(); } while (0)
-#define cpu_bh_enable(cpu)	do { barrier(); local_bh_count(cpu)--; } while (0)
+#define __cpu_bh_enable(cpu)	do { barrier(); local_bh_count(cpu)--; } while (0)
+extern void cpu_bh_enable (unsigned int cpu);
 
 #define local_bh_disable()	cpu_bh_disable(smp_processor_id())
 #define local_bh_enable()	cpu_bh_enable(smp_processor_id())
+#define __local_bh_enable()	__cpu_bh_enable(smp_processor_id())
 
 #define in_softirq() (local_bh_count(smp_processor_id()) != 0)
 
--- linux/arch/i386/kernel/irq.c.orig	Sun May 27 20:55:08 2001
+++ linux/arch/i386/kernel/irq.c	Sun May 27 20:56:45 2001
@@ -628,6 +628,19 @@
 	return 1;
 }
 
+/*
+ * A bh-atomic section might have blocked the execution of softirqs.
+ * re-run them if appropriate.
+ */
+void cpu_bh_enable (unsigned int cpu)
+{
+	if (!--local_bh_count(cpu) &&
+			(softirq_active(cpu) & softirq_mask(cpu))) {
+		do_softirq();
+		__sti();
+	}
+}
+
 /**
  *	request_irq - allocate an interrupt line
  *	@irq: Interrupt line to allocate

Re: [patch] softirq-2.4.5-B0

[David S. Miller]

Ingo Molnar writes:
 > the bug/misbehavior causing bad latencies turned out to be the following:
 > if a hardirq triggers a softirq, but syscall-level code on the same CPU
 > disabled local bhs via local_bh_disable(), then we 'miss' the execution of
 > the softirq, until the next IRQ. (or next direct call to do_softirq()).

Hooray, some sanity in this thread finally :-)

Yes, this makes perfect sense, this is indeed what can happen.

 > the attached softirq-2.4.5-B0 patch fixes this problem by calling
 > do_softirq()  from local_bh_enable() [if the bh count is 0, to avoid
 > recursion].

Yikes!  I do not like this fix.

I'd rather local_bh_enable() not become a more heavy primitive.

I know, in one respect it makes sense because it parallels how
hardware interrupts work, but not this thing is a function call
instead of a counter bump :-(

Any other ideas how to zap this?

Later,
David S. Miller
davem@redhat.com

Re: [patch] softirq-2.4.5-B0

[Ingo Molnar]

On Sun, 27 May 2001, David S. Miller wrote:

> Hooray, some sanity in this thread finally :-)

[ finally i had some sleep after a really long debugging session :-| ]

>  > the attached softirq-2.4.5-B0 patch fixes this problem by calling
>  > do_softirq()  from local_bh_enable() [if the bh count is 0, to avoid
>  > recursion].
>
> Yikes!  I do not like this fix.

i think we have no choice, unfortunately.

and i think function calls are not that scary anymore, especially not with
regparms and similar compiler optimizations. The function is simple, the
function just goes in and returns in 90% of the cases, which should be
handled nicely by most BTBs.

we have other fundamental primitives that are a function call too, eg.
dget(), and they are used just as frequently. In 2.4 we were moving
inlined code into functions in a number of cases, and it appeared to work
out well in most cases.

> I'd rather local_bh_enable() not become a more heavy primitive.
>
> I know, in one respect it makes sense because it parallels how
> hardware interrupts work, but not this thing is a function call
> instead of a counter bump :-(

i believe the important thing is that the function has no serialization or
other 'heavy' stuff. BHs had the misdesign of not being restarted after
being re-enabled, and it caused performance problems - we should not
repeat history.

	Ingo