From: "Russell King (Oracle)" <linux@armlinux.org.uk> To: Thomas Gleixner <tglx@linutronix.de> Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, 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, kvmarm@lists.linux.dev, x86@kernel.org, Miguel Luis <miguel.luis@oracle.com>, James Morse <james.morse@arm.com>, Salil Mehta <salil.mehta@huawei.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, linuxarm@huawei.com, justin.he@arm.com, jianyong.wu@arm.com Subject: Re: [PATCH v5 03/18] ACPI: processor: Register deferred CPUs from acpi_processor_get_info() Date: Fri, 12 Apr 2024 22:52:59 +0100 [thread overview] Message-ID: <ZhmtO6zBExkQGZLk@shell.armlinux.org.uk> (raw) In-Reply-To: <87bk6ez4hj.ffs@tglx> On Fri, Apr 12, 2024 at 10:54:32PM +0200, Thomas Gleixner wrote: > On Fri, Apr 12 2024 at 21:16, Russell King (Oracle) wrote: > > On Fri, Apr 12, 2024 at 08:30:40PM +0200, Rafael J. Wysocki wrote: > >> Say acpi_map_cpu) / acpi_unmap_cpu() are turned into arch calls. > >> What's the difference then? The locking, which should be fine if I'm > >> not mistaken and need_hotplug_init that needs to be set if this code > >> runs after the processor driver has loaded AFAICS. > > > > It is over this that I walked away from progressing this code, because > > I don't think it's quite as simple as you make it out to be. > > > > Yes, acpi_map_cpu() and acpi_unmap_cpu() are already arch implemented > > functions, so Arm64 can easily provide stubs for these that do nothing. > > That never caused me any concern. > > > > What does cause me great concern though are the finer details. For > > example, above you seem to drop the evaluation of _STA for the > > "make_present" case - I've no idea whether that is something that > > should be deleted or not (if it is something that can be deleted, > > then why not delete it now?) > > > > As for the cpu locking, I couldn't find anything in arch_register_cpu() > > that depends on the cpu_maps_update stuff nor needs the cpus_write_lock > > being taken - so I've no idea why the "make_present" case takes these > > locks. > > Anything which updates a CPU mask, e.g. cpu_present_mask, after early > boot must hold the appropriate write locks. Otherwise it would be > possible to online a CPU which just got marked present, but the > registration has not completed yet. Yes. As far as I've been able to determine, arch_register_cpu() doesn't manipulate any of the CPU masks. All it seems to be doing is initialising the struct cpu, registering the embedded struct device, and setting up the sysfs links to its NUMA node. There is nothing obvious in there which manipulates any CPU masks, and this is rather my fundamental point when I said "I couldn't find anything in arch_register_cpu() that depends on ...". If there is something, then comments in the code would be a useful aid because it's highly non-obvious where such a manipulation is located, and hence why the locks are necessary. > > Finally, the "pr->flags.need_hotplug_init = 1" thing... it's not > > obvious that this is required - remember that with Arm64's "enabled" > > toggling, the "processor" is a slice of the system and doesn't > > actually go away - it's just "not enabled" for use. > > > > Again, as "processors" in Arm64 are slices of the system, they have > > to be fully described in ACPI before the OS boots, and they will be > > marked as being "present", which means they will be enumerated, and > > the driver will be probed. Any processor that is not to be used will > > not have its enabled bit set. It is my understanding that every > > processor will result in the ACPI processor driver being bound to it > > whether its enabled or not. > > > > The difference between real hotplug and Arm64 hotplug is that real > > hotplug makes stuff not-present (and thus unenumerable). Arm64 hotplug > > makes stuff not-enabled which is still enumerable. > > Define "real hotplug" :) > > Real physical hotplug does not really exist. That's at least true for > x86, where the physical hotplug support was chased for a while, but > never ended up in production. > > Though virtualization happily jumped on it to hot add/remove CPUs > to/from a guest. > > There are limitations to this and we learned it the hard way on X86. At > the end we came up with the following restrictions: > > 1) All possible CPUs have to be advertised at boot time via firmware > (ACPI/DT/whatever) independent of them being present at boot time > or not. > > That guarantees proper sizing and ensures that associations > between hardware entities and software representations and the > resulting topology are stable for the lifetime of a system. > > It is really required to know the full topology of the system at > boot time especially with hybrid CPUs where some of the cores > have hyperthreading and the others do not. > > > 2) Hot add can only mark an already registered (possible) CPU > present. Adding non-registered CPUs after boot is not possible. > > The CPU must have been registered in #1 already to ensure that > the system topology does not suddenly change in an incompatible > way at run-time. > > The same restriction would apply to real physical hotplug. I don't think > that's any different for ARM64 or any other architecture. This makes me wonder whether the Arm64 has been barking up the wrong tree then, and whether the whole "present" vs "enabled" thing comes from a misunderstanding as far as a CPU goes. However, there is a big difference between the two. On x86, a processor is just a processor. On Arm64, a "processor" is a slice of the system (includes the interrupt controller, PMUs etc) and we must enumerate those even when the processor itself is not enabled. This is the whole reason there's a difference between "present" and "enabled" and why there's a difference between x86 cpu hotplug and arm64 cpu hotplug. The processor never actually goes away in arm64, it's just prevented from being used. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
WARNING: multiple messages have this Message-ID (diff)
From: "Russell King (Oracle)" <linux@armlinux.org.uk> To: Thomas Gleixner <tglx@linutronix.de> Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, 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, kvmarm@lists.linux.dev, x86@kernel.org, Miguel Luis <miguel.luis@oracle.com>, James Morse <james.morse@arm.com>, Salil Mehta <salil.mehta@huawei.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, linuxarm@huawei.com, justin.he@arm.com, jianyong.wu@arm.com Subject: Re: [PATCH v5 03/18] ACPI: processor: Register deferred CPUs from acpi_processor_get_info() Date: Fri, 12 Apr 2024 22:52:59 +0100 [thread overview] Message-ID: <ZhmtO6zBExkQGZLk@shell.armlinux.org.uk> (raw) In-Reply-To: <87bk6ez4hj.ffs@tglx> On Fri, Apr 12, 2024 at 10:54:32PM +0200, Thomas Gleixner wrote: > On Fri, Apr 12 2024 at 21:16, Russell King (Oracle) wrote: > > On Fri, Apr 12, 2024 at 08:30:40PM +0200, Rafael J. Wysocki wrote: > >> Say acpi_map_cpu) / acpi_unmap_cpu() are turned into arch calls. > >> What's the difference then? The locking, which should be fine if I'm > >> not mistaken and need_hotplug_init that needs to be set if this code > >> runs after the processor driver has loaded AFAICS. > > > > It is over this that I walked away from progressing this code, because > > I don't think it's quite as simple as you make it out to be. > > > > Yes, acpi_map_cpu() and acpi_unmap_cpu() are already arch implemented > > functions, so Arm64 can easily provide stubs for these that do nothing. > > That never caused me any concern. > > > > What does cause me great concern though are the finer details. For > > example, above you seem to drop the evaluation of _STA for the > > "make_present" case - I've no idea whether that is something that > > should be deleted or not (if it is something that can be deleted, > > then why not delete it now?) > > > > As for the cpu locking, I couldn't find anything in arch_register_cpu() > > that depends on the cpu_maps_update stuff nor needs the cpus_write_lock > > being taken - so I've no idea why the "make_present" case takes these > > locks. > > Anything which updates a CPU mask, e.g. cpu_present_mask, after early > boot must hold the appropriate write locks. Otherwise it would be > possible to online a CPU which just got marked present, but the > registration has not completed yet. Yes. As far as I've been able to determine, arch_register_cpu() doesn't manipulate any of the CPU masks. All it seems to be doing is initialising the struct cpu, registering the embedded struct device, and setting up the sysfs links to its NUMA node. There is nothing obvious in there which manipulates any CPU masks, and this is rather my fundamental point when I said "I couldn't find anything in arch_register_cpu() that depends on ...". If there is something, then comments in the code would be a useful aid because it's highly non-obvious where such a manipulation is located, and hence why the locks are necessary. > > Finally, the "pr->flags.need_hotplug_init = 1" thing... it's not > > obvious that this is required - remember that with Arm64's "enabled" > > toggling, the "processor" is a slice of the system and doesn't > > actually go away - it's just "not enabled" for use. > > > > Again, as "processors" in Arm64 are slices of the system, they have > > to be fully described in ACPI before the OS boots, and they will be > > marked as being "present", which means they will be enumerated, and > > the driver will be probed. Any processor that is not to be used will > > not have its enabled bit set. It is my understanding that every > > processor will result in the ACPI processor driver being bound to it > > whether its enabled or not. > > > > The difference between real hotplug and Arm64 hotplug is that real > > hotplug makes stuff not-present (and thus unenumerable). Arm64 hotplug > > makes stuff not-enabled which is still enumerable. > > Define "real hotplug" :) > > Real physical hotplug does not really exist. That's at least true for > x86, where the physical hotplug support was chased for a while, but > never ended up in production. > > Though virtualization happily jumped on it to hot add/remove CPUs > to/from a guest. > > There are limitations to this and we learned it the hard way on X86. At > the end we came up with the following restrictions: > > 1) All possible CPUs have to be advertised at boot time via firmware > (ACPI/DT/whatever) independent of them being present at boot time > or not. > > That guarantees proper sizing and ensures that associations > between hardware entities and software representations and the > resulting topology are stable for the lifetime of a system. > > It is really required to know the full topology of the system at > boot time especially with hybrid CPUs where some of the cores > have hyperthreading and the others do not. > > > 2) Hot add can only mark an already registered (possible) CPU > present. Adding non-registered CPUs after boot is not possible. > > The CPU must have been registered in #1 already to ensure that > the system topology does not suddenly change in an incompatible > way at run-time. > > The same restriction would apply to real physical hotplug. I don't think > that's any different for ARM64 or any other architecture. This makes me wonder whether the Arm64 has been barking up the wrong tree then, and whether the whole "present" vs "enabled" thing comes from a misunderstanding as far as a CPU goes. However, there is a big difference between the two. On x86, a processor is just a processor. On Arm64, a "processor" is a slice of the system (includes the interrupt controller, PMUs etc) and we must enumerate those even when the processor itself is not enabled. This is the whole reason there's a difference between "present" and "enabled" and why there's a difference between x86 cpu hotplug and arm64 cpu hotplug. The processor never actually goes away in arm64, it's just prevented from being used. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-04-12 21:53 UTC|newest] Thread overview: 116+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-04-12 14:37 [PATCH v5 00/18] ACPI/arm64: add support for virtual cpu hotplug Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 01/18] cpu: Do not warn on arch_register_cpu() returning -EPROBE_DEFER Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 17:42 ` Rafael J. Wysocki 2024-04-12 17:42 ` Rafael J. Wysocki 2024-04-22 3:53 ` Gavin Shan 2024-04-22 3:53 ` Gavin Shan 2024-04-12 14:37 ` [PATCH v5 02/18] ACPI: processor: Set the ACPI_COMPANION for the struct cpu instance Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 18:10 ` Rafael J. Wysocki 2024-04-12 18:10 ` Rafael J. Wysocki 2024-04-15 15:48 ` Jonathan Cameron 2024-04-15 15:48 ` Jonathan Cameron 2024-04-15 16:16 ` Rafael J. Wysocki 2024-04-15 16:16 ` Rafael J. Wysocki 2024-04-15 16:19 ` Rafael J. Wysocki 2024-04-15 16:19 ` Rafael J. Wysocki 2024-04-15 16:50 ` Jonathan Cameron 2024-04-15 16:50 ` Jonathan Cameron 2024-04-15 17:34 ` Jonathan Cameron 2024-04-15 17:34 ` Jonathan Cameron 2024-04-15 17:41 ` Rafael J. Wysocki 2024-04-15 17:41 ` Rafael J. Wysocki 2024-04-16 17:35 ` Jonathan Cameron 2024-04-16 17:35 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 03/18] ACPI: processor: Register deferred CPUs from acpi_processor_get_info() Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 18:30 ` Rafael J. Wysocki 2024-04-12 18:30 ` Rafael J. Wysocki 2024-04-12 20:16 ` Russell King (Oracle) 2024-04-12 20:16 ` Russell King (Oracle) 2024-04-12 20:54 ` Thomas Gleixner 2024-04-12 20:54 ` Thomas Gleixner 2024-04-12 21:52 ` Russell King (Oracle) [this message] 2024-04-12 21:52 ` Russell King (Oracle) 2024-04-12 23:23 ` Thomas Gleixner 2024-04-12 23:23 ` Thomas Gleixner 2024-04-15 8:45 ` Jonathan Cameron 2024-04-15 8:45 ` Jonathan Cameron 2024-04-15 9:16 ` Jonathan Cameron 2024-04-15 9:16 ` Jonathan Cameron 2024-04-15 9:31 ` Jonathan Cameron 2024-04-15 9:31 ` Jonathan Cameron 2024-04-15 11:57 ` Jonathan Cameron 2024-04-15 11:57 ` Jonathan Cameron 2024-04-15 11:37 ` Rafael J. Wysocki 2024-04-15 11:37 ` Rafael J. Wysocki 2024-04-15 11:56 ` Jonathan Cameron 2024-04-15 11:56 ` Jonathan Cameron 2024-04-15 12:04 ` Rafael J. Wysocki 2024-04-15 12:04 ` Rafael J. Wysocki 2024-04-15 12:23 ` Jonathan Cameron 2024-04-15 12:23 ` Jonathan Cameron 2024-04-16 17:41 ` Jonathan Cameron 2024-04-16 17:41 ` Jonathan Cameron 2024-04-16 19:02 ` Rafael J. Wysocki 2024-04-16 19:02 ` Rafael J. Wysocki 2024-04-17 10:39 ` Jonathan Cameron 2024-04-17 10:39 ` Jonathan Cameron 2024-04-15 12:37 ` Salil Mehta 2024-04-15 12:37 ` Salil Mehta 2024-04-15 12:41 ` Rafael J. Wysocki 2024-04-15 12:41 ` Rafael J. Wysocki 2024-04-15 11:51 ` Salil Mehta 2024-04-15 11:51 ` Salil Mehta 2024-04-15 12:51 ` Rafael J. Wysocki 2024-04-15 12:51 ` Rafael J. Wysocki 2024-04-15 15:31 ` Salil Mehta 2024-04-15 15:31 ` Salil Mehta 2024-04-15 16:38 ` Rafael J. Wysocki 2024-04-15 16:38 ` Rafael J. Wysocki 2024-04-17 15:01 ` Salil Mehta 2024-04-17 15:01 ` Salil Mehta 2024-04-17 16:19 ` Rafael J. Wysocki 2024-04-17 16:19 ` Rafael J. Wysocki 2024-04-15 10:52 ` Jonathan Cameron 2024-04-15 10:52 ` Jonathan Cameron 2024-04-15 11:11 ` Jonathan Cameron 2024-04-15 11:11 ` Jonathan Cameron 2024-04-15 11:52 ` Rafael J. Wysocki 2024-04-15 11:52 ` Rafael J. Wysocki 2024-04-15 11:07 ` Salil Mehta 2024-04-15 11:07 ` Salil Mehta 2024-04-16 14:00 ` Jonathan Cameron 2024-04-16 14:00 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 04/18] ACPI: Rename acpi_processor_hotadd_init and remove pre-processor guards Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 05/18] ACPI: utils: Add an acpi_sta_enabled() helper and use it in acpi_processor_make_present() Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 06/18] ACPI: scan: Add parameter to allow defering some actions in acpi_scan_check_and_detach Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 07/18] ACPI: Add post_eject to struct acpi_scan_handler for cpu hotplug Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 08/18] ACPI: convert acpi_processor_post_eject() to use IS_ENABLED() Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 09/18] ACPI: Check _STA present bit before making CPUs not present Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 10/18] ACPI: Warn when the present bit changes but the feature is not enabled Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 11/18] arm64: acpi: Move get_cpu_for_acpi_id() to a header Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 12/18] irqchip/gic-v3: Don't return errors from gic_acpi_match_gicc() Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 13/18] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 14/18] arm64: psci: Ignore DENIED CPUs Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 15/18] arm64: arch_register_cpu() variant to allow checking of ACPI _STA Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 16/18] ACPI: add support to (un)register CPUs based on the _STA enabled bit Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 17/18] arm64: document virtual CPU hotplug's expectations Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron 2024-04-12 14:37 ` [PATCH v5 18/18] cpumask: Add enabled cpumask for present CPUs that can be brought online Jonathan Cameron 2024-04-12 14:37 ` Jonathan Cameron
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=ZhmtO6zBExkQGZLk@shell.armlinux.org.uk \ --to=linux@armlinux.org.uk \ --cc=Jonathan.Cameron@huawei.com \ --cc=catalin.marinas@arm.com \ --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-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=linuxarm@huawei.com \ --cc=loongarch@lists.linux.dev \ --cc=miguel.luis@oracle.com \ --cc=rafael@kernel.org \ --cc=salil.mehta@huawei.com \ --cc=tglx@linutronix.de \ --cc=will@kernel.org \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.