Re: [PATCH] sparc: Use generic idle loop

[Srivatsa S. Bhat]

Hi Sam,

On 04/01/2013 02:36 PM, Sam Ravnborg wrote:
> Hi Srivatsa,
> 
> thanks for the feedback!
> 
> @davem - I need you to look at this again. I am not sure about the changes
> I do in sparc64_yield().
> 
>>> +/* the idle loop on a Sparc... ;) */
>>> +void arch_cpu_idle(void)
>>>  {
>>> +	if (sparc_idle)
>>> +		(*sparc_idle)();
>>> +	else
>>> +		cpu_relax();
>>>  }
>>
>> You need to enable interrupts when coming out in the 'else' case,
>> because we enter with interrupts disabled and expect to come out
>> of arch_cpu_idle() with interrupts enabled. (Otherwise you'll hit
>> that WARN_ON() in the generic code). Thomas has handled a similar
>> case in the mips patch.
> 
> Something like this should do it then:
> 
> /* Idle loop support */
> void arch_cpu_idle(void)
> {
>         if (sparc_idle)
>                 (*sparc_idle)();
>         else
>                 local_irq_enable();
> }
> 
> I dropped cpu_relax() as this is a simple barrier() which is
> part of the generic code (using rmb()).
> 

Yep, that sounds good.

[...] 
>> (Same is the case with the need_resched and
>> the cpu_is_offline check).
> There are some comments about race-conditions in the commit where they were added.
> I think the out-most pair can be dropped as this is done in generic code but
> that the inner ones needs to stay.
> 
> I simplified arch_cpu_idle() - former sparc64_yield() based on your comments.
> - dropped TIF_POLLING_NRFLAG handling
> - dropped smp_mb__after_clear_bit() - this is done in generic code using rmb()
> - dropped the out-most while (!need_resched() && !cpu_is_offline(cpu))
> - always enable irq on exit (but I am not sure if this is correct)
> 
> But I need davem to have a look at this - as I am fooling around in
> code that I do not know much about..
> So this is *NOT* to be applied.
> 
> 	Sam
>
[...]
>  /* XXX cli/sti -> local_irq_xxx here, check this works once SMP is fixed. */
> diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c
> index cdb80b2..cecb0d6 100644
> --- a/arch/sparc/kernel/process_64.c
> +++ b/arch/sparc/kernel/process_64.c
> @@ -52,17 +52,12 @@
> 
>  #include "kstack.h"
> 
> -static void sparc64_yield(int cpu)
> +/* Idle loop support on sparc64. */
> +void arch_cpu_idle(void)
>  {
>  	if (tlb_type != hypervisor) {
>  		touch_nmi_watchdog();
> -		return;
> -	}
> -
> -	clear_thread_flag(TIF_POLLING_NRFLAG);
> -	smp_mb__after_clear_bit();
> -
> -	while (!need_resched() && !cpu_is_offline(cpu)) {
> +	} else {
>  		unsigned long pstate;
> 
>  		/* Disable interrupts. */
> @@ -73,7 +68,7 @@ static void sparc64_yield(int cpu)
>  			: "=&r" (pstate)
>  			: "i" (PSTATE_IE));
> 
> -		if (!need_resched() && !cpu_is_offline(cpu))
> +		if (!need_resched() && !cpu_is_offline(smp_processor_id()))
>  			sun4v_cpu_yield();
> 
>  		/* Re-enable interrupts. */
> @@ -84,36 +79,16 @@ static void sparc64_yield(int cpu)
>  			: "=&r" (pstate)
>  			: "i" (PSTATE_IE));
>  	}
> -
> -	set_thread_flag(TIF_POLLING_NRFLAG);
> +	local_irq_enable();
>  }

Nitpick: you can probably move the local_irq_enable() to the
'if' block, since the else block already has assembly code to enable
the interrupts. But anyway its up to you.

This version of your code looks fine to me.

Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>

Regards,
Srivatsa S. Bhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/