Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752566AbdGEJHv (ORCPT <rfc822;w@1wt.eu>);
        Wed, 5 Jul 2017 05:07:51 -0400
Received: from Galois.linutronix.de ([146.0.238.70]:51713 "EHLO
        Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752525AbdGEJHt (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 5 Jul 2017 05:07:49 -0400
Date: Wed, 5 Jul 2017 11:07:34 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Peter Zijlstra <peterz@infradead.org>
cc: Vikram Mulukutla <markivx@codeaurora.org>,
        Rusty Russell <rusty@rustcorp.com.au>, Tejun Heo <tj@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Sebastian Sewior <bigeasy@linutronix.de>
Subject: Re: [PATCH] smp/hotplug: Move unparking of percpu threads to the
 control CPU
In-Reply-To: <20170705090440.hxnpsj2pgxykg3t6@hirez.programming.kicks-ass.net>
Message-ID: <alpine.DEB.2.20.1707051105500.2019@nanos>
References: <alpine.DEB.2.20.1707042218020.2131@nanos> <20170705090440.hxnpsj2pgxykg3t6@hirez.programming.kicks-ass.net>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1531
Lines: 62

On Wed, 5 Jul 2017, Peter Zijlstra wrote:
> On Tue, Jul 04, 2017 at 10:20:23PM +0200, Thomas Gleixner wrote:
> > @@ -775,23 +787,13 @@ void notify_cpu_starting(unsigned int cp
> 
> The comment right above this function now seems stale..

Will fix.

> >  void cpuhp_online_idle(enum cpuhp_state state)
> >  {
> >  	struct cpuhp_cpu_state *st = this_cpu_ptr(&cpuhp_state);
> > -	unsigned int cpu = smp_processor_id();
> >  
> >  	/* Happens for the boot cpu */
> >  	if (state != CPUHP_AP_ONLINE_IDLE)
> >  		return;
> >  
> >  	st->state = CPUHP_AP_ONLINE_IDLE;
> > -
> > -	/* Unpark the stopper thread and the hotplug thread of this cpu */
> > -	stop_machine_unpark(cpu);
> > -	kthread_unpark(st->thread);
> > -
> > -	/* Should we go further up ? */
> > -	if (st->target > CPUHP_AP_ONLINE_IDLE)
> > -		__cpuhp_kick_ap_work(st);
> > -	else
> > -		complete(&st->done);
> > +	complete(&st->done);
> >  }
> 
> 
> OK, so if I get this right we do something like:
> 
> 
> BP				AP
> 
> bringup_cpu();
>   __cpu_up()  ------------>     /* stuff */
>   bringup_wait_for_ap()
>     wait_for_completion();
> 				cpuhp_online_idle();
> 		<------------    complete(&st->done);
>     unpark()
> 				while(1)
> 				  do_idle();

actually I added after unpark():

     kick_ap()
     wait_for_completion()

So the AP will execute the online callbacks in its own hotplug thread.

> Where you moved the unpark() from the AP's idle thread to the BP's
> context and thus allow scheduling etc..
> 
> Yes that should work fine I think.

Thanks,

	tglx