On Tue, Jul 04, 2017 at 10:20:23PM +0200, Thomas Gleixner wrote:
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -271,11 +271,25 @@ void cpu_hotplug_enable(void)
> EXPORT_SYMBOL_GPL(cpu_hotplug_enable);
> #endif /* CONFIG_HOTPLUG_CPU */
>
> +static void __cpuhp_kick_ap_work(struct cpuhp_cpu_state *st);
> +
> static int bringup_wait_for_ap(unsigned int cpu)
> {
> struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
>
> + /* Wait for the CPU to reach IDLE_ONLINE */
> wait_for_completion(&st->done);
> + BUG_ON(!cpu_online(cpu));
> +
> + /* Unpark the stopper thread and the hotplug thread of the target 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);
> + wait_for_completion(&st->done);
> + }
> return st->result;
> }
>
> @@ -296,9 +310,7 @@ static int bringup_cpu(unsigned int cpu)
> irq_unlock_sparse();
> if (ret)
> return ret;
> - ret = bringup_wait_for_ap(cpu);
> - BUG_ON(!cpu_online(cpu));
> - return ret;
> + return bringup_wait_for_ap(cpu);
> }
>
> /*
> @@ -775,23 +787,13 @@ void notify_cpu_starting(unsigned int cp
The comment right above this function now seems stale..
> 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();
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.
