Re: [PATCH tip/core/rcu 04/20] metag: Use common outgoing-CPU-notification code

[James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>]

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

Hi Paul,

On 03/03/15 17:42, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> This commit removes the open-coded CPU-offline notification with new
> common code.  This change avoids calling scheduler code using RCU from
> an offline CPU that RCU is ignoring.  This commit is compatible with
> the existing code in not checking for timeout during a prior offline
> for a given CPU.
> 
> Signed-off-by: Paul E. McKenney <paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Cc: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Cc: <linux-metag-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

I gave this a try via linux-next, but unfortunately it causes the
following warning every time a CPU goes down:
META213-Thread0 DSP [LogF] CPU1: unable to kill

If I add printks, I see that the state on entry to both cpu_wait_death
and cpu_report_death is already CPU_POST_DEAD, suggesting that it hasn't
changed from its initial value.

Should arches other than x86 now be calling cpu_set_state_online()? The
patchlet below seems to resolve it for Meta (not sure if that is the
best place in the startup sequence to do it, perhaps it doesn't matter).

diff --git a/arch/metag/kernel/smp.c b/arch/metag/kernel/smp.c
index ac3a199e33e7..430e379ec71f 100644
--- a/arch/metag/kernel/smp.c
+++ b/arch/metag/kernel/smp.c
@@ -383,6 +383,7 @@ asmlinkage void secondary_start_kernel(void)
 	 * OK, now it's safe to let the boot CPU continue
 	 */
 	set_cpu_online(cpu, true);
+	cpu_set_state_online(cpu);
 	complete(&cpu_running);
 
 	/*

Looking at the comment before cpu_set_state_online:
> /*
>  * Mark the specified CPU online.
>  *
>  * Note that it is permissible to omit this call entirely, as is
>  * done in architectures that do no CPU-hotplug error checking.
>  */

Which suggests it wasn't wrong to omit it before your patches came
along.

Cheers
James


> ---
>  arch/metag/kernel/smp.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/metag/kernel/smp.c b/arch/metag/kernel/smp.c
> index f006d2276f40..ac3a199e33e7 100644
> --- a/arch/metag/kernel/smp.c
> +++ b/arch/metag/kernel/smp.c
> @@ -261,7 +261,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
>  }
>  
>  #ifdef CONFIG_HOTPLUG_CPU
> -static DECLARE_COMPLETION(cpu_killed);
>  
>  /*
>   * __cpu_disable runs on the processor to be shutdown.
> @@ -299,7 +298,7 @@ int __cpu_disable(void)
>   */
>  void __cpu_die(unsigned int cpu)
>  {
> -	if (!wait_for_completion_timeout(&cpu_killed, msecs_to_jiffies(1)))
> +	if (!cpu_wait_death(cpu, 1))
>  		pr_err("CPU%u: unable to kill\n", cpu);
>  }
>  
> @@ -314,7 +313,7 @@ void cpu_die(void)
>  	local_irq_disable();
>  	idle_task_exit();
>  
> -	complete(&cpu_killed);
> +	(void)cpu_report_death();
>  
>  	asm ("XOR	TXENABLE, D0Re0,D0Re0\n");
>  }
> 


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