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

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- 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 --]