Re: [RFC PATCH 2/4] ACPI: processor: refactor acpi_processor_get_info: isolate cpu hotpug init delay

[Miguel Luis]

> On 10 Apr 2024, at 13:20, Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> 
> On Tue,  9 Apr 2024 15:05:31 +0000
> Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:
> 
>> Delaying a hotplugged CPU initialization depends on
>> CONFIG_ACPI_HOTPLUG_CPU. Isolate that.
>> 
>> Signed-off-by: Miguel Luis <miguel.luis@xxxxxxxxxx>
> 
> Again, needs more explanation.

In agreement.

> Post the full set with the v4 vCPU
> HP patches on top of this so we can see how it is used.
> 

I’ll get a link to a repo for the next version besides would like primarily to
establish acpi_processor_{get_info|remove} first since these changes
would need to live with and without vCPU HP.

> I guess the aim here is to share the bulk of this code between
> the present and enabled paths? Whilst I think they should look
> more similar actual code sharing seems like a bad idea for a
> couple of reasons.

That would be my understanding from comments on v4. Both present and
enabled paths do have common procedures up to certain point. IIUC, from .1
and .2 from comments [1] and [2] while .3 would be architecture specific code.

[1]: https://lore.kernel.org/linux-arm-kernel/CAJZ5v0iiJpUWq5GMSnKFWQTzn_bdwoQz9m=hDaXNg4Lj_ePF4g@xxxxxxxxxxxxxx/
[2]: https://lore.kernel.org/linux-arm-kernel/20240322185327.00002416@xxxxxxxxxx/

> 
> Imagine an arch that supports both present and enabled setting (so vCPU HP and
> CPU HP) on that this function will be defined but will not be the right
> thing to do for vCPU HP.  Note that in theory this is true of x86 but no one
> has added support for the 'online capable bit' yet.

… I agree with the above. It reinforces refactoring acpi_processor_get_info
so it clearly decouples present and enabled paths.

> 
> The impression for the _present() path will be that acpi_process_hotplug_delay_init()
> should be called, and that's not true.  That should be obvious in the code
> not hidden behind a stubbed out function.

Ack. Need to check how we’re differentiating both paths.

> 
> Finally, you've pulled acpi_process_enumearte_extra out of the CONFIG_ACPI_HOTPLUG_CPU
> block and I'm fairly sure it still has acpi_map_cpu() calls which aren't
> defined yet for now ACPI_HOTPLUG_CPU configs.

Yep, it still has. Unless you squash the next patch into this one, which I
didn’t so one could see these changes progressively rather than
self-contained.

Miguel

> 
> Jonathan
> 
>> ---
>> drivers/acpi/acpi_processor.c | 34 ++++++++++++++++++----------------
>> 1 file changed, 18 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 37e8b69113dd..9ea58b61d741 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -184,7 +184,22 @@ static void __init acpi_pcc_cpufreq_init(void) {}
>> 
>> /* Initialization */
>> #ifdef CONFIG_ACPI_HOTPLUG_CPU
>> -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>> +static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
>> +{
>> + /*
>> + * CPU got hot-added, but cpu_data is not initialized yet.  Set a flag
>> + * to delay cpu_idle/throttling initialization and do it when the CPU
>> + * gets online for the first time.
>> + */
>> + pr_info("CPU%d has been hot-added\n", pr->id);
>> + pr->flags.need_hotplug_init = 1;
>> +}
>> +#else
>> +static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
>> +#endif /* CONFIG_ACPI_HOTPLUG_CPU */
>> +
>> +/* Enumerate extra CPUs */
>> +static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
>> {
>> unsigned long long sta;
>> acpi_status status;
>> @@ -210,25 +225,12 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>> goto out;
>> }
>> 
>> - /*
>> - * CPU got hot-added, but cpu_data is not initialized yet.  Set a flag
>> - * to delay cpu_idle/throttling initialization and do it when the CPU
>> - * gets online for the first time.
>> - */
>> - pr_info("CPU%d has been hot-added\n", pr->id);
>> - pr->flags.need_hotplug_init = 1;
>> -
>> + acpi_processor_hotplug_delay_init(pr);
>> out:
>> cpus_write_unlock();
>> cpu_maps_update_done();
>> return ret;
>> }
>> -#else
>> -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
>> -{
>> - return -ENODEV;
>> -}
>> -#endif /* CONFIG_ACPI_HOTPLUG_CPU */
>> 
>> static int acpi_evaluate_processor(struct acpi_device *device,
>>   struct acpi_processor *pr,
>> @@ -347,7 +349,7 @@ static int acpi_processor_get_info(struct acpi_device *device)
>> *  because cpuid <-> apicid mapping is persistent now.
>> */
>> if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
>> - int ret = acpi_processor_hotadd_init(pr);
>> + int ret = acpi_processor_enumerate_extra(pr);
>> 
>> if (ret)
>> return ret;
>