On Wed, 10 Apr 2024 17:20:01 +0000
Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:
> > 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.
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@xxxxxxxxxxxxxx/
> [2]: https://lore.kernel.org/linux-arm-kernel/20240322185327.00002416@xxxxxxxxxx/
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;
> >
>
