Re: [PATCH] sparc: Use generic idle loop

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 03/30/2013 01:59 AM, Sam Ravnborg wrote:
> Add generic cpu_idle support
> 
> sparc32:
> - replace call to cpu_idle() with cpu_startup_entry()
> - add arch_cpu_idle()
> 
> sparc64:
> - smp_callin() includes cpu_startup_entry() call so we can
>   skip calling cpu_idle from assembler
> - add arch_cpu_idle_enter() and arch_cpu_idle_dead()
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: David S. Miller <davem@davemloft.net>
> 
> ---
> This patch is on top of smp/testing in the tip tree.
> It is build tested on sparc32 + sparc64
> 
> 	Sam
> 
[...]
> 
> diff --git a/arch/sparc/kernel/process_32.c b/arch/sparc/kernel/process_32.c
> index 62eede1..ec92959 100644
> --- a/arch/sparc/kernel/process_32.c
> +++ b/arch/sparc/kernel/process_32.c
> @@ -64,23 +64,13 @@ extern void fpsave(unsigned long *, unsigned long *, void *, unsigned long *);
>  struct task_struct *last_task_used_math = NULL;
>  struct thread_info *current_set[NR_CPUS];
> 
> -/*
> - * the idle loop on a Sparc... ;)
> - */
> -void cpu_idle(void)
> +/* the idle loop on a Sparc... ;) */
> +void arch_cpu_idle(void)
>  {
> -	set_thread_flag(TIF_POLLING_NRFLAG);
> -
> -	/* endless idle loop with no priority at all */
> -	for (;;) {
> -		while (!need_resched()) {
> -			if (sparc_idle)
> -				(*sparc_idle)();
> -			else
> -				cpu_relax();
> -		}
> -		schedule_preempt_disabled();
> -	}
> +	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.

> 
>  /* 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..5ed6b02 100644
> --- a/arch/sparc/kernel/process_64.c
> +++ b/arch/sparc/kernel/process_64.c
> @@ -52,8 +52,13 @@
> 
>  #include "kstack.h"
> 
> -static void sparc64_yield(int cpu)
> +/* The idle loop on sparc64. */
> +void arch_cpu_idle_enter()
>  {
> +	int cpu;
> +
> +	cpu = smp_processor_id();
> +
>  	if (tlb_type != hypervisor) {
>  		touch_nmi_watchdog();
>  		return;
> @@ -88,31 +93,10 @@ static void sparc64_yield(int cpu)
>  	set_thread_flag(TIF_POLLING_NRFLAG);
>  }
> 

Hmm, this looks weird. The contents of this function should have
been inside arch_cpu_idle() (not arch_cpu_idle_enter). And it is
unnecessary to set/clear POLLING flags here, since they are handled
in the generic code. (Same is the case with the need_resched and
the cpu_is_offline check).

I agree that the naming is a little subtle, but in the current
scheme of things in this patchset, arch_cpu_idle_enter() is what
you call to do initialization _before_ you perform cpu idle, whereas
arch_cpu_idle() is the function that is supposed to do the actual
arch-specific idling.

Regards,
Srivatsa S. Bhat

> -/* The idle loop on sparc64. */
> -void cpu_idle(void)
> +void arch_cpu_idle_dead()
>  {
> -	int cpu = smp_processor_id();
> -
> -	set_thread_flag(TIF_POLLING_NRFLAG);
> -
> -	while(1) {
> -		tick_nohz_idle_enter();
> -		rcu_idle_enter();
> -
> -		while (!need_resched() && !cpu_is_offline(cpu))
> -			sparc64_yield(cpu);
> -
> -		rcu_idle_exit();
> -		tick_nohz_idle_exit();
> -
> -#ifdef CONFIG_HOTPLUG_CPU
> -		if (cpu_is_offline(cpu)) {
> -			sched_preempt_enable_no_resched();
> -			cpu_play_dead();
> -		}
> -#endif
> -		schedule_preempt_disabled();
> -	}
> +	sched_preempt_enable_no_resched();
> +	cpu_play_dead();
>  }