[Adeos-main] ipipe_suspend_domain vs. pipeline state of caller

[Adeos-main] ipipe_suspend_domain vs. pipeline state of caller

[Jan Kiszka]

Hi Philippe,

on x86, trace_hardirqs_on in safe_halt feels unhappy about the fact that
it is called with neither real irqs disable nor the root domain stalled.
Call path is cpu_idle -> default_idle -> safe_halt. We loose the
stalling of the root domain in cpu_idle via ipipe_suspend_domain.

That raises the question how ipipe_suspend_domain is supposed to deal
with the pipeline state of the calling domain. Currently it clears the
stall unconditionally, which is obviously not correct for the idle path.
My feeling is that it should not alter the state, rather restore it on
exit. But this is an exported interface (though a widely unused today, I
guess), and I don't claim to oversee all corner cases.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] ipipe_suspend_domain vs. pipeline state of caller

[Philippe Gerum]

On Wed, 2009-11-18 at 17:30 +0100, Jan Kiszka wrote:
> Hi Philippe,
> 
> on x86, trace_hardirqs_on in safe_halt feels unhappy about the fact that
> it is called with neither real irqs disable nor the root domain stalled.
> Call path is cpu_idle -> default_idle -> safe_halt. We loose the
> stalling of the root domain in cpu_idle via ipipe_suspend_domain.
> 

Reinstating the stall bit upon return from suspension is the way to go.

> That raises the question how ipipe_suspend_domain is supposed to deal
> with the pipeline state of the calling domain. Currently it clears the
> stall unconditionally, which is obviously not correct for the idle path.
> My feeling is that it should not alter the state, rather restore it on
> exit. But this is an exported interface (though a widely unused today, I
> guess), and I don't claim to oversee all corner cases.
> 

At the core of the pipeline logic, there is the assumption that a domain
may never, ever suspend in stalled state, so that no interrupt lingers
unexpectedly in the log while it enters sleep, and this is what
suspend_domain() enforces before switching to the next domain.

> Jan
> 


-- 
Philippe.

[Adeos-main] [PATCH] Restore pipeline state on exit from ipipe_suspend_domain

[Jan Kiszka]

This fixes the valid complaint about safe_halt being called with the
root domain unstalled.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---
 kernel/ipipe/core.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

Applies to 2.6.31, but is relevant for older kernels as well.

diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
index ffaceaa..cd946f3 100644
--- a/kernel/ipipe/core.c
+++ b/kernel/ipipe/core.c
@@ -615,12 +615,13 @@ void ipipe_suspend_domain(void)
 	struct ipipe_domain *this_domain, *next_domain;
 	struct ipipe_percpu_domain_data *p;
 	struct list_head *ln;
-	unsigned long flags;
+	unsigned long flags, saved_status;

 	local_irq_save_hw(flags);

 	this_domain = next_domain = __ipipe_current_domain;
 	p = ipipe_cpudom_ptr(this_domain);
+	saved_status = p->status & ~IPIPE_SYNC_MASK;
 	p->status &= ~(IPIPE_STALL_MASK|IPIPE_SYNC_MASK);

 	if (p->irqpend_himask != 0)
@@ -654,6 +655,8 @@ sync_stage:
 	}

 	__ipipe_current_domain = this_domain;
+	p = ipipe_cpudom_ptr(this_domain);
+	p->status = saved_status;

 	local_irq_restore_hw(flags);
 }
-- 
1.6.0.2

Re: [Adeos-main] [PATCH] Restore pipeline state on exit from ipipe_suspend_domain

[Philippe Gerum]

On Wed, 2009-11-18 at 18:12 +0100, Jan Kiszka wrote:
> This fixes the valid complaint about safe_halt being called with the
> root domain unstalled.

The fix should go to the caller. ipipe_suspend_domain() acts as a
logical barrier: after that point, you may assume that the current
domain is unstalled.

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> ---
>  kernel/ipipe/core.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> Applies to 2.6.31, but is relevant for older kernels as well.
> 
> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
> index ffaceaa..cd946f3 100644
> --- a/kernel/ipipe/core.c
> +++ b/kernel/ipipe/core.c
> @@ -615,12 +615,13 @@ void ipipe_suspend_domain(void)
>  	struct ipipe_domain *this_domain, *next_domain;
>  	struct ipipe_percpu_domain_data *p;
>  	struct list_head *ln;
> -	unsigned long flags;
> +	unsigned long flags, saved_status;
> 
>  	local_irq_save_hw(flags);
> 
>  	this_domain = next_domain = __ipipe_current_domain;
>  	p = ipipe_cpudom_ptr(this_domain);
> +	saved_status = p->status & ~IPIPE_SYNC_MASK;
>  	p->status &= ~(IPIPE_STALL_MASK|IPIPE_SYNC_MASK);
> 
>  	if (p->irqpend_himask != 0)
> @@ -654,6 +655,8 @@ sync_stage:
>  	}
> 
>  	__ipipe_current_domain = this_domain;
> +	p = ipipe_cpudom_ptr(this_domain);
> +	p->status = saved_status;
> 
>  	local_irq_restore_hw(flags);
>  }


-- 
Philippe.

Re: [Adeos-main] [PATCH] Restore pipeline state on exit from ipipe_suspend_domain

[Jan Kiszka]

Philippe Gerum wrote:
> On Wed, 2009-11-18 at 18:12 +0100, Jan Kiszka wrote:
>> This fixes the valid complaint about safe_halt being called with the
>> root domain unstalled.
> 
> The fix should go to the caller. ipipe_suspend_domain() acts as a
> logical barrier: after that point, you may assume that the current
> domain is unstalled.

The caller so far expect to find no interruption window between return
from ipipe_suspend_domain and yet another local_irq_disable. It expects
to remain stalled all the time until safe_halt.

Jan

> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>> ---
>>  kernel/ipipe/core.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> Applies to 2.6.31, but is relevant for older kernels as well.
>>
>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>> index ffaceaa..cd946f3 100644
>> --- a/kernel/ipipe/core.c
>> +++ b/kernel/ipipe/core.c
>> @@ -615,12 +615,13 @@ void ipipe_suspend_domain(void)
>>  	struct ipipe_domain *this_domain, *next_domain;
>>  	struct ipipe_percpu_domain_data *p;
>>  	struct list_head *ln;
>> -	unsigned long flags;
>> +	unsigned long flags, saved_status;
>>
>>  	local_irq_save_hw(flags);
>>
>>  	this_domain = next_domain = __ipipe_current_domain;
>>  	p = ipipe_cpudom_ptr(this_domain);
>> +	saved_status = p->status & ~IPIPE_SYNC_MASK;
>>  	p->status &= ~(IPIPE_STALL_MASK|IPIPE_SYNC_MASK);
>>
>>  	if (p->irqpend_himask != 0)
>> @@ -654,6 +655,8 @@ sync_stage:
>>  	}
>>
>>  	__ipipe_current_domain = this_domain;
>> +	p = ipipe_cpudom_ptr(this_domain);
>> +	p->status = saved_status;
>>
>>  	local_irq_restore_hw(flags);
>>  }
> 
> 


-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH] Restore pipeline state on exit from ipipe_suspend_domain

[Jan Kiszka]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> On Wed, 2009-11-18 at 18:12 +0100, Jan Kiszka wrote:
>>> This fixes the valid complaint about safe_halt being called with the
>>> root domain unstalled.
>> The fix should go to the caller. ipipe_suspend_domain() acts as a
>> logical barrier: after that point, you may assume that the current
>> domain is unstalled.
> 
> The caller so far expect to find no interruption window between return
> from ipipe_suspend_domain and yet another local_irq_disable. It expects
> to remain stalled all the time until safe_halt.

Checked again: Opening the IRQ window here is bogus, may cause
rescheduling delays to Linux (if not much worse things).

I suppose it's better to adjust the assumption that ipipe_suspend_domain
behaves like "sti; hlt". Are there users that rely on this?
__ipipe_walk_pipeline does not look like it would.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH] Restore pipeline state on exit from ipipe_suspend_domain

[Philippe Gerum]

On Wed, 2009-11-18 at 18:37 +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Philippe Gerum wrote:
> >> On Wed, 2009-11-18 at 18:12 +0100, Jan Kiszka wrote:
> >>> This fixes the valid complaint about safe_halt being called with the
> >>> root domain unstalled.
> >> The fix should go to the caller. ipipe_suspend_domain() acts as a
> >> logical barrier: after that point, you may assume that the current
> >> domain is unstalled.
> > 
> > The caller so far expect to find no interruption window between return
> > from ipipe_suspend_domain and yet another local_irq_disable. It expects
> > to remain stalled all the time until safe_halt.
> 
> Checked again: Opening the IRQ window here is bogus, may cause
> rescheduling delays to Linux (if not much worse things).
> 
> I suppose it's better to adjust the assumption that ipipe_suspend_domain
> behaves like "sti; hlt". Are there users that rely on this?
> __ipipe_walk_pipeline does not look like it would.

I chose to never apply the mantra "never care for out of tree code" for
Adeos, granted, at the expense of quite a lot of headaches, but that
layer is a standalone building block which exports a stable API since
years.

I may revert that decision in a foreseeable future, when X3 starts
notably. But I'm still reluctant to break such a significant assumption
in the current patch series.

I would rather move ipipe_suspend_domain() out of the atomic section on
x86 (granted, this should be done carefully if ever possible).

> 
> Jan
> 


-- 
Philippe.

[Adeos-main] [PATCH] x86: Move ipipe_suspend_domain out of IRQ-disabled section

[Jan Kiszka]

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

Philippe Gerum wrote:
> On Wed, 2009-11-18 at 18:37 +0100, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> On Wed, 2009-11-18 at 18:12 +0100, Jan Kiszka wrote:
>>>>> This fixes the valid complaint about safe_halt being called with the
>>>>> root domain unstalled.
>>>> The fix should go to the caller. ipipe_suspend_domain() acts as a
>>>> logical barrier: after that point, you may assume that the current
>>>> domain is unstalled.
>>> The caller so far expect to find no interruption window between return
>>> from ipipe_suspend_domain and yet another local_irq_disable. It expects
>>> to remain stalled all the time until safe_halt.
>> Checked again: Opening the IRQ window here is bogus, may cause
>> rescheduling delays to Linux (if not much worse things).
>>
>> I suppose it's better to adjust the assumption that ipipe_suspend_domain
>> behaves like "sti; hlt". Are there users that rely on this?
>> __ipipe_walk_pipeline does not look like it would.
> 
> I chose to never apply the mantra "never care for out of tree code" for
> Adeos, granted, at the expense of quite a lot of headaches, but that
> layer is a standalone building block which exports a stable API since
> years.
> 
> I may revert that decision in a foreseeable future, when X3 starts
> notably. But I'm still reluctant to break such a significant assumption
> in the current patch series.
> 
> I would rather move ipipe_suspend_domain() out of the atomic section on
> x86 (granted, this should be done carefully if ever possible).

Likely feasible. The good news is I missed the fact that there is
another check for needs_resched later on, right before the actual halt.
So the problem should be curable by moving ipipe_suspend_domain, at
least on x86.

Jan

-------->

x86: Move ipipe_suspend_domain out of IRQ-disabled section

ipipe_suspend_domain reenables IRQs on return, so we have to move it
before the point where the kernel disables it for halt.

This fixes the valid complaint about safe_halt being called with the
root domain unstalled.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---
 arch/x86/kernel/process_32.c |    3 ++-
 arch/x86/kernel/process_64.c |    4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index a8a5cd1..e31de9e 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -111,10 +111,11 @@ void cpu_idle(void)
 			if (cpu_is_offline(cpu))
 				play_dead();

+			ipipe_suspend_domain();
+
 			local_irq_disable();
 			/* Don't trace irqs off for idle */
 			stop_critical_timings();
-  			ipipe_suspend_domain();
 			pm_idle();
 			start_critical_timings();
 		}
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 02efd18..b30dc4d 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -136,6 +136,9 @@ void cpu_idle(void)

 			if (cpu_is_offline(smp_processor_id()))
 				play_dead();
+
+			ipipe_suspend_domain();
+
 			/*
 			 * Idle routines should keep interrupts disabled
 			 * from here on, until they go to idle.
@@ -145,7 +148,6 @@ void cpu_idle(void)
 			enter_idle();
 			/* Don't trace irqs off for idle */
 			stop_critical_timings();
-  			ipipe_suspend_domain();
 			pm_idle();
 			start_critical_timings();
 			/* In many cases the interrupt that ended idle


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Adeos-main] [PATCH] x86: Move ipipe_suspend_domain out of IRQ-disabled section

[Jan Kiszka]

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

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> On Wed, 2009-11-18 at 18:37 +0100, Jan Kiszka wrote:
>>> Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> On Wed, 2009-11-18 at 18:12 +0100, Jan Kiszka wrote:
>>>>>> This fixes the valid complaint about safe_halt being called with the
>>>>>> root domain unstalled.
>>>>> The fix should go to the caller. ipipe_suspend_domain() acts as a
>>>>> logical barrier: after that point, you may assume that the current
>>>>> domain is unstalled.
>>>> The caller so far expect to find no interruption window between return
>>>> from ipipe_suspend_domain and yet another local_irq_disable. It expects
>>>> to remain stalled all the time until safe_halt.
>>> Checked again: Opening the IRQ window here is bogus, may cause
>>> rescheduling delays to Linux (if not much worse things).
>>>
>>> I suppose it's better to adjust the assumption that ipipe_suspend_domain
>>> behaves like "sti; hlt". Are there users that rely on this?
>>> __ipipe_walk_pipeline does not look like it would.
>> I chose to never apply the mantra "never care for out of tree code" for
>> Adeos, granted, at the expense of quite a lot of headaches, but that
>> layer is a standalone building block which exports a stable API since
>> years.
>>
>> I may revert that decision in a foreseeable future, when X3 starts
>> notably. But I'm still reluctant to break such a significant assumption
>> in the current patch series.
>>
>> I would rather move ipipe_suspend_domain() out of the atomic section on
>> x86 (granted, this should be done carefully if ever possible).
> 
> Likely feasible. The good news is I missed the fact that there is
> another check for needs_resched later on, right before the actual halt.
> So the problem should be curable by moving ipipe_suspend_domain, at
> least on x86.
> 
> Jan
> 
> -------->
> 
> x86: Move ipipe_suspend_domain out of IRQ-disabled section
> 
> ipipe_suspend_domain reenables IRQs on return, so we have to move it
> before the point where the kernel disables it for halt.
> 
> This fixes the valid complaint about safe_halt being called with the
> root domain unstalled.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> ---
>  arch/x86/kernel/process_32.c |    3 ++-
>  arch/x86/kernel/process_64.c |    4 +++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index a8a5cd1..e31de9e 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -111,10 +111,11 @@ void cpu_idle(void)
>  			if (cpu_is_offline(cpu))
>  				play_dead();
> 
> +			ipipe_suspend_domain();
> +
>  			local_irq_disable();
>  			/* Don't trace irqs off for idle */
>  			stop_critical_timings();
> -  			ipipe_suspend_domain();
>  			pm_idle();
>  			start_critical_timings();
>  		}
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 02efd18..b30dc4d 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -136,6 +136,9 @@ void cpu_idle(void)
> 
>  			if (cpu_is_offline(smp_processor_id()))
>  				play_dead();
> +
> +			ipipe_suspend_domain();
> +
>  			/*
>  			 * Idle routines should keep interrupts disabled
>  			 * from here on, until they go to idle.
> @@ -145,7 +148,6 @@ void cpu_idle(void)
>  			enter_idle();
>  			/* Don't trace irqs off for idle */
>  			stop_critical_timings();
> -  			ipipe_suspend_domain();
>  			pm_idle();
>  			start_critical_timings();
>  			/* In many cases the interrupt that ended idle
> 

Oops, forgot to switch off line wrapping. You can pull from

git://git.kiszka.org/ipipe-2.6.git queues/2.6.31-x86

instead.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Adeos-main] [PATCH] Restore pipeline state on exit from ipipe_suspend_domain

[Philippe Gerum]

On Wed, 2009-11-18 at 18:25 +0100, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Wed, 2009-11-18 at 18:12 +0100, Jan Kiszka wrote:
> >> This fixes the valid complaint about safe_halt being called with the
> >> root domain unstalled.
> > 
> > The fix should go to the caller. ipipe_suspend_domain() acts as a
> > logical barrier: after that point, you may assume that the current
> > domain is unstalled.
> 
> The caller so far expect to find no interruption window between return
> from ipipe_suspend_domain and yet another local_irq_disable. It expects
> to remain stalled all the time until safe_halt.

Then you should fix the caller, to move the call to
ipipe_suspend_domain() out of the atomic section.

> 
> Jan
> 
> > 
> >> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> >> ---
> >>  kernel/ipipe/core.c |    5 ++++-
> >>  1 files changed, 4 insertions(+), 1 deletions(-)
> >>
> >> Applies to 2.6.31, but is relevant for older kernels as well.
> >>
> >> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
> >> index ffaceaa..cd946f3 100644
> >> --- a/kernel/ipipe/core.c
> >> +++ b/kernel/ipipe/core.c
> >> @@ -615,12 +615,13 @@ void ipipe_suspend_domain(void)
> >>  	struct ipipe_domain *this_domain, *next_domain;
> >>  	struct ipipe_percpu_domain_data *p;
> >>  	struct list_head *ln;
> >> -	unsigned long flags;
> >> +	unsigned long flags, saved_status;
> >>
> >>  	local_irq_save_hw(flags);
> >>
> >>  	this_domain = next_domain = __ipipe_current_domain;
> >>  	p = ipipe_cpudom_ptr(this_domain);
> >> +	saved_status = p->status & ~IPIPE_SYNC_MASK;
> >>  	p->status &= ~(IPIPE_STALL_MASK|IPIPE_SYNC_MASK);
> >>
> >>  	if (p->irqpend_himask != 0)
> >> @@ -654,6 +655,8 @@ sync_stage:
> >>  	}
> >>
> >>  	__ipipe_current_domain = this_domain;
> >> +	p = ipipe_cpudom_ptr(this_domain);
> >> +	p->status = saved_status;
> >>
> >>  	local_irq_restore_hw(flags);
> >>  }
> > 
> > 
> 
> 


-- 
Philippe.