Serial port latency

Hi!

I've seen similar bugs. If you hook something on schedule_tq and forget to set current->need_resched, this is exactly what you get.

Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

From: "Pavel Machek" <[email protected]>

disappeared

forget

I'm running with a patch that printk's if cpu_idle() is called while a softirq is pending.
If I access the floppy on my K6/200 every track triggers the check, and sometimes the console blanking code triggers it.

What about creating a special cpu_is_idle() function that the idle functions must call before sleeping?

--
Manfred

Hi!

Seems floppy and console is buggy, then.

I'd say just fix all the bugs.
Pavel

The best software in life is free (not shareware)!		Pavel
GCM d? s-: !g p?:+ au- a--@ w+ v- C++@ UL+++ L++ N++ E++ W--- M- Y- R+

No. The softirq implementation is buggy.
I can trigger the problem with the TASKLET_HI (floppy), and both net rx and tx (ping -l)

Ok, there are 2 bugs that are (afaics) impossible to fix without checking for pending softirq's in cpu_idle():

a)
queue_task(my_task1, tq_immediate);
mark_bh();
schedule();
;within schedule: do_softirq()
;within my_task1:
mark_bh();
; bh returns, but do_softirq won't loop
; do_softirq returns.
; schedule() clears current->need_resched
; idle thread scheduled.
--> idle can run although softirq's are pending

I assume I trigger this race with the floppy driver.

b)
hw interrupt
do_softirq
within the net_rx handler: another hw interrupt, additional packets are queued
do_softirq won't loop.
returns to idle thread. --> packets delayed unnecessary.

What about the attached patch? Obviously the other idle cpu must be converted to use the function as well.

--
Manfred

@@ -73,6 +73,30 @@
 	hlt_counter--;
 }
 
+/**
+ * cpu_is_idle - helper function for idle functions
+ * 
+ * pm_idle functions must call this function to verify that
+ * the cpu is really idle. It must be called with disabled
+ * local interrupts.
+ * Return values:
+ * 0: cpu was not idle, local interrupts reenabled.
+ * 1: go into power saving mode, local interrupts are
+ *    still disabled.
+*/
+static inline int cpu_is_idle(void)
+{
+	if (current->need_resched) {
+		__sti();
+		return 0;
+	}
+	if (softirq_active(smp_processor_id()) & softirq_mask(smp_processor_id())) {
+		__sti();
+		do_softirq();
+		return 0;
+	}
+	return 1;
+}
 /*
  * We use this if we don't have any better
  * idle routine..

@@ -81,10 +105,8 @@
 {
 	if (current_cpu_data.hlt_works_ok && !hlt_counter) {
 		__cli();
-		if (!current->need_resched)
+		if (cpu_is_idle())
 			safe_halt();
-		else
-			__sti();
 	}
 }
 

Hi!

Or anything else can run altrough softirqs are pending. If it is computation job, softinterrupts are delayed quiet a bit, right?

So right fix seems to be "loop in do_softirq".

Pavel

I'm [email protected]. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at [email protected]

From: "Pavel Machek" <[email protected]>

No, it's the wrong fix.
A network server under high load would loop forever within the softirq, never returning to process level.

do_softirq cannot loop, the right fix is "check often for pending softirq's".
It's checked before a process returns to user space, it's checked when a process schedules. What's missing is that the idle functions must check for pending softirqs, too.

--
Manfred

Hi!

Ok. I was missing the fact it is checked on going userspace.

The best software in life is free (not shareware)!		Pavel
GCM d? s-: !g p?:+ au- a--@ w+ v- C++@ UL+++ L++ N++ E++ W--- M- Y- R+

What about the attached patch?

--
Manfred

// $Header$
// Kernel Version:

//  VERSION = 2
//  PATCHLEVEL = 4
//  SUBLEVEL = 3
//  EXTRAVERSION = -ac3

@@ -269,4 +269,25 @@
 extern int probe_irq_off(unsigned long);	/* returns 0 or negative on failure */
 extern unsigned int probe_irq_mask(unsigned long);	/* returns mask of ISA interrupts */
 
+/**
+ * cpu_is_idle - helper function for idle functions
+ * 
+ * pm_idle functions must call this function to verify that
+ * the cpu is really idle. It must be called with disabled
+ * local interrupts.
+ * Return values:
+ * 0: cpu was not idle.
+ * 1: go into power saving mode.
+ */
+static inline int cpu_is_idle(void)
+{
+	if (current->need_resched) {
+		return 0;
+	}
+	if (softirq_active(smp_processor_id()) & softirq_mask(smp_processor_id())) {
+		do_softirq();
+		return 0;
+	}
+	return 1;
+}
 #endif

@@ -73,6 +73,7 @@
 	hlt_counter--;
 }
 
+
 /*
  * We use this if we don't have any better
  * idle routine..

@@ -81,7 +82,7 @@
 {
 	if (current_cpu_data.hlt_works_ok && !hlt_counter) {
 		__cli();
-		if (!current->need_resched)
+		if (cpu_is_idle())
 			safe_halt();
 		else
 			__sti();

@@ -92,26 +93,21 @@
  * On SMP it's slightly faster (but much more power-consuming!)
  * to poll the ->need_resched flag instead of waiting for the
  * cross-CPU IPI to arrive. Use this option with caution.
+ *
+ * Isn't this identical to default_idle with the 'no-hlt' boot
+ * option? <[email protected]>
  */
 static void poll_idle (void)
 {
 	int oldval;
 
+	for(;;) {
+		if (!cpu_is_idle())
+			break;
+		__sti();
+		rep_nop();
+	}
 	__sti();
-
-	/*
-	 * Deal with another CPU just having chosen a thread to
-	 * run here:
-	 */
-	oldval = xchg(&current->need_resched, -1);
-
-	if (!oldval)
-		asm volatile(
-			"2:"
-			"cmpl $-1, %0;"
-			"rep; nop;"
-			"je 2b;"
-				: :"m" (current->need_resched));
 }
 
 /*

@@ -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++;

Hello!

Seeems, it is not. need_resched=-1 avoids useless IPIs.

Alexey

From: <[email protected]>

softirq_mask(smp_processor_id())) {

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.

I already wondered why need_resched is set to -1 ;-)

I'll remove that comment and repost the patch.

--
Manfred

Hello!

+ 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

From: <[email protected]>
To: "Manfred Spraul" <[email protected]>
Cc: <[email protected]>
Sent: Sunday, April 08, 2001 7:58 PM
Subject: Re: softirq buggy [Re: Serial port latency]

the

softirq_mask(smp_processor_id())) {

8)

No, the code is correct. 0 means "don't stop the cpu". The pm_idle function pointer will return to cpu_idle() (arch/i386/kernel/process.c), and that function contains another

    while(!current->need_resched)
        idle();

loop ;-)

different.

Yes, I'll correct that.

I'm not sure - what if the computer is just a router?
But OTHO: the cpu is idle, so it doesn't matter at all if the idle cpu spends it's time within schedule() or within safe_hlt(), I'll change my patch.

I have another question:
I added cpu_is_idle() into <linux/interrupt.h>. Is that acceptable, or is there a better header file for such a function?

--
Manfred

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

// $Header$
// Kernel Version:

//  VERSION = 2
//  PATCHLEVEL = 4
//  SUBLEVEL = 3
//  EXTRAVERSION = -ac3

@@ -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 */

@@ -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)

@@ -440,6 +440,7 @@
 #endif
 EXPORT_SYMBOL(kstat);
 EXPORT_SYMBOL(nr_running);
+EXPORT_SYMBOL(cpu_is_idle);
 
 /* misc */
 EXPORT_SYMBOL(panic);

@@ -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();

@@ -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++;

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)

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

Fixed.

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

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

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.

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.

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).

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.

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

Hello!

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.

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

Agreed.

Exactly.

Andrea

keventd fixes tq_scheduler case (tq_scheduler is dead).

Andrea