Re: [PATCH 1/1] hotplug-cpu.c: show 'last online CPU' error in dlpar_cpu_remove()
From: Daniel Henrique Barboza <hidden>
Date: 2021-03-22 23:31:13
On 3/19/21 8:26 AM, Michael Ellerman wrote:
Daniel Henrique Barboza [off-list ref] writes:quoted
Ping On 3/5/21 2:38 PM, Daniel Henrique Barboza wrote:quoted
Of all the reasons that dlpar_cpu_remove() can fail, the 'last online CPU' is one that can be caused directly by the user offlining CPUs in a partition/virtual machine that has hotplugged CPUs. Trying to reclaim a hotplugged CPU can fail if the CPU is now the last online in the system. This is easily reproduced using QEMU [1].Sorry, I saw this earlier and never got around to replying.
No problem. Thanks for the review!
I'm wondering if we neet to catch it earlier, ie. in dlpar_offline_cpu(). By the time we return to dlpar_cpu_remove() we've dropped the cpu_add_remove_lock (cpu_maps_update_done), so num_online_cpus() could change out from under us, meaning the num_online_cpus() == 1 check might trigger incorrectly (or vice versa). Something like the patch below (completely untested :D)
Makes sense. I'll try it out to see if it works.
And writing that patch makes me wonder, is == 1 the right check? In most cases we'll remove all but one thread of the core, but we'll fail on the last thread. Leaving that core effectively stuck in SMT1. Is that useful behaviour? Should we instead check at the start that we can remove all threads of the core without going to zero online CPUs?
I think it's ok to allow SMT1 cores, speaking from QEMU perspective. QEMU does not have a "core hotunplug" operation where the whole core is hotunplugged at once. The CPU hotplug/unplug operations are done as single CPU thread add/removal. If the user wants to run 4 cores, all of them with SMT1, QEMU will allow it. Libvirt does not operate with the core granularity either - you can specify the amount of vcpus the guest should run with, and Libvirt will send hotplug/unplug requests to QEMU to match the desired value. It doesn't bother with how many threads of a core were offlined or not. Thanks, DHB
quoted hunk ↗ jump to hunk
cheersdiff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c index 12cbffd3c2e3..498c22331ac8 100644 --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c@@ -271,6 +271,12 @@ static int dlpar_offline_cpu(struct device_node *dn) if (!cpu_online(cpu)) break; + if (num_online_cpus() == 1) { + pr_warn("Unable to remove last online CPU %pOFn\n", dn); + rc = EBUSY; + goto out_unlock; + } + cpu_maps_update_done(); rc = device_offline(get_cpu_device(cpu)); if (rc)@@ -283,6 +289,7 @@ static int dlpar_offline_cpu(struct device_node *dn) thread); } } +out_unlock: cpu_maps_update_done(); out: