Message-ID: <20240410204002.00000ce9@Huawei.com>
Date: Wed, 10 Apr 2024 20:40:02 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Miguel Luis <miguel.luis@...cle.com>
CC: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"rmk+kernel@...linux.org.uk" <rmk+kernel@...linux.org.uk>
Subject: Re: [RFC PATCH 2/4] ACPI: processor: refactor
 acpi_processor_get_info: isolate cpu hotpug init delay

On Wed, 10 Apr 2024 17:20:01 +0000
Miguel Luis <miguel.luis@...cle.com> wrote:

> > On 10 Apr 2024, at 13:20, Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> > 
> > On Tue,  9 Apr 2024 15:05:31 +0000
> > Miguel Luis <miguel.luis@...cle.com> wrote:
> >   
> >> Delaying a hotplugged CPU initialization depends on
> >> CONFIG_ACPI_HOTPLUG_CPU. Isolate that.
> >> 
> >> Signed-off-by: Miguel Luis <miguel.luis@...cle.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;  
> >   
> 

Powered by blists - more mailing lists