ACPI, cpu hotplug: move try_offline_node() after acpi_unmap_lsapic()

PATCH In review Yasuaki Ishimatsu 5 participants 13 messages Aug 6, 2013 – Aug 13, 2013

Subsystems (2)

ACPI Driver core

lore

Share & tools

lore

Yasuaki Ishimatsu

author

2013-08-06 10:11 UTC #0 lore raw

PATCH 1/1 ACPI, cpu hotplug: move try_offline_node() after acpi_unmap_lsapic()

try_offline_node() checks that all cpus related with removed node have been removed by using cpu_present_bits. If all cpus related with removed node have been removed, try_offline_node() clears the node information.

But try_offline_node() called from acpi_processor_remove() never clears the node information. For disabling cpu_present_bits, acpi_unmap_lsapic() need be called. But acpi_unmap_lsapic() is called after try_offline_node() runs. So when try_offline_node() runs, the cpu's cpu_present_bits is always set.

This patch moves try_offline_node() after acpi_unmap_lsapic().

Signed-off-by Yasuaki Ishimatsu <[email protected]>

---
 drivers/acpi/acpi_processor.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/acpi/acpi_processor.c +2 -1

index fd6c51c..5a74a9c 100644
@@ -451,7 +451,6 @@ static void acpi_processor_remove(struct acpi_device *device)
 	/* Clean up. */
 	per_cpu(processor_device_array, pr->id) = NULL;
 	per_cpu(processors, pr->id) = NULL;
-	try_offline_node(cpu_to_node(pr->id));
 
 	/* Remove the CPU. */
 	get_online_cpus();

@@ -459,6 +458,8 @@ static void acpi_processor_remove(struct acpi_device *device)
 	acpi_unmap_lsapic(pr->id);
 	put_online_cpus();
 
+	try_offline_node(cpu_to_node(pr->id));
+
  out:
 	free_cpumask_var(pr->throttling.shared_cpu_map);
 	kfree(pr);

Toshi Kani

Acked-by on v1 1/1

2013-08-06 16:57 UTC #1 lore raw

Quoted 10 lines · Yasuaki Ishimatsu

try_offline_node() checks that all cpus related with removed node have been removed by using cpu_present_bits. If all cpus related with removed node have been removed, try_offline_node() clears the node information.
But try_offline_node() called from acpi_processor_remove() never clears the node information. For disabling cpu_present_bits, acpi_unmap_lsapic() need be called. But acpi_unmap_lsapic() is called after try_offline_node() runs. So when try_offline_node() runs, the cpu's cpu_present_bits is always set.

Quoted 3 lines

This patch moves try_offline_node() after acpi_unmap_lsapic().
Signed-off-by Yasuaki Ishimatsu <[email protected]>

The change looks good to me.

Acked-by Toshi Kani <[email protected]>

BTW, do you know why try_offline_node() has to use stop_machine()?

Thanks,
-Toshi

Yasuaki Ishimatsu

author

2013-08-07 03:24 UTC #2 lore raw

Quoted 11 lines · Toshi Kani

Quoted 10 lines · Yasuaki Ishimatsu
try_offline_node() checks that all cpus related with removed node have been removed by using cpu_present_bits. If all cpus related with removed node have been removed, try_offline_node() clears the node information.
But try_offline_node() called from acpi_processor_remove() never clears the node information. For disabling cpu_present_bits, acpi_unmap_lsapic() need be called. But acpi_unmap_lsapic() is called after try_offline_node() runs. So when try_offline_node() runs, the cpu's cpu_present_bits is always set.

Quoted 7 lines

Quoted 3 lines
This patch moves try_offline_node() after acpi_unmap_lsapic().
Signed-off-by Yasuaki Ishimatsu <[email protected]>
The change looks good to me.
Acked-by Toshi Kani <[email protected]>

Thanks, Toshi!

Quoted 1 line

BTW, do you know why try_offline_node() has to use stop_machine()?

I don't know the reason.

Thanks,
Yasuaki Ishimatsu

Quoted 4 lines

Thanks,
-Toshi

Tang Chen

2013-08-09 07:30 UTC #3 lore raw

In reply to Toshi Kani

Quoted 13 lines · Toshi Kani

Quoted 12 lines · Yasuaki Ishimatsu
try_offline_node() checks that all cpus related with removed node have been removed by using cpu_present_bits. If all cpus related with removed node have been removed, try_offline_node() clears the node information.
But try_offline_node() called from acpi_processor_remove() never clears the node information. For disabling cpu_present_bits, acpi_unmap_lsapic() need be called. But acpi_unmap_lsapic() is called after try_offline_node() runs. So when try_offline_node() runs, the cpu's cpu_present_bits is always set.
This patch moves try_offline_node() after acpi_unmap_lsapic().

Quoted 7 lines

Quoted 1 line
Signed-off-by Yasuaki Ishimatsu<[email protected]>
The change looks good to me.
Acked-by Toshi Kani<[email protected]>
BTW, do you know why try_offline_node() has to use stop_machine()?

try_offline_node() is used to check if the node could be hot-removed after each memory or cpu hot-remove operation.

In memory hot-remove path, we have lock_memory_hotplug() to series all the memory hot-remove options.

But when doing cpu hot-remove,

acpi_processor_remove()
|->try_offline_node()

There is no lock to protect it. I think, when we are going to hot-remove a node, others should not do any memory or cpu hotplug operation. In memory hotplug path, we have lock_memory_hotplug(). But in cpu hotplug path, I didn't find any lock. So we used stop_machine() to call check_cpu_on_node(). If we find any cpu still present, we return and do not remove the node.

Thanks.

Toshi Kani

2013-08-09 22:16 UTC #4 lore raw

Quoted 30 lines · Tang Chen

Quoted 13 lines · Toshi Kani
Quoted 12 lines · Yasuaki Ishimatsu
try_offline_node() checks that all cpus related with removed node have been removed by using cpu_present_bits. If all cpus related with removed node have been removed, try_offline_node() clears the node information.
But try_offline_node() called from acpi_processor_remove() never clears the node information. For disabling cpu_present_bits, acpi_unmap_lsapic() need be called. But acpi_unmap_lsapic() is called after try_offline_node() runs. So when try_offline_node() runs, the cpu's cpu_present_bits is always set.
This patch moves try_offline_node() after acpi_unmap_lsapic().
Quoted 7 lines
Quoted 1 line
Signed-off-by Yasuaki Ishimatsu<[email protected]>
The change looks good to me.
Acked-by Toshi Kani<[email protected]>
BTW, do you know why try_offline_node() has to use stop_machine()?
try_offline_node() is used to check if the node could be hot-removed after each memory or cpu hot-remove operation.
In memory hot-remove path, we have lock_memory_hotplug() to series all the memory hot-remove options.
But when doing cpu hot-remove,

Quoted 8 lines

acpi_processor_remove()
|->try_offline_node()
There is no lock to protect it. I think, when we are going to hot-remove a node, others should not do any memory or cpu hotplug operation. In memory hotplug path, we have lock_memory_hotplug(). But in cpu hotplug path, I didn't find any lock. So we used stop_machine() to call check_cpu_on_node(). If we find any cpu still present, we return and do not remove the node.

CPU/Memory hotplug operations and sysfs eject are serialized with acpi_os_hotplug_execute(). CPU online/offline is protected by cpu_hotplug_[begin|done]() and [get|put]_online_cpus(). But, yes, online/offline and hotplug operations are not serialized. I tried to serialize them before, but that framework was not well received.

Anyway, it looks to me that cpu_up()->mem_online_node() path can race with try_offline_node(). And I do not think stop_machine() protects this case, either. try_offline_node() stops the execution of num_online_node() during stop_machine(), but then lets it continues to run after that. I think they need to be protected by a lock.

I will look further to see if I can come up with some solution.

Thanks,
-Toshi

Rafael J. Wysocki

2013-08-09 23:18 UTC #5 lore raw

Quoted 40 lines · Toshi Kani

Quoted 30 lines · Tang Chen
Quoted 13 lines · Toshi Kani
Quoted 12 lines · Yasuaki Ishimatsu
try_offline_node() checks that all cpus related with removed node have been removed by using cpu_present_bits. If all cpus related with removed node have been removed, try_offline_node() clears the node information.
But try_offline_node() called from acpi_processor_remove() never clears the node information. For disabling cpu_present_bits, acpi_unmap_lsapic() need be called. But acpi_unmap_lsapic() is called after try_offline_node() runs. So when try_offline_node() runs, the cpu's cpu_present_bits is always set.
This patch moves try_offline_node() after acpi_unmap_lsapic().
Quoted 7 lines
Quoted 1 line
Signed-off-by Yasuaki Ishimatsu<[email protected]>
The change looks good to me.
Acked-by Toshi Kani<[email protected]>
BTW, do you know why try_offline_node() has to use stop_machine()?
try_offline_node() is used to check if the node could be hot-removed after each memory or cpu hot-remove operation.
In memory hot-remove path, we have lock_memory_hotplug() to series all the memory hot-remove options.
But when doing cpu hot-remove,
Quoted 8 lines
acpi_processor_remove()
|->try_offline_node()
There is no lock to protect it. I think, when we are going to hot-remove a node, others should not do any memory or cpu hotplug operation. In memory hotplug path, we have lock_memory_hotplug(). But in cpu hotplug path, I didn't find any lock. So we used stop_machine() to call check_cpu_on_node(). If we find any cpu still present, we return and do not remove the node.

Quoted 5 lines

CPU/Memory hotplug operations and sysfs eject are serialized with acpi_os_hotplug_execute(). CPU online/offline is protected by cpu_hotplug_[begin|done]() and [get|put]_online_cpus(). But, yes, online/offline and hotplug operations are not serialized. I tried to serialize them before, but that framework was not well received.

What about lock_device_hotplug()? It is taken by both online/offline and the ACPI hotplug code, isn't it?

Quoted 2 lines

Anyway, it looks to me that cpu_up()->mem_online_node() path can race with try_offline_node().

It can in principle, but I'm not sure if there's a way to trigger that race. Do you have an example?

Rafael

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Toshi Kani

2013-08-10 02:12 UTC #6 lore raw

Quoted 41 lines · Rafael J. Wysocki

Quoted 40 lines · Toshi Kani
Quoted 38 lines · Tang Chen
Quoted 13 lines · Toshi Kani
Quoted 12 lines · Yasuaki Ishimatsu
try_offline_node() checks that all cpus related with removed node have been removed by using cpu_present_bits. If all cpus related with removed node have been removed, try_offline_node() clears the node information.
But try_offline_node() called from acpi_processor_remove() never clears the node information. For disabling cpu_present_bits, acpi_unmap_lsapic() need be called. But acpi_unmap_lsapic() is called after try_offline_node() runs. So when try_offline_node() runs, the cpu's cpu_present_bits is always set.
This patch moves try_offline_node() after acpi_unmap_lsapic().
Quoted 7 lines
Quoted 1 line
Signed-off-by Yasuaki Ishimatsu<[email protected]>
The change looks good to me.
Acked-by Toshi Kani<[email protected]>
BTW, do you know why try_offline_node() has to use stop_machine()?
try_offline_node() is used to check if the node could be hot-removed after each memory or cpu hot-remove operation.
In memory hot-remove path, we have lock_memory_hotplug() to series all the memory hot-remove options.
But when doing cpu hot-remove,
acpi_processor_remove()
|->try_offline_node()
There is no lock to protect it. I think, when we are going to hot-remove a node, others should not do any memory or cpu hotplug operation. In memory hotplug path, we have lock_memory_hotplug(). But in cpu hotplug path, I didn't find any lock. So we used stop_machine() to call check_cpu_on_node(). If we find any cpu still present, we return and do not remove the node.

Quoted 8 lines

Quoted 5 lines
CPU/Memory hotplug operations and sysfs eject are serialized with acpi_os_hotplug_execute(). CPU online/offline is protected by cpu_hotplug_[begin|done]() and [get|put]_online_cpus(). But, yes, online/offline and hotplug operations are not serialized. I tried to serialize them before, but that framework was not well received.
What about lock_device_hotplug()? It is taken by both online/offline and the ACPI hotplug code, isn't it?

Oh, that's right! I forgot about this one. Yes, lock_device_hotplug() nicely protects online/offline and hotplug operations. :-)

Quoted 5 lines

Quoted 2 lines
Anyway, it looks to me that cpu_up()->mem_online_node() path can race with try_offline_node().
It can in principle, but I'm not sure if there's a way to trigger that race. Do you have an example?

With lock_device_hotplug(), I agree that we do not have this race condition -- cpu_up() may not run while other hotplug is running. store_online() will be blocked at lock_device_hotplug() in such case. When store_online() acquired the lock, this CPU may have been deleted. So, we still need to make sure that this case is handled properly. I suppose sysfs keeps *dev valid with ref_count (Is that right?). I think cpu_up() needs to check with cpu_present(), not cpu_possible(), at the top. Otherwise, cpu_to_node(cpu) may return NUMA_NO_NODE (-1), which is probably not a good value for node_online(nid).

Thanks,
-Toshi

Rafael J. Wysocki

2013-08-11 20:31 UTC #7 lore raw

Quoted 57 lines · Toshi Kani

Quoted 41 lines · Rafael J. Wysocki
Quoted 40 lines · Toshi Kani
Quoted 38 lines · Tang Chen
Quoted 13 lines · Toshi Kani
Quoted 12 lines · Yasuaki Ishimatsu
try_offline_node() checks that all cpus related with removed node have been removed by using cpu_present_bits. If all cpus related with removed node have been removed, try_offline_node() clears the node information.
But try_offline_node() called from acpi_processor_remove() never clears the node information. For disabling cpu_present_bits, acpi_unmap_lsapic() need be called. But acpi_unmap_lsapic() is called after try_offline_node() runs. So when try_offline_node() runs, the cpu's cpu_present_bits is always set.
This patch moves try_offline_node() after acpi_unmap_lsapic().
Quoted 7 lines
Quoted 1 line
Signed-off-by Yasuaki Ishimatsu<[email protected]>
The change looks good to me.
Acked-by Toshi Kani<[email protected]>
BTW, do you know why try_offline_node() has to use stop_machine()?
try_offline_node() is used to check if the node could be hot-removed after each memory or cpu hot-remove operation.
In memory hot-remove path, we have lock_memory_hotplug() to series all the memory hot-remove options.
But when doing cpu hot-remove,
acpi_processor_remove()
|->try_offline_node()
There is no lock to protect it. I think, when we are going to hot-remove a node, others should not do any memory or cpu hotplug operation. In memory hotplug path, we have lock_memory_hotplug(). But in cpu hotplug path, I didn't find any lock. So we used stop_machine() to call check_cpu_on_node(). If we find any cpu still present, we return and do not remove the node.
Quoted 8 lines
Quoted 5 lines
CPU/Memory hotplug operations and sysfs eject are serialized with acpi_os_hotplug_execute(). CPU online/offline is protected by cpu_hotplug_[begin|done]() and [get|put]_online_cpus(). But, yes, online/offline and hotplug operations are not serialized. I tried to serialize them before, but that framework was not well received.
What about lock_device_hotplug()? It is taken by both online/offline and the ACPI hotplug code, isn't it?
Oh, that's right! I forgot about this one. Yes, lock_device_hotplug() nicely protects online/offline and hotplug operations. :-)
Quoted 3 lines
Quoted 2 lines
Anyway, it looks to me that cpu_up()->mem_online_node() path can race with try_offline_node().

Quoted 8 lines

Quoted 2 lines
It can in principle, but I'm not sure if there's a way to trigger that race. Do you have an example?
With lock_device_hotplug(), I agree that we do not have this race condition -- cpu_up() may not run while other hotplug is running. store_online() will be blocked at lock_device_hotplug() in such case. When store_online() acquired the lock, this CPU may have been deleted. So, we still need to make sure that this case is handled properly.

Yes.

Quoted 1 line

I suppose sysfs keeps *dev valid with ref_count (Is that right?).

Yes, it does.

Quoted 3 lines

I think cpu_up() needs to check with cpu_present(), not cpu_possible(), at the top. Otherwise, cpu_to_node(cpu) may return NUMA_NO_NODE (-1), which is probably not a good value for node_online(nid).

We do cpu_to_node(cpuid) in cpu_subsys_online() before that, so maybe it's better to check the result already there and bail out if that's negative?

Something like the patch below.

Thanks,
Rafael

---
 drivers/base/cpu.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/base/cpu.c +5 -1

Index: linux-pm/drivers/base/cpu.c
===================================================================
@@ -43,11 +43,14 @@ static int __ref cpu_subsys_online(struc
 	struct cpu *cpu = container_of(dev, struct cpu, dev);
 	int cpuid = dev->id;
 	int from_nid, to_nid;
-	int ret;
+	int ret = -ENODEV;
 
 	cpu_hotplug_driver_lock();
 
 	from_nid = cpu_to_node(cpuid);
+	if (from_nid == NUMA_NO_NODE)
+		goto out;
+
 	ret = cpu_up(cpuid);
 	/*
 	 * When hot adding memory to memoryless node and enabling a cpu

@@ -57,6 +60,7 @@ static int __ref cpu_subsys_online(struc
 	if (from_nid != to_nid)
 		change_cpu_under_node(cpu, from_nid, to_nid);
 
+ out:
 	cpu_hotplug_driver_unlock();
 	return ret;
 }

Toshi Kani

2013-08-12 20:09 UTC #8 lore raw

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines · Toshi Kani
Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Toshi Kani
Quoted 1 line · Tang Chen
On 08/07/2013 12:56 AM, Toshi Kani wrote:

Quoted 53 lines

Quoted 34 lines
Quoted 19 lines
Quoted 19 lines
Quoted 18 lines
Quoted 1 line
BTW, do you know why try_offline_node() has to use stop_machine()?
try_offline_node() is used to check if the node could be hot-removed after each memory or cpu hot-remove operation.
In memory hot-remove path, we have lock_memory_hotplug() to series all the memory hot-remove options.
But when doing cpu hot-remove,
acpi_processor_remove()
|->try_offline_node()
There is no lock to protect it. I think, when we are going to hot-remove a node, others should not do any memory or cpu hotplug operation. In memory hotplug path, we have lock_memory_hotplug(). But in cpu hotplug path, I didn't find any lock. So we used stop_machine() to call check_cpu_on_node(). If we find any cpu still present, we return and do not remove the node.
Quoted 8 lines
Quoted 5 lines
CPU/Memory hotplug operations and sysfs eject are serialized with acpi_os_hotplug_execute(). CPU online/offline is protected by cpu_hotplug_[begin|done]() and [get|put]_online_cpus(). But, yes, online/offline and hotplug operations are not serialized. I tried to serialize them before, but that framework was not well received.
What about lock_device_hotplug()? It is taken by both online/offline and the ACPI hotplug code, isn't it?
Oh, that's right! I forgot about this one. Yes, lock_device_hotplug() nicely protects online/offline and hotplug operations. :-)
Quoted 3 lines
Quoted 2 lines
Anyway, it looks to me that cpu_up()->mem_online_node() path can race with try_offline_node().
Quoted 8 lines
Quoted 2 lines
It can in principle, but I'm not sure if there's a way to trigger that race. Do you have an example?
With lock_device_hotplug(), I agree that we do not have this race condition -- cpu_up() may not run while other hotplug is running. store_online() will be blocked at lock_device_hotplug() in such case. When store_online() acquired the lock, this CPU may have been deleted. So, we still need to make sure that this case is handled properly.
Yes.
Quoted 1 line
I suppose sysfs keeps *dev valid with ref_count (Is that right?).
Yes, it does.
Quoted 3 lines
I think cpu_up() needs to check with cpu_present(), not cpu_possible(), at the top. Otherwise, cpu_to_node(cpu) may return NUMA_NO_NODE (-1), which is probably not a good value for node_online(nid).

Quoted 5 lines

We do cpu_to_node(cpuid) in cpu_subsys_online() before that, so maybe it's better to check the result already there and bail out if that's negative?
Something like the patch below.

That looks good to me.

Thanks,
-Toshi

Rafael J. Wysocki

2013-08-13 00:29 UTC #9 lore raw

PATCH 1/1 driver core / cpu: Check if NUMA node is valid before bringing CPU up

From: Rafael J. Wysocki <[email protected]>

There is a potential race condition between cpu_subsys_online() and either acpi_processor_remove() or remove_memory() that execute try_offline_node(). Namely, it is possible that cpu_subsys_online() will run right after the CPUs NUMA node has been put offline and cpu_to_node() executed by it will return NUMA_NO_NODE (-1). In that case the CPU is gone and it doesn't make sense to call cpu_up() for it, so make cpu_subsys_online() return -ENODEV then.

Signed-off-by Rafael J. Wysocki <[email protected]>

Acked-by Toshi Kani <[email protected]>

---

Hi Greg,

Can you please kindly queue up this one for 3.12?

I've added the Toshi's ACK as he said it looked good to him. :-)

Thanks,
Rafael

---
 drivers/base/cpu.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/base/cpu.c +5 -1

Index: linux-pm/drivers/base/cpu.c
===================================================================
@@ -43,11 +43,14 @@ static int __ref cpu_subsys_online(struc
 	struct cpu *cpu = container_of(dev, struct cpu, dev);
 	int cpuid = dev->id;
 	int from_nid, to_nid;
-	int ret;
+	int ret = -ENODEV;
 
 	cpu_hotplug_driver_lock();
 
 	from_nid = cpu_to_node(cpuid);
+	if (from_nid == NUMA_NO_NODE)
+		goto out;
+
 	ret = cpu_up(cpuid);
 	/*
 	 * When hot adding memory to memoryless node and enabling a cpu

@@ -57,6 +60,7 @@ static int __ref cpu_subsys_online(struc
 	if (from_nid != to_nid)
 		change_cpu_under_node(cpu, from_nid, to_nid);
 
+ out:
 	cpu_hotplug_driver_unlock();
 	return ret;
 }

Toshi Kani

2013-08-13 00:39 UTC #10 lore raw

Re: [PATCH] driver core / cpu: Check if NUMA node is valid before bringing CPU up

Quoted 12 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
There is a potential race condition between cpu_subsys_online() and either acpi_processor_remove() or remove_memory() that execute try_offline_node(). Namely, it is possible that cpu_subsys_online() will run right after the CPUs NUMA node has been put offline and cpu_to_node() executed by it will return NUMA_NO_NODE (-1). In that case the CPU is gone and it doesn't make sense to call cpu_up() for it, so make cpu_subsys_online() return -ENODEV then.
Signed-off-by Rafael J. Wysocki <[email protected]>
Acked-by Toshi Kani <[email protected]>

Quoted 7 lines

---
Hi Greg,
Can you please kindly queue up this one for 3.12?
I've added the Toshi's ACK as he said it looked good to him. :-)

That's fine by me.
-Toshi

Gu Zheng

Tested-by on v1 1/1

2013-08-09 02:47 UTC #11 lore raw

In reply to Yasuaki Ishimatsu

Hi Ishimatsu-san,

Quoted 11 lines · Yasuaki Ishimatsu

try_offline_node() checks that all cpus related with removed node have been removed by using cpu_present_bits. If all cpus related with removed node have been removed, try_offline_node() clears the node information.
But try_offline_node() called from acpi_processor_remove() never clears the node information. For disabling cpu_present_bits, acpi_unmap_lsapic() need be called. But acpi_unmap_lsapic() is called after try_offline_node() runs. So when try_offline_node() runs, the cpu's cpu_present_bits is always set.
This patch moves try_offline_node() after acpi_unmap_lsapic().

It works well.

Quoted 2 lines

Signed-off-by Yasuaki Ishimatsu <[email protected]>

Tested-by Gu Zheng <[email protected]>

Thanks,
Gu

Quoted 32 lines

---
 drivers/acpi/acpi_processor.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/acpi/acpi_processor.c +2 -2

index fd6c51c..5a74a9c 100644
@@ -451,7 +451,6 @@ static void acpi_processor_remove(struct acpi_device *device)
 	/* Clean up. */
 	per_cpu(processor_device_array, pr->id) = NULL;
 	per_cpu(processors, pr->id) = NULL;
-	try_offline_node(cpu_to_node(pr->id));
 
 	/* Remove the CPU. */
 	get_online_cpus();

@@ -459,6 +458,8 @@ static void acpi_processor_remove(struct acpi_device *device)
 	acpi_unmap_lsapic(pr->id);
 	put_online_cpus();
 
+	try_offline_node(cpu_to_node(pr->id));
+
  out:
 	free_cpumask_var(pr->throttling.shared_cpu_map);
 	kfree(pr);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Yasuaki Ishimatsu

author

2013-08-09 05:54 UTC #12 lore raw

Quoted 15 lines · Gu Zheng

Hi Ishimatsu-san,
Quoted 11 lines · Yasuaki Ishimatsu
try_offline_node() checks that all cpus related with removed node have been removed by using cpu_present_bits. If all cpus related with removed node have been removed, try_offline_node() clears the node information.
But try_offline_node() called from acpi_processor_remove() never clears the node information. For disabling cpu_present_bits, acpi_unmap_lsapic() need be called. But acpi_unmap_lsapic() is called after try_offline_node() runs. So when try_offline_node() runs, the cpu's cpu_present_bits is always set.
This patch moves try_offline_node() after acpi_unmap_lsapic().

Quoted 7 lines

It works well.
Quoted 2 lines
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Tested-by Gu Zheng <[email protected]>

Thank you for testing.

Thanks,
Yasuaki Ishimatsu

Quoted 38 lines

Thanks,
Gu

Quoted 32 lines

---
  drivers/acpi/acpi_processor.c |    3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)

drivers/acpi/acpi_processor.c +2 -2

index fd6c51c..5a74a9c 100644
@@ -451,7 +451,6 @@ static void acpi_processor_remove(struct acpi_device *device)
  	/* Clean up. */
  	per_cpu(processor_device_array, pr->id) = NULL;
  	per_cpu(processors, pr->id) = NULL;
-	try_offline_node(cpu_to_node(pr->id));
 
  	/* Remove the CPU. */
  	get_online_cpus();

@@ -459,6 +458,8 @@ static void acpi_processor_remove(struct acpi_device *device)
  	acpi_unmap_lsapic(pr->id);
  	put_online_cpus();
 
+	try_offline_node(cpu_to_node(pr->id));
+
   out:
  	free_cpumask_var(pr->throttling.shared_cpu_map);
  	kfree(pr);
 
--