| Message ID | 20240409150536.9933-3-miguel.luis@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: processor: refactor acpi_processor_{get_info|remove} | expand |
> On 10 Apr 2024, at 13:20, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > On Tue, 9 Apr 2024 15:05:31 +0000 > Miguel Luis <miguel.luis@oracle.com> wrote: > >> Delaying a hotplugged CPU initialization depends on >> CONFIG_ACPI_HOTPLUG_CPU. Isolate that. >> >> Signed-off-by: Miguel Luis <miguel.luis@oracle.com> > > 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@mail.gmail.com/ [2]: https://lore.kernel.org/linux-arm-kernel/20240322185327.00002416@Huawei.com/ > > 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; >
On Wed, 10 Apr 2024 17:20:01 +0000 Miguel Luis <miguel.luis@oracle.com> wrote: > > On 10 Apr 2024, at 13:20, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > > > On Tue, 9 Apr 2024 15:05:31 +0000 > > Miguel Luis <miguel.luis@oracle.com> wrote: > > > >> Delaying a hotplugged CPU initialization depends on > >> CONFIG_ACPI_HOTPLUG_CPU. Isolate that. > >> > >> Signed-off-by: Miguel Luis <miguel.luis@oracle.com> > > > > 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. Great. > > > 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@mail.gmail.com/ > [2]: https://lore.kernel.org/linux-arm-kernel/20240322185327.00002416@Huawei.com/ 3 is not just architecture specific code, it's architecture and action specific. i.e. What is done in there should not happen in the present path. From what is in [2] I became much less convinced much code should be shared.. Lightly editing where that thread went today, there is some shared code in the make_present / make_enabled path, but not that much. As per that discussion, cpu_maps_update* is harmless, but also pointless and potentially misleading in the enable case. static int acpi_processor_make_present(struct acpi_processor *pr) { unsigned long long sta; acpi_status status; int ret; if (!IS_ENABLED(CONFIG_ACPI_HOTPLUG_CPU)) { pr_err_once("Changing CPU present bit is not supported\n"); return -ENODEV; } // The _STA check here is needed still or we need to push it into // arch_register_cpu() on x86 similarly to proposal on arm64. status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta); if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT)) return -ENODEV; if (invalid_phys_cpuid(pr->phys_id)) return -ENODEV; cpu_maps_update_begin(); cpus_write_lock(); ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); if (ret) goto out; ret = arch_register_cpu(pr->id); if (ret) { acpi_unmap_cpu(pr->id); 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; out: cpus_write_unlock(); cpu_maps_update_done(); return ret; } static int acpi_processor_make_enabled(struct acpi_processor *pr) { unsigned long long sta; acpi_status status; bool present, enabled; int ret; if (invalid_phys_cpuid(pr->phys_id)) return -ENODEV; cpus_write_lock(); ret = arch_register_cpu(pr->id); cpus_write_unlock(); return ret; } > > > > > 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. I haven't looked as much at the remove path recently but for the enable path the code that should run in the enable path is much less than in the present path. > > > > > 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. > I think that makes it non bisectable, so you can't do this. Either don't move that code until after the next patch, or squash the 2 together. Less important in an RFC though, Jonathan > 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; > > >
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;
Delaying a hotplugged CPU initialization depends on CONFIG_ACPI_HOTPLUG_CPU. Isolate that. Signed-off-by: Miguel Luis <miguel.luis@oracle.com> --- drivers/acpi/acpi_processor.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-)