Convert cpu_up/down to device_online/offline

Started by Qais Yousef on Nov 25, 2019 · 3 messages from 1 author · Last reply Feb 5, 2020

Series

14 patches, 18 files

+143 −51

Subsystems (15)

Show 12 more

Maintainers (28)

Show 24 more
STALE2473d SUPERSEDED

Status: Superseded by v4 (posted 2020-03-23). [go to v4]

Revision v2 (4 revisions archived).

Revisions (4)
  1. v1
  2. v2 current [diff against previous]
  3. v3 [diff against previous]
  4. v4 [diff against previous]

open the first message on its own page

  1. [PATCH v2 06/14] powerpc: Replace cpu_up/down with device_online/offline
    Revisions and related patches
    Revisions (4)
    1. v1
    2. v2 current [diff against previous]
    3. v3 [diff against previous]
    4. v4 [diff against previous]
Message details and related content
Changes in v2:
	* Add 2 new patches that create smp_shutdown_nonboot_cpus() to be used
	  in machine_shutdown() in ia64, arm and arm64
	* Use proper kernel-doc for the newly introduced functions
	* Renamed a function
	* Removed a stale comment in a function
	* Rebased on top of 5.4-rc8

	git clone git://linux-arm.org/linux-qy.git -b cpu-hp-cleanup-v2

Using cpu_up/down directly to bring cpus online/offline loses synchronization
with sysfs and could suffer from a race similar to what is described in
commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and serialization
during LPM").

cpu_up/down seem to be more of a internal implementation detail for the cpu
subsystem to use to boot up cpus, perform suspend/resume and low level hotplug
operations. Users outside of the cpu subsystem would be better using the device
core API to bring a cpu online/offline which is the interface used to hotplug
memory and other system devices.

Several users have already migrated to use the device core API, this series
converts the remaining users and hides cpu_up/down from internal users at the
end.

I noticed this problem while working on a hack to disable offlining
a particular CPU but noticed that setting the offline_disabled attribute in the
device struct isn't enough because users can easily bypass the device core.
While my hack isn't a valid use case but it did highlight the inconsistency in
the way cpus are being onlined/offlined and this attempt hopefully improves on
this.

The first 8 patches fix arch users.

The remaining 6 patches fix generic code users. Particularly creating a new
special exported API for the device core to use instead of cpu_up/down.

The last patch removes cpu_up/down from cpu.h and unexport the functions.

In some cases where the use of cpu_up/down seemed legitimate, I encapsulated

103 more lines

[PATCH v2 06/14] powerpc: Replace cpu_up/down with device_online/offline

Message details and related content
The core device API performs extra housekeeping bits that are missing
from directly calling cpu_up/down.

See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
serialization during LPM") for an example description of what might go
wrong.

This also prepares to make cpu_up/down a private interface for anything
but the cpu subsystem.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Qais Yousef <redacted>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <redacted>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Enrico Weigelt <info@metux.net>
CC: Ram Pai <redacted>
CC: Nicholas Piggin <npiggin@gmail.com>
CC: Thiago Jung Bauermann <redacted>
CC: Christophe Leroy <redacted>
CC: Thomas Gleixner <redacted>
CC: <redacted>
CC: <redacted>
---
 arch/powerpc/kernel/machine_kexec_64.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c
index 04a7cba58eff..ebf8cc7acc4d 100644
--- a/arch/powerpc/kernel/machine_kexec_64.c
+++ b/arch/powerpc/kernel/machine_kexec_64.c
@@ -208,13 +208,15 @@ static void wake_offline_cpus(void)
 {
 	int cpu = 0;
 
+	lock_device_hotplug();
 	for_each_present_cpu(cpu) {
 		if (!cpu_online(cpu)) {
 			printk(KERN_INFO "kexec: Waking offline cpu %d.\n",
 			       cpu);
-			WARN_ON(cpu_up(cpu));
+			WARN_ON(device_online(get_cpu_device(cpu)));
 		}
 	}
+	unlock_device_hotplug();
 }
 
 static void kexec_prepare_cpus(void)
-- 
2.17.1
Message details and related content
Hi Thomas

On 11/25/19 11:27, Qais Yousef wrote:
Changes in v2:
	* Add 2 new patches that create smp_shutdown_nonboot_cpus() to be used
	  in machine_shutdown() in ia64, arm and arm64
	* Use proper kernel-doc for the newly introduced functions
	* Renamed a function
	* Removed a stale comment in a function
	* Rebased on top of 5.4-rc8

	git clone git://linux-arm.org/linux-qy.git -b cpu-hp-cleanup-v2
I want to spin v3 to address Russel's comments. If you have any feedback it'd
be great to have them before I spin v3.

Thanks

--
Qais Yousef
Using cpu_up/down directly to bring cpus online/offline loses synchronization
with sysfs and could suffer from a race similar to what is described in
commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and serialization
during LPM").

cpu_up/down seem to be more of a internal implementation detail for the cpu
subsystem to use to boot up cpus, perform suspend/resume and low level hotplug
operations. Users outside of the cpu subsystem would be better using the device
core API to bring a cpu online/offline which is the interface used to hotplug
memory and other system devices.

Several users have already migrated to use the device core API, this series
converts the remaining users and hides cpu_up/down from internal users at the
end.

I noticed this problem while working on a hack to disable offlining
a particular CPU but noticed that setting the offline_disabled attribute in the
device struct isn't enough because users can easily bypass the device core.
While my hack isn't a valid use case but it did highlight the inconsistency in
the way cpus are being onlined/offlined and this attempt hopefully improves on
this.

The first 8 patches fix arch users.

The remaining 6 patches fix generic code users. Particularly creating a new
special exported API for the device core to use instead of cpu_up/down.

The last patch removes cpu_up/down from cpu.h and unexport the functions.

In some cases where the use of cpu_up/down seemed legitimate, I encapsulated
the logic in a higher level - special purposed function; and converted the code
to use that instead.

I did re-run the rcu torture, lock torture and psci checker tests and no
problem was noticed. I did perform build tests on all arch affected except for
parisc.

Hopefully I got the CC list right for all the patches. Apologies in advance if
some people were omitted from some patches but they should have been CCed.

94 more quoted lines

Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help