Re: [PATCH] Re: softirq buggy

Re: [PATCH] Re: softirq buggy

[Studierende der Universitaet des Saarlandes]

> > I'd prefer to inline cpu_is_idle(), but optimizing the idle 
> >code path is probably not that important ;-) 
>
> Sure it is, in one way: how fast can you get back to work? 
> (not OK to take a millisecond getting out of the idle loop) 

2 short function calls instead of 2 "if(current->need_resched)" on the
way out.

I didn't try very hard to fix the inline dependencies, could you try to
move cpu_is_idle() from kernel/sched.c into <linux/pm.h>?

I'm sure it won't be more difficult than the last "Athlon+SMP doesn't
compile" problem ;-)

--
	Manfred

Re: softirq buggy [Re: Serial port latency]

[kuznet]

Hello!

> But with a huge overhead. I'd prefer to call it directly from within the
> idle functions, the overhead of schedule is IMHO too high.


+	if (current->need_resched) {
+		return 0;
		^^^^^^^^
+	}
+	if (softirq_active(smp_processor_id()) & softirq_mask(smp_processor_id())) {
+		do_softirq();
+		return 0;
		^^^^^^^^^
You return one value in both casesand I decided it means "schedule". 8)
Apparently you meaned return 1 in the first case. 8)

But in this case it becomes wrong. do_softirq() can raise need_reshed
and moreover irqs arrive during it. Order of check should be different.


BTW what's about overhead... I suspect it is _lower_ in the case
of schedule(). In the case of networking at least, when softirq
most likely wakes some socket.

Alexey

[PATCH] Re: softirq buggy

[Manfred Spraul]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]

I've attached a new patch:

* cpu_is_idle() moved to <linux/pm.h>
* function uninlined due to header dependencies
* cpu_is_idle() doesn't call do_softirq directly, instead the caller
returns to schedule()
* cpu_is_idle() exported for modules.
* docu updated.

I'd prefer to inline cpu_is_idle(), but optimizing the idle code path is
probably not that important ;-)

--
	Manfred

[-- Attachment #2: patch-cpu-idle --]
[-- Type: text/plain, Size: 2940 bytes --]

// $Header$
// Kernel Version:
//  VERSION = 2
//  PATCHLEVEL = 4
//  SUBLEVEL = 3
//  EXTRAVERSION = -ac3
--- 2.4/include/linux/pm.h	Thu Jan  4 23:50:47 2001
+++ build-2.4/include/linux/pm.h	Sun Apr  8 21:02:02 2001
@@ -186,6 +186,8 @@
 extern void (*pm_idle)(void);
 extern void (*pm_power_off)(void);
 
+int cpu_is_idle(void);
+
 #endif /* __KERNEL__ */
 
 #endif /* _LINUX_PM_H */
--- 2.4/kernel/sched.c	Sat Apr  7 22:02:27 2001
+++ build-2.4/kernel/sched.c	Sun Apr  8 21:02:37 2001
@@ -1242,6 +1242,28 @@
 	sched_data->last_schedule = get_cycles();
 }
 
+/**
+ * cpu_is_idle - helper function for idle functions
+ * 
+ * pm_idle functions must call this function to verify that
+ * the cpu is really idle.
+ * The return value is valid until local interrupts are
+ * reenabled.
+ * Return values:
+ * 1: go into power saving mode.
+ * 0: cpu is not idle, return to schedule()
+ */
+int cpu_is_idle(void)
+{
+	if (current->need_resched)
+		return 0;
+
+	if (softirq_active(smp_processor_id()) & softirq_mask(smp_processor_id()))
+		return 0;
+
+	return 1;
+}
+
 extern void init_timervecs (void);
 
 void __init sched_init(void)
--- 2.4/kernel/ksyms.c	Sat Apr  7 22:02:27 2001
+++ build-2.4/kernel/ksyms.c	Sun Apr  8 21:42:48 2001
@@ -440,6 +440,7 @@
 #endif
 EXPORT_SYMBOL(kstat);
 EXPORT_SYMBOL(nr_running);
+EXPORT_SYMBOL(cpu_is_idle);
 
 /* misc */
 EXPORT_SYMBOL(panic);
--- 2.4/arch/i386/kernel/process.c	Thu Feb 22 22:28:52 2001
+++ build-2.4/arch/i386/kernel/process.c	Sun Apr  8 21:25:23 2001
@@ -81,7 +81,7 @@
 {
 	if (current_cpu_data.hlt_works_ok && !hlt_counter) {
 		__cli();
-		if (!current->need_resched)
+		if (cpu_is_idle())
 			safe_halt();
 		else
 			__sti();
@@ -105,13 +105,10 @@
 	 */
 	oldval = xchg(&current->need_resched, -1);
 
-	if (!oldval)
-		asm volatile(
-			"2:"
-			"cmpl $-1, %0;"
-			"rep; nop;"
-			"je 2b;"
-				: :"m" (current->need_resched));
+	if (!oldval) {
+		while(cpu_is_idle())
+			rep_nop();
+	}
 }
 
 /*
@@ -131,7 +128,7 @@
 		void (*idle)(void) = pm_idle;
 		if (!idle)
 			idle = default_idle;
-		while (!current->need_resched)
+		while (cpu_is_idle())
 			idle();
 		schedule();
 		check_pgt_cache();
--- 2.4/drivers/acpi/cpu.c	Sat Apr  7 22:02:01 2001
+++ build-2.4/drivers/acpi/cpu.c	Sat Apr  7 23:55:17 2001
@@ -148,7 +148,7 @@
 		unsigned long diff;
 		
 		__cli();
-		if (current->need_resched)
+		if (!cpu_is_idle())
 			goto out;
 		if (acpi_bm_activity())
 			goto sleep2;
@@ -171,7 +171,7 @@
 		unsigned long diff;
 
 		__cli();
-		if (current->need_resched)
+		if (!cpu_is_idle())
 			goto out;
 		if (acpi_bm_activity())
 			goto sleep2;
@@ -205,7 +205,7 @@
 		unsigned long diff;
 
 		__cli();
-		if (current->need_resched)
+		if (!cpu_is_idle())
 			goto out;
 
 		time = acpi_read_pm_timer();
@@ -235,7 +235,7 @@
 		unsigned long diff;
 
 		__cli();
-		if (current->need_resched)
+		if (!cpu_is_idle())
 			goto out;
 		time = acpi_read_pm_timer();
 		acpi_c1_count++;

Re: [PATCH] Re: softirq buggy

[Albert D. Cahalan]

> I'd prefer to inline cpu_is_idle(), but optimizing the idle
> code path is probably not that important ;-)

Sure it is, in one way: how fast can you get back to work?
(not OK to take a millisecond getting out of the idle loop)

Re: [PATCH] Re: softirq buggy

[Andrea Arcangeli]

On Sun, Apr 08, 2001 at 11:35:36PM +0200, Manfred Spraul wrote:
> I've attached a new patch:
> 
> * cpu_is_idle() moved to <linux/pm.h>
> * function uninlined due to header dependencies
> * cpu_is_idle() doesn't call do_softirq directly, instead the caller
> returns to schedule()
> * cpu_is_idle() exported for modules.
> * docu updated.
> 
> I'd prefer to inline cpu_is_idle(), but optimizing the idle code path is
> probably not that important ;-)

your cpu_is_idle will return 0 in the need_resched != 0 check even if the cpu
is idle (because of the -1 trick for avoiding the SMP-IPI to notify the cpu).

The issue you are addressing is quite londstanding and it is not only related
to the loop with an idle cpu.

This is the way I prefer to fix it:

	ftp://ftp.us.kernel.org/pub/linux/kernel/people/andrea/patches/v2.4/2.4.4pre1/ksoftirqd-1

Andrea

Re: [PATCH] Re: softirq buggy

[Manfred Spraul]

Andrea Arcangeli wrote:
> 
> your cpu_is_idle will return 0 in the need_resched != 0 check even if the cpu
> is idle (because of the -1 trick for avoiding the SMP-IPI to notify the cpu).
>
Fixed.

> The issue you are addressing is quite londstanding and it is not only related
> to the loop with an idle cpu.
> 
> This is the way I prefer to fix it:
> 
>         ftp://ftp.us.kernel.org/pub/linux/kernel/people/andrea/patches/v2.4/2.4.4pre1/ksoftirqd-1
>
The return path to user space checks for pending softirqs. A delay of
1/HZ is only possible if the cpu loops in kernel space without returning
to user space - and the functions that can loop check
'current->need_resched'. That means that either cpu_is_idle() must be
renamed to schedule_required() and all 'need_resched' users should use
that function, or something like your patch.

Is a full thread really necessary? Just setting 'need_resched' should be
enough, schedule() checks for pending softirqs.
And do you have a rough idea how often that new thread is scheduled
under load?

Btw, you don't schedule the ksoftirqd thread if do_softirq() returns
from the 'if(in_interrupt())' check.
I assume that this is the most common case of delayed softirq
processing:

; in process context
spin_lock_bh();
; hw interrupt arrives
; do_softirq returns immediately
spin_unlock_bh();


--
	Manfred

Re: [PATCH] Re: softirq buggy

[Andrea Arcangeli]

[-- Attachment #1: Type: text/plain, Size: 2730 bytes --]

On Mon, Apr 09, 2001 at 05:26:27PM +0200, Manfred Spraul wrote:
> The return path to user space checks for pending softirqs. A delay of

And it breaks the loop too if new softirq events become pending again in
background.

> 1/HZ is only possible if the cpu loops in kernel space without returning
> to user space - and the functions that can loop check

It is also possible when new events are posted and I think it makes
sense to scale the softirq load with the scheduler when it is flooding.

Theoretically one could move the _whole_ softirq load into the ksoftirqd, but
that would increase the latency too much and I think it is better to use it
only as a fallback when we have to giveup but we still would like to keep
processing the softirq load so we let the scheduler to choose if we still can
do that or if we should giveup on the softirq.

> Is a full thread really necessary? Just setting 'need_resched' should be
> enough, schedule() checks for pending softirqs.

If you abuse need_resched then you can starve userspace again, if you are ok
to starve userspace indefinitely then it is more efficient to keep looping
forever into do_softirq as far as new events are posted in background instead
of exiting do_softirq and waiting the scheduler to kickin again.

> And do you have a rough idea how often that new thread is scheduled
> under load?

The scheduling is not as heavy as with tasks, it's a kernel thread
so the tlb isn't touched. However yes it will generate some overhead
with schedule() compared to just waiting the 1/HZ but letting the scheduler to
understand when the softirq should keep running instead of another task is
supposed to be a feature. I run a netpipe run with an alpha SMP as receiver
with the ksoftirqd patch and then without and the numbers didn't changed at all
even if the ksofitrqd was often running (1/2% of the load of the machine).

> Btw, you don't schedule the ksoftirqd thread if do_softirq() returns
> from the 'if(in_interrupt())' check.

That's not necessary and it's intentional, such check will be passed in the
last do_softirq executed before returning to userspace or kernel normal
context, the reason of such check is only to avoid recursing too much on the
stack during nested irqs.

> I assume that this is the most common case of delayed softirq
> processing:
> 
> ; in process context
> spin_lock_bh();
> ; hw interrupt arrives
> ; do_softirq returns immediately
> spin_unlock_bh();

This is yet another case and it's handled before returning to userspace so the
latency should still be pretty small (and there would be no singificant
advantage and almost certainly only a performance drop in waking up ksoftirqd
from the `do_softirq returns immediatly' line).

Andrea

[-- Attachment #2: ksofitrqd.png --]
[-- Type: image/png, Size: 3940 bytes --]

Re: [PATCH] Re: softirq buggy

[kuznet]

Hello!

> Btw, you don't schedule the ksoftirqd thread if do_softirq() returns
> from the 'if(in_interrupt())' check.

ksoftirqd will not be switched to before the first schedule
or ret form syscall, when softirqs will be processed in any case.
So, wake up in this case would be mistake.


> I assume that this is the most common case of delayed softirq
> processing:

softirqs have the same latency warranty as rt threads, so that
this is not a problem at all.

The _real_ problem is softirqs generated from another softirqs:
additonal thread is made _not_ to speed up softirqs, but to _tame_
them (if I understood Andres's explanations correctly).

Alexey

Re: [PATCH] Re: softirq buggy

[Andrea Arcangeli]

On Mon, Apr 09, 2001 at 09:48:02PM +0400, kuznet@ms2.inr.ac.ru wrote:
> Hello!
> 
> > Btw, you don't schedule the ksoftirqd thread if do_softirq() returns
> > from the 'if(in_interrupt())' check.
> 
> ksoftirqd will not be switched to before the first schedule
> or ret form syscall, when softirqs will be processed in any case.
> So, wake up in this case would be mistake.

Agreed.

> The _real_ problem is softirqs generated from another softirqs:
> additonal thread is made _not_ to speed up softirqs, but to _tame_
> them (if I understood Andres's explanations correctly).

Exactly.

Andrea