[Adeos-main] latency fix for 2.6.25?

[Adeos-main] latency fix for 2.6.25?

[Pierangelo Masarati]

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

Dear ADEOS users,

2.6.25 seems to show abnormal latencies on hardware that showed good 
performances up to 2.6.24.  We think we traced down the issue to x86's 
process_xx.c, which disappeared after regressing default_idle() to 
2.6.24.  The related changes are described in the attached patch.

Sincerely, p.


[-- Attachment #2: process_xx-2.6.25.patch --]
[-- Type: text/x-patch, Size: 4010 bytes --]

--- process_32.c.orig	2008-04-17 04:49:44.000000000 +0200
+++ process_32.c	2008-06-16 09:38:20.000000000 +0200
@@ -111,20 +111,25 @@ void default_idle(void)
 		 */
 		smp_mb();
 
-		local_irq_disable();
+		local_irq_disable_hw();
 		if (!need_resched()) {
+#ifndef CONFIG_IPIPE
 			ktime_t t0, t1;
 			u64 t0n, t1n;
 
 			t0 = ktime_get();
 			t0n = ktime_to_ns(t0);
+#endif
 			safe_halt();	/* enables interrupts racelessly */
+#ifndef CONFIG_IPIPE
 			local_irq_disable();
 			t1 = ktime_get();
 			t1n = ktime_to_ns(t1);
 			sched_clock_idle_wakeup_event(t1n - t0n);
-		}
-		local_irq_enable();
+			local_irq_enable(); /* This will force enable_hw as well. */
+#endif
+		} else
+			local_irq_enable_hw();
 		current_thread_info()->status |= TS_POLLING;
 	} else {
 		/* loop is done by the caller */
@@ -203,6 +208,7 @@ void cpu_idle(void)
 				play_dead();
 
 			__get_cpu_var(irq_stat).idle_timestamp = jiffies;
+ 			ipipe_suspend_domain();
 			idle();
 		}
 		tick_nohz_restart_sched_tick();
@@ -269,6 +275,11 @@ static int __cpuinit mwait_usable(const 
 
 void __cpuinit select_idle_routine(const struct cpuinfo_x86 *c)
 {
+#ifdef CONFIG_IPIPE
+#define default_to_mwait force_mwait
+#else
+#define default_to_mwait 1
+#endif
 	static int selected;
 
 	if (selected)
@@ -284,7 +295,7 @@ void __cpuinit select_idle_routine(const
 		 * Skip, if setup has overridden idle.
 		 * One CPU supports mwait => All CPUs supports mwait
 		 */
-		if (!pm_idle) {
+		if (!pm_idle && default_to_mwait) {
 			printk(KERN_INFO "using mwait in idle threads.\n");
 			pm_idle = mwait_idle;
 		}
--- process_64.c.orig	2008-04-17 04:49:44.000000000 +0200
+++ process_64.c	2008-06-16 09:38:20.000000000 +0200
@@ -53,6 +53,8 @@
 
 asmlinkage extern void ret_from_fork(void);
 
+asmlinkage extern void thread_return(void);
+
 unsigned long kernel_thread_flags = CLONE_VM | CLONE_UNTRACED;
 
 unsigned long boot_option_idle_override = 0;
@@ -105,20 +107,25 @@ void default_idle(void)
 	 * test NEED_RESCHED:
 	 */
 	smp_mb();
-	local_irq_disable();
+	local_irq_disable_hw();
 	if (!need_resched()) {
+#ifndef CONFIG_IPIPE
 		ktime_t t0, t1;
 		u64 t0n, t1n;
 
 		t0 = ktime_get();
 		t0n = ktime_to_ns(t0);
+#endif
 		safe_halt();	/* enables interrupts racelessly */
+#ifndef CONFIG_IPIPE
 		local_irq_disable();
 		t1 = ktime_get();
 		t1n = ktime_to_ns(t1);
 		sched_clock_idle_wakeup_event(t1n - t0n);
-	}
-	local_irq_enable();
+		local_irq_enable(); /* This will force enable_hw as well. */
+#endif
+	} else
+		local_irq_enable_hw();
 	current_thread_info()->status |= TS_POLLING;
 }
 
@@ -185,6 +192,7 @@ void cpu_idle(void)
 			 */
 			local_irq_disable();
 			enter_idle();
+ 			ipipe_suspend_domain();
 			idle();
 			/* In many cases the interrupt that ended idle
 			   has already called exit_idle. But some idle
@@ -265,6 +273,11 @@ static int __cpuinit mwait_usable(const 
 
 void __cpuinit select_idle_routine(const struct cpuinfo_x86 *c)
 {
+#ifdef CONFIG_IPIPE
+#define default_to_mwait force_mwait
+#else
+#define default_to_mwait 1
+#endif
 	static int selected;
 
 	if (selected)
@@ -280,7 +293,7 @@ void __cpuinit select_idle_routine(const
 		 * Skip, if setup has overridden idle.
 		 * One CPU supports mwait => All CPUs supports mwait
 		 */
-		if (!pm_idle) {
+ 		if (!pm_idle && default_to_mwait) {
 			printk(KERN_INFO "using mwait in idle threads.\n");
 			pm_idle = mwait_idle;
 		}
@@ -483,6 +496,7 @@ int copy_thread(int nr, unsigned long cl
 	p->thread.sp = (unsigned long) childregs;
 	p->thread.sp0 = (unsigned long) (childregs+1);
 	p->thread.usersp = me->thread.usersp;
+ 	p->thread.rip = (unsigned long) thread_return;
 
 	set_tsk_thread_flag(p, TIF_FORK);
 
@@ -602,7 +616,7 @@ __switch_to(struct task_struct *prev_p, 
 {
 	struct thread_struct *prev = &prev_p->thread,
 				 *next = &next_p->thread;
-	int cpu = smp_processor_id();
+	int cpu = raw_smp_processor_id();
 	struct tss_struct *tss = &per_cpu(init_tss, cpu);
 
 	/* we're going to use this soon, after a few expensive things */

Re: [Adeos-main] latency fix for 2.6.25?

[Philippe Gerum]

Pierangelo Masarati wrote:
> Dear ADEOS users,
> 
> 2.6.25 seems to show abnormal latencies on hardware that showed good
> performances up to 2.6.24.  We think we traced down the issue to x86's
> process_xx.c, which disappeared after regressing default_idle() to
> 2.6.24.  The related changes are described in the attached patch.
> 

This patch would badly break the runqueue statistics, and likely the Linux
scheduler tick engine too.

Actually, the hunk in default_idle() seems useless, since co-kernel activity
should be accounted as Linux idle time anyway. Does this patch also fixes
the issue you tracked down?

diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 8d71912..d97c8c2 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -111,7 +111,7 @@ void default_idle(void)
 		 */
 		smp_mb();

-		local_irq_disable_hw();
+		local_irq_disable();
 		if (!need_resched()) {
 			ktime_t t0, t1;
 			u64 t0n, t1n;
@@ -123,9 +123,8 @@ void default_idle(void)
 			t1 = ktime_get();
 			t1n = ktime_to_ns(t1);
 			sched_clock_idle_wakeup_event(t1n - t0n);
-			local_irq_enable(); /* This will force enable_hw as well. */
-		} else
-			local_irq_enable_hw();
+		}
+		local_irq_enable();
 		current_thread_info()->status |= TS_POLLING;
 	} else {
 		/* loop is done by the caller */
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index fa242a4..a3b2fa7 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -107,7 +107,7 @@ void default_idle(void)
 	 * test NEED_RESCHED:
 	 */
 	smp_mb();
-	local_irq_disable_hw();
+	local_irq_disable();
 	if (!need_resched()) {
 		ktime_t t0, t1;
 		u64 t0n, t1n;
@@ -119,9 +119,8 @@ void default_idle(void)
 		t1 = ktime_get();
 		t1n = ktime_to_ns(t1);
 		sched_clock_idle_wakeup_event(t1n - t0n);
-		local_irq_enable(); /* This will force enable_hw as well. */
-	} else
-		local_irq_enable_hw();
+	}
+	local_irq_enable();
 	current_thread_info()->status |= TS_POLLING;
 }

-- 
Philippe.

Re: [Adeos-main] latency fix for 2.6.25?

[Pierangelo Masarati]

Philippe Gerum wrote:
> Pierangelo Masarati wrote:
>> Dear ADEOS users,
>>
>> 2.6.25 seems to show abnormal latencies on hardware that showed good
>> performances up to 2.6.24.  We think we traced down the issue to x86's
>> process_xx.c, which disappeared after regressing default_idle() to
>> 2.6.24.  The related changes are described in the attached patch.
>>
> 
> This patch would badly break the runqueue statistics, and likely the Linux
> scheduler tick engine too.
> 
> Actually, the hunk in default_idle() seems useless, since co-kernel activity
> should be accounted as Linux idle time anyway. Does this patch also fixes
> the issue you tracked down?

Dear Philippe,

we'll try it.  It will require some time to empirically let it run for a 
while to be sure.  Usually, the weird latency effect occurs within half 
a hour, but we'd like to wait a little longer.

Apart from not disturbing Linux, your fix should work, since it doesn't 
touch the hw flag.  The problem with the patch is likely that: CPU i 
gets the seqlock after hlt, and can be preempted by the RTOS; CPU k 
tries to acquire the lock before hlt, i.e. with hw flags disabled, so it 
cannot be preempted by the RTOS.  If the RTOS after preempting CPU i 
does a bit of work, the RTOS on CPU k is stalled until the RTOS finishes 
working on CPU i.

In any case, it is now running on two machines: a 32 and a 64 bit.  I'll 
let you know.

Sincerely, p.

Re: [Adeos-main] latency fix for 2.6.25?

[Philippe Gerum]

Pierangelo Masarati wrote:
> Philippe Gerum wrote:
>> Pierangelo Masarati wrote:
>>> Dear ADEOS users,
>>>
>>> 2.6.25 seems to show abnormal latencies on hardware that showed good
>>> performances up to 2.6.24.  We think we traced down the issue to x86's
>>> process_xx.c, which disappeared after regressing default_idle() to
>>> 2.6.24.  The related changes are described in the attached patch.
>>>
>> This patch would badly break the runqueue statistics, and likely the Linux
>> scheduler tick engine too.
>>
>> Actually, the hunk in default_idle() seems useless, since co-kernel activity
>> should be accounted as Linux idle time anyway. Does this patch also fixes
>> the issue you tracked down?
> 
> Dear Philippe,
> 
> we'll try it.  It will require some time to empirically let it run for a 
> while to be sure.  Usually, the weird latency effect occurs within half 
> a hour, but we'd like to wait a little longer.
> 
> Apart from not disturbing Linux, your fix should work, since it doesn't 
> touch the hw flag.  The problem with the patch is likely that: CPU i 
> gets the seqlock after hlt,

I don't see any seqlock in the idling code. Did you mean the spinlock in
sched_clock_idle_wakeup_event()?

 and can be preempted by the RTOS; CPU k
> tries to acquire the lock before hlt, i.e. with hw flags disabled,

Hw flags are still on actually -- local_irq_disable() won't switch them off.

 so it
> cannot be preempted by the RTOS.  If the RTOS after preempting CPU i 
> does a bit of work, the RTOS on CPU k is stalled until the RTOS finishes 
> working on CPU i.

> 
> In any case, it is now running on two machines: a 32 and a 64 bit.  I'll 
> let you know.
> 
> Sincerely, p.
> 
> _______________________________________________
> Adeos-main mailing list
> Adeos-main@domain.hid
> https://mail.gna.org/listinfo/adeos-main
> 


-- 
Philippe.

Re: [Adeos-main] latency fix for 2.6.25?

[Philippe Gerum]

Philippe Gerum wrote:
> Pierangelo Masarati wrote:
>> Philippe Gerum wrote:
>>> Pierangelo Masarati wrote:
>>>> Dear ADEOS users,
>>>>
>>>> 2.6.25 seems to show abnormal latencies on hardware that showed good
>>>> performances up to 2.6.24.  We think we traced down the issue to x86's
>>>> process_xx.c, which disappeared after regressing default_idle() to
>>>> 2.6.24.  The related changes are described in the attached patch.
>>>>
>>> This patch would badly break the runqueue statistics, and likely the Linux
>>> scheduler tick engine too.
>>>
>>> Actually, the hunk in default_idle() seems useless, since co-kernel activity
>>> should be accounted as Linux idle time anyway. Does this patch also fixes
>>> the issue you tracked down?
>> Dear Philippe,
>>
>> we'll try it.  It will require some time to empirically let it run for a 
>> while to be sure.  Usually, the weird latency effect occurs within half 
>> a hour, but we'd like to wait a little longer.
>>
>> Apart from not disturbing Linux, your fix should work, since it doesn't 
>> touch the hw flag.  The problem with the patch is likely that: CPU i 
>> gets the seqlock after hlt,
> 
> I don't see any seqlock in the idling code. Did you mean the spinlock in
> sched_clock_idle_wakeup_event()?
>

You likely refer to ktime_get_ts(); fortunately, we should not lock out interrupts.

>  and can be preempted by the RTOS; CPU k
>> tries to acquire the lock before hlt, i.e. with hw flags disabled,
> 
> Hw flags are still on actually -- local_irq_disable() won't switch them off.
> 
>  so it
>> cannot be preempted by the RTOS.  If the RTOS after preempting CPU i 
>> does a bit of work, the RTOS on CPU k is stalled until the RTOS finishes 
>> working on CPU i.
> 
>> In any case, it is now running on two machines: a 32 and a 64 bit.  I'll 
>> let you know.
>>
>> Sincerely, p.
>>
>> _______________________________________________
>> Adeos-main mailing list
>> Adeos-main@domain.hid
>> https://mail.gna.org/listinfo/adeos-main
>>
> 
> 


-- 
Philippe.

Re: [Adeos-main] latency fix for 2.6.25?

[Philippe Gerum]

Philippe Gerum wrote:
> Pierangelo Masarati wrote:
>> Philippe Gerum wrote:
>>> Pierangelo Masarati wrote:
>>>> Dear ADEOS users,
>>>>
>>>> 2.6.25 seems to show abnormal latencies on hardware that showed good
>>>> performances up to 2.6.24.  We think we traced down the issue to x86's
>>>> process_xx.c, which disappeared after regressing default_idle() to
>>>> 2.6.24.  The related changes are described in the attached patch.
>>>>
>>> This patch would badly break the runqueue statistics, and likely the Linux
>>> scheduler tick engine too.
>>>
>>> Actually, the hunk in default_idle() seems useless, since co-kernel activity
>>> should be accounted as Linux idle time anyway. Does this patch also fixes
>>> the issue you tracked down?
>> Dear Philippe,
>>
>> we'll try it.  It will require some time to empirically let it run for a 
>> while to be sure.  Usually, the weird latency effect occurs within half 
>> a hour, but we'd like to wait a little longer.
>>
>> Apart from not disturbing Linux, your fix should work, since it doesn't 
>> touch the hw flag.  The problem with the patch is likely that: CPU i 
>> gets the seqlock after hlt,
>

Ok, you were talking about the _previous_ implementation, right? Ok, I must be a
bit slow today; Yes, the issue was very likely the one you described.

> I don't see any seqlock in the idling code. Did you mean the spinlock in
> sched_clock_idle_wakeup_event()?
> 
>  and can be preempted by the RTOS; CPU k
>> tries to acquire the lock before hlt, i.e. with hw flags disabled,
> 
> Hw flags are still on actually -- local_irq_disable() won't switch them off.
> 
>  so it
>> cannot be preempted by the RTOS.  If the RTOS after preempting CPU i 
>> does a bit of work, the RTOS on CPU k is stalled until the RTOS finishes 
>> working on CPU i.
> 
>> In any case, it is now running on two machines: a 32 and a 64 bit.  I'll 
>> let you know.
>>
>> Sincerely, p.
>>
>> _______________________________________________
>> Adeos-main mailing list
>> Adeos-main@domain.hid
>> https://mail.gna.org/listinfo/adeos-main
>>
> 
> 


-- 
Philippe.

Re: [Adeos-main] latency fix for 2.6.25?

[Pierangelo Masarati]

Philippe Gerum wrote:

> Ok, you were talking about the _previous_ implementation, right?

Yes.  It's been running for a few hours today, so your fix seems to be 
fine; in any case, the tests will run overnight, and I'll let you know 
tomorrow.  I believe this issue could not be easily pointed out by just 
a simple latency check.

Sincerely, p.

Re: [Adeos-main] latency fix for 2.6.25?

[Philippe Gerum]

Pierangelo Masarati wrote:
> Philippe Gerum wrote:
> 
>> Ok, you were talking about the _previous_ implementation, right?
> 
> Yes.  It's been running for a few hours today, so your fix seems to be 
> fine; in any case, the tests will run overnight, and I'll let you know 
> tomorrow.  I believe this issue could not be easily pointed out by just 
> a simple latency check.
>

Agreed, we need to create a load that makes races on seqlocks more likely, and
ideally with lots of readers delaying a single writer.

> Sincerely, p.
> 
> _______________________________________________
> Adeos-main mailing list
> Adeos-main@domain.hid
> https://mail.gna.org/listinfo/adeos-main
> 


-- 
Philippe.

Re: [Adeos-main] latency fix for 2.6.25?

[Pierangelo Masarati]

Philippe Gerum wrote:

>>> Ok, you were talking about the _previous_ implementation, right?
>> Yes.  It's been running for a few hours today, so your fix seems to be 
>> fine; in any case, the tests will run overnight, and I'll let you know 
>> tomorrow.  I believe this issue could not be easily pointed out by just 
>> a simple latency check.
>>
> 
> Agreed, we need to create a load that makes races on seqlocks more likely, and
> ideally with lots of readers delaying a single writer.

The testing of your fix ran overnight without problems, so we'd consider 
it fine.  Another possible fix to preserve linux timings is to use 
local_irq_disable_hw() after halt.  It seems to create no significant 
latency.

Sincerely, p.

Re: [Adeos-main] latency fix for 2.6.25?

[Philippe Gerum]

Pierangelo Masarati wrote:
> Philippe Gerum wrote:
> 
>>>> Ok, you were talking about the _previous_ implementation, right?
>>> Yes.  It's been running for a few hours today, so your fix seems to be 
>>> fine; in any case, the tests will run overnight, and I'll let you know 
>>> tomorrow.  I believe this issue could not be easily pointed out by just 
>>> a simple latency check.
>>>
>> Agreed, we need to create a load that makes races on seqlocks more likely, and
>> ideally with lots of readers delaying a single writer.
> 
> The testing of your fix ran overnight without problems, so we'd consider 
> it fine.

Ok, thanks. Will issue -03 asap.

  Another possible fix to preserve linux timings is to use
> local_irq_disable_hw() after halt.  It seems to create no significant 
> latency.
> 

The only way to fully preserve the timings would involve running
sched_clock_idle_wakeup_event() with hw interrupts off, which grabs a spinlock
on the current runqueue; I'm worried about latencies introduced by sporadic
contention on that lock due to task migrations.

OTOH, the scheduler tick handler implements a counter-measure against underflows
of the per-runqueue clocks, so even if the RTOS preempts often or for a long
time the idling code, it should resync reasonably well. Hopefully.

-- 
Philippe.