Michael Ellerman <m...@ellerman.id.au> writes:
> Daniel Henrique Barboza <danielhb...@gmail.com> writes:
>> This is enough to say that we can't easily see the history behind this 
>> comment.
>> I also believe that we're better of without it since it doesn't make sense
>> with the current codebase.
>
> It was added by the original CPU hotplug commit for ppc64::
>
> https://github.com/mpe/linux-fullhistory/commit/0e9fd9441cd2113b67b14e739267c9e69761489b
>
>
> The code was fairly similar:
>
> void __cpu_die(unsigned int cpu)
> {
>       int tries;
>       int cpu_status;
>       unsigned int pcpu = get_hard_smp_processor_id(cpu);
>
>       for (tries = 0; tries < 5; tries++) {
>               cpu_status = query_cpu_stopped(pcpu);
>
>               if (cpu_status == 0)
>                       break;
>               set_current_state(TASK_UNINTERRUPTIBLE);
>               schedule_timeout(HZ);
>       }
>       if (cpu_status != 0) {
>               printk("Querying DEAD? cpu %i (%i) shows %i\n",
>                      cpu, pcpu, cpu_status);
>       }
>
>       /* Isolation and deallocation are definatly done by
>        * drslot_chrp_cpu.  If they were not they would be
>        * done here.  Change isolate state to Isolate and
>        * change allocation-state to Unusable.
>        */
>       paca[cpu].xProcStart = 0;
>
>       /* So we can recognize if it fails to come up next time. */
>       cpu_callin_map[cpu] = 0;
> }
>
>
> drslot_chrp_cpu() still exists in drmgr:
>
>   
> https://github.com/ibm-power-utilities/powerpc-utils/blob/e798c4a09fbf0fa0f421e624cfa366a6c405c9fe/src/drmgr/drslot_chrp_cpu.c#L406
>
>
> I agree the comment is no longer meaningful and can be removed.

Thanks for providing this background.

> It might be good to then add a comment explaining why we need to set
> cpu_start = 0.

Sure, I can take that as a follow-up. Or perhaps it should be moved to
the online path.

> It's not immediately clear why we need to. When we bring a CPU back
> online in smp_pSeries_kick_cpu() we ask RTAS to start it and then
> immediately set cpu_start = 1, ie. there isn't a separate step that sets
> cpu_start = 1 for hotplugged CPUs.

Hmm I'm not following the distinction you seem to be drawing between
bringing a CPU back online and a hotplugged CPU. kick_cpu is used in all
cases AFAIK.

Reply via email to