linux-ia64.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Cc: <linux-pm@vger.kernel.org>, <loongarch@lists.linux.dev>,
	<linux-acpi@vger.kernel.org>, <linux-arch@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-riscv@lists.infradead.org>, <kvmarm@lists.linux.dev>,
	<x86@kernel.org>, <acpica-devel@lists.linuxfoundation.org>,
	<linux-csky@vger.kernel.org>, <linux-doc@vger.kernel.org>,
	<linux-ia64@vger.kernel.org>, <linux-parisc@vger.kernel.org>,
	Salil Mehta <salil.mehta@huawei.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	<jianyong.wu@arm.com>, <justin.he@arm.com>,
	James Morse <james.morse@arm.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Miguel Luis <miguel.luis@oracle.com>
Subject: Re: [PATCH RFC v4 12/15] arm64: psci: Ignore DENIED CPUs
Date: Thu, 11 Apr 2024 14:25:30 +0100	[thread overview]
Message-ID: <20240411142530.00007617@huawei.com> (raw)
In-Reply-To: <20240411123523.0000487a@huawei.com>

On Thu, 11 Apr 2024 12:35:23 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Wed, 31 Jan 2024 16:50:38 +0000
> Russell King (Oracle) <rmk+kernel@armlinux.org.uk> wrote:
> 
> > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > 
> > When a CPU is marked as disabled, but online capable in the MADT, PSCI
> > applies some firmware policy to control when it can be brought online.
> > PSCI returns DENIED to a CPU_ON request if this is not currently
> > permitted. The OS can learn the current policy from the _STA enabled bit.
> > 
> > Handle the PSCI DENIED return code gracefully instead of printing an
> > error.
> > 
> > See https://developer.arm.com/documentation/den0022/f/?lang=en page 58.
> > 
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > [ morse: Rewrote commit message ]
> > Signed-off-by: James Morse <james.morse@arm.com>
> > Tested-by: Miguel Luis <miguel.luis@oracle.com>
> > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> > Tested-by: Jianyong Wu <jianyong.wu@arm.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---  
> 
> This change to return failure from __cpu_up in non error cases exposes
> an possible issue with cpu_up() in kernel/cpu.c in that it brings the numa node
> before we try (and fail) to bring up CPUs that may be denied.
> 
> We could try offlining the numa node on error, or just register it later.
> 
> Currently I'm testing this change which I think is harmless for cases that don't
> fail the cpu_up()
> 
> For the cpu hotplug path note the node only comes online wiht the cpu online, not
> the earlier hotplug. Reasonable given there is nothing online in the node before
> that point.
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 537099bf5d02..a4730396ccea 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1742,10 +1742,6 @@ static int cpu_up(unsigned int cpu, enum cpuhp_state target)
>                 return -EINVAL;
>         }
> 
> -       err = try_online_node(cpu_to_node(cpu));
> -       if (err)
> -               return err;
> -
>         cpu_maps_update_begin();
> 
>         if (cpu_hotplug_disabled) {
> @@ -1760,7 +1756,10 @@ static int cpu_up(unsigned int cpu, enum cpuhp_state target)
>         err = _cpu_up(cpu, 0, target);
>  out:
>         cpu_maps_update_done();
> -       return err;
> +       if (err)
> +               return err;
> +
> +       return try_online_node(cpu_to_node(cpu));
>  }
> 
> There is a kicker in the remove path where currently check_cpu_on_node()
> checks for_each_present_cpu() whereas to work for us we need to use
> for_each_online_cpu() or the node is never removed.
> 
> Now my current view is that we should only show
> nodes in /sys/bus/nodes/devices/ if there is a CPU online (assuming no other
> reasons the node should be online such as memory).
> That's easy enough to make work but all I'm really learning is that the semantics
> of what is an online form a node point of view is not consistent.
> 
> Fixing this will create a minor change on x86 but does anyone really care
> about what happens in the offline path wrt to 'when' the node disappears?
> I think the corner case is.
> 
> 1. Add 2 CPUs (A, B) in a CPU only node.
> 2. Online CPU A - this brings /sys/bus/devices/nodeX online
> 3. Remove CPU A - no effect because the check for try_remove is on presence and CPU B is
>    still present.
> 4. Online CPU B - no change.
> 5. Offline CPU B - no change.
> 4. Remove CPU B - /sys/bus/device/nodeX offline (disappears)
> 
> To make it work on arm64 where we never make CPUs not present.
> 
> 1. Add 2 CPUs (A, B) in a CPU only node.
> 2. Online CPU A - this brings /sys/bus/devices/nodeX online
> 3. Remove CPU A - /sys/bus/devices/nodeX offline (disappears)
> 4. Online CPU B - this brings /sys/bus/device/nodeX online
> 5. Offline CPU B - no change (node updates only happen in hotplug code)
> 6. Remove CPU B - /sys/bus/device/nodeX offline (disappears).
> 
> Step 5 may seem weird but I think we can't mess with nodes there because
> userspace may well rely on them still being around for some reason
> (it's a much more common situation).
> 
> My assumption is that step3 removing the node isn't going to hurt anyone?
> 
> If no one shouts, i'll go ahead with rolling a v5 patch set where this is done.

This needs a little more thought as if we follow above sequence there isn't
currently any cleanup of
/sys/bus/cpu/devices/cpuB/node link.

If you follow the sequence above an attempt is made to create it again.
Whilst I have a WIP bit of dancing code that deals with this it is ugly as
the check on whether a node is online needs to be dropped on this path.
The problem coming back to the tear down and setup paths being in entirely different
states and not remotely symmetric. 

So my proposal for now is to treat the whole numa node problem as something
for another day.

Step 1)
For ARM64 cpu hp - NUMA nodes created at boot for any present CPUs - so all
of them.  They never go away, hence no annoying tear down to do.

Step 2)
We can consider moving to dynamic numa node handling either via the messy
approach I have working, or via more major surgery.
I have a bunch of other work in this area for memory hotplug, so maybe
I can role those 2 activities together.

Jonathan
> 
> 
> Jonathan
> 
> 
> 
> 
> 
> > Changes since RFC v2
> >  * Add specification reference
> >  * Use EPERM rather than EPROBE_DEFER
> > Changes since RFC v3:
> >  * Use EPERM everywhere
> >  * Drop unnecessary changes to drivers/firmware/psci/psci.c
> > ---
> >  arch/arm64/kernel/psci.c | 2 +-
> >  arch/arm64/kernel/smp.c  | 3 ++-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
> > index 29a8e444db83..fabd732d0a2d 100644
> > --- a/arch/arm64/kernel/psci.c
> > +++ b/arch/arm64/kernel/psci.c
> > @@ -40,7 +40,7 @@ static int cpu_psci_cpu_boot(unsigned int cpu)
> >  {
> >  	phys_addr_t pa_secondary_entry = __pa_symbol(secondary_entry);
> >  	int err = psci_ops.cpu_on(cpu_logical_map(cpu), pa_secondary_entry);
> > -	if (err)
> > +	if (err && err != -EPERM)
> >  		pr_err("failed to boot CPU%d (%d)\n", cpu, err);
> >  
> >  	return err;
> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > index 4ced34f62dab..dc0e0b3ec2d4 100644
> > --- a/arch/arm64/kernel/smp.c
> > +++ b/arch/arm64/kernel/smp.c
> > @@ -132,7 +132,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
> >  	/* Now bring the CPU into our world */
> >  	ret = boot_secondary(cpu, idle);
> >  	if (ret) {
> > -		pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
> > +		if (ret != -EPERM)
> > +			pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
> >  		return ret;
> >  	}
> >    
> 


  reply	other threads:[~2024-04-11 13:25 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-31 16:48 [RFC PATCH v4 00/15] ACPI/arm64: add support for virtual cpu hotplug Russell King (Oracle)
2024-01-31 16:49 ` [PATCH RFC v4 01/15] ACPI: Only enumerate enabled (or functional) processor devices Russell King
2024-01-31 17:25   ` Miguel Luis
2024-02-15 20:10   ` Rafael J. Wysocki
2024-02-19  9:45     ` Jonathan Cameron
2024-02-20 11:30     ` Russell King (Oracle)
2024-02-21 13:01   ` Rafael J. Wysocki
2024-01-31 16:49 ` [PATCH RFC v4 02/15] ACPI: processor: Register all CPUs from acpi_processor_get_info() Russell King
2024-02-15 19:22   ` Rafael J. Wysocki
2024-02-20 11:27     ` Russell King (Oracle)
2024-02-20 15:13       ` Russell King (Oracle)
2024-02-20 16:24         ` Jonathan Cameron
2024-02-20 19:59           ` Rafael J. Wysocki
2024-02-21 12:04             ` Rafael J. Wysocki
2024-02-20 20:59     ` Thomas Gleixner
2024-03-22 18:53     ` Jonathan Cameron
2024-04-10 12:43       ` Jonathan Cameron
2024-04-10 13:28         ` Rafael J. Wysocki
2024-04-10 13:50           ` Jonathan Cameron
2024-04-10 14:19             ` Rafael J. Wysocki
2024-04-10 15:58               ` Jonathan Cameron
2024-04-10 18:56             ` Russell King (Oracle)
2024-04-10 19:08               ` Rafael J. Wysocki
2024-04-10 21:07                 ` Jonathan Cameron
2024-01-31 16:49 ` [PATCH RFC v4 03/15] ACPI: Move acpi_bus_trim_one() before acpi_scan_hot_remove() Russell King
2024-01-31 16:49 ` [PATCH RFC v4 04/15] ACPI: Rename acpi_processor_hotadd_init and remove pre-processor guards Russell King
2024-01-31 16:50 ` [PATCH RFC v4 05/15] ACPI: Add post_eject to struct acpi_scan_handler for cpu hotplug Russell King
2024-01-31 16:50 ` [PATCH RFC v4 06/15] ACPI: convert acpi_processor_post_eject() to use IS_ENABLED() Russell King (Oracle)
2024-01-31 16:50 ` [PATCH RFC v4 07/15] ACPI: Check _STA present bit before making CPUs not present Russell King
2024-01-31 16:50 ` [PATCH RFC v4 08/15] ACPI: Warn when the present bit changes but the feature is not enabled Russell King
2024-01-31 16:50 ` [PATCH RFC v4 09/15] arm64: acpi: Move get_cpu_for_acpi_id() to a header Russell King
2024-01-31 16:50 ` [PATCH RFC v4 10/15] irqchip/gic-v3: Don't return errors from gic_acpi_match_gicc() Russell King
2024-02-02 16:44   ` Jonathan Cameron
2024-01-31 16:50 ` [PATCH RFC v4 11/15] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs Russell King
2024-02-02 16:47   ` Jonathan Cameron
2024-01-31 16:50 ` [PATCH RFC v4 12/15] arm64: psci: Ignore DENIED CPUs Russell King
2024-04-11 11:35   ` Jonathan Cameron
2024-04-11 13:25     ` Jonathan Cameron [this message]
2024-01-31 16:50 ` [PATCH RFC v4 13/15] ACPI: add support to (un)register CPUs based on the _STA enabled bit Russell King
2024-01-31 16:50 ` [PATCH RFC v4 14/15] arm64: document virtual CPU hotplug's expectations Russell King
2024-02-02 17:04   ` Jonathan Cameron
2024-01-31 16:50 ` [PATCH RFC v4 15/15] cpumask: Add enabled cpumask for present CPUs that can be brought online Russell King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240411142530.00007617@huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=acpica-devel@lists.linuxfoundation.org \
    --cc=james.morse@arm.com \
    --cc=jean-philippe@linaro.org \
    --cc=jianyong.wu@arm.com \
    --cc=justin.he@arm.com \
    --cc=kvmarm@lists.linux.dev \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-csky@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-ia64@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-parisc@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=loongarch@lists.linux.dev \
    --cc=miguel.luis@oracle.com \
    --cc=rafael@kernel.org \
    --cc=rmk+kernel@armlinux.org.uk \
    --cc=salil.mehta@huawei.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).