by Jonathan Cameron

[permalink] [raw]

Subject: Re: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU

On Thu, 11 Apr 2024 11:02:37 +0000
Miguel Luis <[email protected]> wrote:

> > On 10 Apr 2024, at 13:31, Jonathan Cameron <[email protected]> wrote:
> >
> > On Tue, 9 Apr 2024 15:05:33 +0000
> > Miguel Luis <[email protected]> wrote:
> >
> >> acpi_unmap_cpu is architecture dependent. Isolate it.
> >> The pre-processor guard for detach may now be restricted to
> >> cpu unmap.
> >>
> >> Signed-off-by: Miguel Luis <[email protected]>
> > Again the why question isn't answered by the patch description.
> >
> > I assume this is to try and resolve the remove question of releasing
> > resources that was outstanding on vCPU HP v4 series Russell posted.
> >
> > I've not looked as closely at the remove path as the add one yet, but
> > my gut feeling is same issue applies.
> > This code that runs in here should not be dependent on whether
> > CONFIG_ACPI_HOTPLUG_CPU is enabled or not.
>
> I agree.
>
> > What we do for the
> > make disabled flow should not run a few of the steps in
> > acpi_processor_remove() we should make that clear by calling
> > a different function that doesn't have those steps.
> >
>
> Perhaps this got answered already elsewhere but is it OK for the detach handler
> to be out of CONFIG_ACPI_HOTPLUG_CPU ?

There is code that is again specific to CONFIG_ACPI_HOTPLUG_CPU and
code that is specific to the disabling only case. So I think the conditions
will end up looking pretty similar to the attach path.
>
> Miguel
>
> >> ---
> >> drivers/acpi/acpi_processor.c | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> >> index c6e2f64a056b..edcd6a8d4735 100644
> >> --- a/drivers/acpi/acpi_processor.c
> >> +++ b/drivers/acpi/acpi_processor.c
> >> @@ -492,6 +492,14 @@ static int acpi_processor_add(struct acpi_device *device,
> >> }
> >>
> >> #ifdef CONFIG_ACPI_HOTPLUG_CPU
> >> +static void acpi_processor_hotunplug_unmap_cpu(struct acpi_processor *pr)
> >> +{
> >> + acpi_unmap_cpu(pr->id);
> >> +}
> >> +#else
> >> +static void acpi_processor_hotunplug_unmap_cpu(struct acpi_processor *pr) {}
> >> +#endif /* CONFIG_ACPI_HOTPLUG_CPU */
> >> +
> >> /* Removal */
> >> static void acpi_processor_remove(struct acpi_device *device)
> >> {
> >> @@ -524,7 +532,7 @@ static void acpi_processor_remove(struct acpi_device *device)
> >>
> >> /* Remove the CPU. */
> >> arch_unregister_cpu(pr->id);
> >> - acpi_unmap_cpu(pr->id);
> >> + acpi_processor_hotunplug_unmap_cpu(pr);
> >>
> >> cpus_write_unlock();
> >> cpu_maps_update_done();
> >> @@ -535,7 +543,6 @@ static void acpi_processor_remove(struct acpi_device *device)
> >> free_cpumask_var(pr->throttling.shared_cpu_map);
> >> kfree(pr);
> >> }
> >> -#endif /* CONFIG_ACPI_HOTPLUG_CPU */
> >>
> >> #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
> >> bool __init processor_physically_present(acpi_handle handle)
> >> @@ -660,9 +667,7 @@ static const struct acpi_device_id processor_device_ids[] = {
> >> static struct acpi_scan_handler processor_handler = {
> >> .ids = processor_device_ids,
> >> .attach = acpi_processor_add,
> >> -#ifdef CONFIG_ACPI_HOTPLUG_CPU
> >> .detach = acpi_processor_remove,
> >> -#endif
> >> .hotplug = {
> >> .enabled = true,
> >> },
> >
> >
>

2024-04-11 17:52:03

by Miguel Luis

[permalink] [raw]

Subject: Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU

> On 11 Apr 2024, at 13:57, Jonathan Cameron <[email protected]> wrote:
>
> On Thu, 11 Apr 2024 10:52:13 +0000
> Miguel Luis <[email protected]> wrote:
>
>>> On 10 Apr 2024, at 19:44, Jonathan Cameron <[email protected]> wrote:
>>>
>>> On Wed, 10 Apr 2024 18:29:34 +0000
>>> Miguel Luis <[email protected]> wrote:
>>>
>>>>> On 10 Apr 2024, at 13:23, Jonathan Cameron <[email protected]> wrote:
>>>>>
>>>>> On Tue, 9 Apr 2024 15:05:32 +0000
>>>>> Miguel Luis <[email protected]> wrote:
>>>>>
>>>>>> mapping and unmaping a cpu at the stage of extra cpu enumeration is
>>>>>> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's
>>>>>> isolate that functionality from architecture independent one.
>>>>>
>>>>> Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu()
>>>>> to make the arch specific nature of that call more obvious?
>>>>
>>>> Not sure about the pattern to use here but that seems fine to me. Current usage
>>>> is architectures export acpi_map_cpu from the acpi interface and do their
>>>> thing.
>>>>
>>>> Question is what to do when there’s a use-case which dismisses acpi_map_cpu and
>>>> it gets called on the code path?
>>>
>>> I'm not sure what you mean by dismisses?
>>>
>>
>> I mean when acpi_map_cpu is not needed.
>>
>>> Is missing perhaps?
>>
>> Yes.
>>
>>> If that is what you mean, I think it's a mistake to allow
>>> that code to be called from a path that isn't dependent on
>>> CONFIG_ACPI_HOTPLUG_CPU.
>>> It makes no sense to do so and stubbing it out to give
>>> the impression that the calling it does make sense (when looking at the caller)
>>> is misleading.
>>
>> OK, that would be what not to do.
>>
>> acpi_processor_enumerate_extra could deal with make_present and make_enabled while
>> a stub would still be needed for make_present since it depends on
>> CONFIG_ACPI_HOTPLUG_CPU?
>
> Sure, you could make it do that with a bunch of checks on the
> config being enabled, but currently I don't see the overlap in
> shared code as being sufficient for that to make sense.
>
> The discussion before was assuming that things like the acpi_map_cpu
> calls might do stuff that is wanted in the make_enabled() case.
>
> Given they don't do anything that we want there I don't see sharing
> the code as useful.
>
> I am however in favor of renaming those hotplug only calls to something
> more meaningful so no one 'thinks' they may be relevant in the
> enabling only case!
>
> Jonathan
>
> p.s. I'm smashing the outputs of the thread with Rafael into a coherent
> patch set at the moment, perhaps seeing that will make it clearer what
> is going on. I got distracted by fixing numa node handling this morning
> but that's now pushed out for a follow on series.
>

Thanks! Looking forward to see v5.

Miguel

>
>>
>> Miguel
>>
>>>
>>> Jonathan
>>>
>>>
>>>>
>>>> 1) export it and do nothing - it would be creating unnecessary dependency.
>>>>
>>>> 2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped
>>>> into CONFIG_ACPI_HOTPLUG_CPU.
>>>>
>>>> Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely
>>>> used for CPU HP and the same applies to acpi_unmap_cpu.
>>>>
>>>>> I think that has caused more confusion in the discussion than
>>>>> whether it is hotplug specific or not.
>>>>
>>>> Indeed. Within the CPU HP path there are these arch specific intricacies.
>>>>
>>>>>
>>>>> As mentioned in patch 2, fairly sure this needs to go before that
>>>>> patch.
>>>>
>>>> 2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late
>>>> CPU initialisation I think.
>>>>
>>>> Miguel
>>>>
>>>>>
>>>>> Jonathan
>>>>>
>>>>>>
>>>>>> Signed-off-by: Miguel Luis <[email protected]>
>>>>>> ---
>>>>>> drivers/acpi/acpi_processor.c | 17 +++++++++++++++--
>>>>>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>>>>>> index 9ea58b61d741..c6e2f64a056b 100644
>>>>>> --- a/drivers/acpi/acpi_processor.c
>>>>>> +++ b/drivers/acpi/acpi_processor.c
>>>>>> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
>>>>>> pr_info("CPU%d has been hot-added\n", pr->id);
>>>>>> pr->flags.need_hotplug_init = 1;
>>>>>> }
>>>>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
>>>>>> +{
>>>>>> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
>>>>>> +}
>>>>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr)
>>>>>> +{
>>>>>> + acpi_unmap_cpu(pr->id);
>>>>>> +}
>>>>>> #else
>>>>>> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
>>>>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
>>>>>> +{
>>>>>> + return 0;
>>>>>> +}
>>>>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {}
>>>>>> #endif /* CONFIG_ACPI_HOTPLUG_CPU */
>>>>>>
>>>>>> /* Enumerate extra CPUs */
>>>>>> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
>>>>>> cpu_maps_update_begin();
>>>>>> cpus_write_lock();
>>>>>>
>>>>>> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
>>>>>> + ret = acpi_processor_hotplug_map_cpu(pr);
>>>>>> if (ret)
>>>>>> goto out;
>>>>>>
>>>>>> ret = arch_register_cpu(pr->id);
>>>>>> if (ret) {
>>>>>> - acpi_unmap_cpu(pr->id);
>>>>>> + acpi_processor_hotplug_unmap_cpu(pr);
>>>>>> goto out;
>>>>>> }