acpi: Fix CPU hot removal problem

We run linux as a guest in Xen environment. When we used the xen tools (xm vcpu-set <n>) to hot add and remove vcpu to and from the guest, we encountered the failure on vcpu removal. We found the reason is that it didn't go to really remove cpu in the cpu removal code path.

This patch adds acpi_bus_trim in acpi_process_hotplug_notify to fix this issue. With this patch, it works fine for us.

Signed-off-by Canquan Shen <[email protected]>

---
 drivers/acpi/processor_driver.c |    6 ++++++

 1 files changed, 6 insertions(+), 0 deletions(-)

drivers/acpi/processor_driver.c +1 -0

index a4e0f1b..03d92d6 100644
@@ -641,6 +641,7 @@ static void acpi_processor_hotplug_notify(acpi_handle
 handle,
        struct acpi_processor *pr;
        struct acpi_device *device = NULL;
        int result;
+       u32 id;
 
 

       switch (event) {

drivers/acpi/processor_driver.c +5 -0

@@ -677,6 +678,11 @@ static void acpi_processor_hotplug_notify(acpi_handle
 handle,
                                    "Driver data is NULL, dropping EJECT\n");
                        return;
                }
+               id = pr->id;
+               if (acpi_bus_trim(device, 1)) {
+                       printk(KERN_ERR  PREFIX
+                                   "Fail to Remove CPU %d\n", id);
+               }
                break;

       default:
               ACPI_DEBUG_PRINT((ACPI_DB_INFO,
--
1.7.6.0

Quoted 5 lines · canquan.shen

We run linux as a guest in Xen environment. When we used the xen tools (xm vcpu-set <n>) to hot add and remove vcpu to and from the guest, we encountered the failure on vcpu removal. We found the reason is that it didn't go to really remove cpu in the cpu removal code path.

Quoted 4 lines

This patch adds acpi_bus_trim in acpi_process_hotplug_notify to fix this issue. With this patch, it works fine for us.

Signed-off-by Canquan Shen <[email protected]>

Reviewed-by Bjorn Helgaas <[email protected]>

Quoted 27 lines

---
 drivers/acpi/processor_driver.c |    6 ++++++

 1 files changed, 6 insertions(+), 0 deletions(-)

drivers/acpi/processor_driver.c +1 -0

index a4e0f1b..03d92d6 100644
@@ -641,6 +641,7 @@ static void acpi_processor_hotplug_notify(acpi_handle
 handle,
        struct acpi_processor *pr;
        struct acpi_device *device = NULL;
        int result;
+       u32 id;
 
 

       switch (event) {

drivers/acpi/processor_driver.c +4 -0

@@ -677,6 +678,11 @@ static void acpi_processor_hotplug_notify(acpi_handle
 handle,
                                    "Driver data is NULL, dropping EJECT\n");
                        return;
                }
+               id = pr->id;
+               if (acpi_bus_trim(device, 1)) {
+                       printk(KERN_ERR  PREFIX
+                                   "Fail to Remove CPU %d\n", id);

Quoted 8 lines

drivers/acpi/processor_driver.c +1 -0

+               }
                break;

       default:
               ACPI_DEBUG_PRINT((ACPI_DB_INFO,
--
1.7.6.0

Quoted 6 lines · Bjorn Helgaas

Quoted 5 lines · canquan.shen

We run linux as a guest in Xen environment. When we used the xen tools (xm vcpu-set <n>) to hot add and remove vcpu to and from the guest, we encountered the failure on vcpu removal. We found the reason is that it didn't go to really remove cpu in the cpu removal code path.

Quoted 6 lines

Quoted 4 lines

This patch adds acpi_bus_trim in acpi_process_hotplug_notify to fix this issue. With this patch, it works fine for us.

Signed-off-by Canquan Shen <[email protected]>

Reviewed-by Bjorn Helgaas <[email protected]>

On second thought, let's think about this a bit more.

As I mentioned before, I have a long-term goal to move the hotplug flow out of drivers and into the ACPI core. That will be easier if the code in the drivers is as generic as possible.

The dock and acpiphp hot-remove code calls acpi_bus_trim(), then evaluates _EJ0. The core acpi_bus_hot_remove_device() function already does both acpi_bus_trim() and _EJ0. This function is currently only used when we write to sysfs "eject" files, but I wonder if we should use it in acpi_processor_hotplug_notify() as well.

That would get us one step closer to removing this gunk from the drivers and having acpi_bus_notify() look something like this:

    case ACPI_NOTIFY_EJECT_REQUEST:
        driver->ops.remove(device);
        acpi_bus_hot_remove_device(device);
        break;

There is a description of a CPU hot-remove that does include _EJ0 methods in the "DIG64 Hot-Plug & Partitioning Flows Specification" [1], sec 2.2.4. I know this document is Itanium-oriented, but this part seems fairly generic and it's the only description of the process I've seen so far.

So would using acpi_bus_hot_remove_device() instead of acpi_bus_trim() also solve your problem, Canquan?

Bjorn

[1] http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf

Quoted 36 lines

Quoted 35 lines

---
 drivers/acpi/processor_driver.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

drivers/acpi/processor_driver.c +1 -0

index a4e0f1b..03d92d6 100644
@@ -641,6 +641,7 @@ static void acpi_processor_hotplug_notify(acpi_handle
 handle,
       struct acpi_processor *pr;
       struct acpi_device *device = NULL;
       int result;
+       u32 id;
 
 

       switch (event) {

drivers/acpi/processor_driver.c +5 -0

@@ -677,6 +678,11 @@ static void acpi_processor_hotplug_notify(acpi_handle
 handle,
                                   "Driver data is NULL, dropping EJECT\n");
                       return;
               }
+               id = pr->id;
+               if (acpi_bus_trim(device, 1)) {
+                       printk(KERN_ERR  PREFIX
+                                   "Fail to Remove CPU %d\n", id);
+               }
               break;
       default:

ACPI_DEBUG_PRINT((ACPI_DB_INFO,
--
1.7.6.0

Quoted 6 lines · Bjorn Helgaas

Quoted 5 lines · canquan.shen

We run linux as a guest in Xen environment. When we used the xen tools (xm vcpu-set<n>) to hot add and remove vcpu to and from the guest, we encountered the failure on vcpu removal. We found the reason is that it didn't go to really remove cpu in the cpu removal code path.

Quoted 6 lines

Quoted 4 lines

This patch adds acpi_bus_trim in acpi_process_hotplug_notify to fix this issue. With this patch, it works fine for us.

Signed-off-by Canquan Shen<[email protected]>

Reviewed-by Bjorn Helgaas<[email protected]>

On second thought, let's think about this a bit more.

As I mentioned before, I have a long-term goal to move the hotplug flow out of drivers and into the ACPI core. That will be easier if the code in the drivers is as generic as possible.

The dock and acpiphp hot-remove code calls acpi_bus_trim(), then evaluates _EJ0. The core acpi_bus_hot_remove_device() function already does both acpi_bus_trim() and _EJ0. This function is currently only used when we write to sysfs "eject" files, but I wonder if we should use it in acpi_processor_hotplug_notify() as well.

That would get us one step closer to removing this gunk from the drivers and having acpi_bus_notify() look something like this:

     case ACPI_NOTIFY_EJECT_REQUEST:
         driver->ops.remove(device);
         acpi_bus_hot_remove_device(device);
         break;

There is a description of a CPU hot-remove that does include _EJ0 methods in the "DIG64 Hot-Plug& Partitioning Flows Specification" [1], sec 2.2.4. I know this document is Itanium-oriented, but this part seems fairly generic and it's the only description of the process I've seen so far.

So would using acpi_bus_hot_remove_device() instead of acpi_bus_trim() also solve your problem, Canquan?

Bjorn

[1] http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf

Quoted 27 lines

Quoted 27 lines

---
  drivers/acpi/processor_driver.c |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

drivers/acpi/processor_driver.c +1 -0

index a4e0f1b..03d92d6 100644
@@ -641,6 +641,7 @@ static void acpi_processor_hotplug_notify(acpi_handle
 handle,
        struct acpi_processor *pr;
        struct acpi_device *device = NULL;
        int result;
+       u32 id;
 
 

        switch (event) {

drivers/acpi/processor_driver.c +4 -0

@@ -677,6 +678,11 @@ static void acpi_processor_hotplug_notify(acpi_handle
 handle,
                                    "Driver data is NULL, dropping EJECT\n");
                        return;
                }
+               id = pr->id;
+               if (acpi_bus_trim(device, 1)) {
+                       printk(KERN_ERR  PREFIX
+                                   "Fail to Remove CPU %d\n", id);

Quoted 9 lines

Quoted 8 lines

drivers/acpi/processor_driver.c +1 -0

+               }
                break;
        default:

ACPI_DEBUG_PRINT((ACPI_DB_INFO,
--
1.7.6.0

.

Quoted 39 lines · Bjorn Helgaas

Quoted 9 lines · Bjorn Helgaas

Quoted 6 lines · canquan.shen

We run linux as a guest in Xen environment. When we used the xen tools (xm vcpu-set<n>) to hot add and remove vcpu to and from the guest, we encountered the failure on vcpu removal. We found the reason is that it didn't go to really remove cpu in the cpu removal code path.

Quoted 6 lines

Quoted 4 lines

This patch adds acpi_bus_trim in acpi_process_hotplug_notify to fix this issue. With this patch, it works fine for us.

Signed-off-by Canquan Shen<[email protected]>

Reviewed-by Bjorn Helgaas<[email protected]>

On second thought, let's think about this a bit more.

As I mentioned before, I have a long-term goal to move the hotplug flow out of drivers and into the ACPI core.  That will be easier if the code in the drivers is as generic as possible.

The dock and acpiphp hot-remove code calls acpi_bus_trim(), then evaluates _EJ0.  The core acpi_bus_hot_remove_device() function already does both acpi_bus_trim() and _EJ0.  This function is currently only used when we write to sysfs "eject" files, but I wonder if we should use it in acpi_processor_hotplug_notify() as well.

That would get us one step closer to removing this gunk from the drivers and having acpi_bus_notify() look something like this:

    case ACPI_NOTIFY_EJECT_REQUEST:
        driver->ops.remove(device);
        acpi_bus_hot_remove_device(device);
        break;

Quoted 9 lines

There is a description of a CPU hot-remove that does include _EJ0 methods in the "DIG64 Hot-Plug&  Partitioning Flows Specification" [1], sec 2.2.4.  I know this document is Itanium-oriented, but this part seems fairly generic and it's the only description of the process I've seen so far.

So would using acpi_bus_hot_remove_device() instead of acpi_bus_trim() also solve your problem, Canquan?

Yes. It can solve my problem.
I fully aggree to replace acpi_bus_hot_remove_device() to acpi_bus_trim(). Initially I insert the acpi_bus_hot_remove_device() in acpi_bus_notify function . lately I think I should give a chance for user,and so send KOBJ_OFFLINE message to the udvev module.

But why add the  driver->ops.remove(device) before acpi_bus_hot_remove_device(device).  it can be called in acpi_bus_hot_remove_device code path as bellowing:
   acpi_bus_trim
       acpi_bus_remove
           device_release_driver __device_release_driver acpi_device_remove acpi_drv->ops.remove

Quoted 58 lines · canquan.shen

Quoted 39 lines · Bjorn Helgaas

Quoted 9 lines · Bjorn Helgaas

Quoted 6 lines · canquan.shen

We run linux as a guest in Xen environment. When we used the xen tools (xm vcpu-set<n>) to hot add and remove vcpu to and from the guest, we encountered the failure on vcpu removal. We found the reason is that it didn't go to really remove cpu in the cpu removal code path.

Quoted 6 lines

Quoted 4 lines

This patch adds acpi_bus_trim in acpi_process_hotplug_notify to fix this issue. With this patch, it works fine for us.

Signed-off-by Canquan Shen<[email protected]>

Reviewed-by Bjorn Helgaas<[email protected]>

On second thought, let's think about this a bit more.

As I mentioned before, I have a long-term goal to move the hotplug flow out of drivers and into the ACPI core. That will be easier if the code in the drivers is as generic as possible.

The dock and acpiphp hot-remove code calls acpi_bus_trim(), then evaluates _EJ0. The core acpi_bus_hot_remove_device() function already does both acpi_bus_trim() and _EJ0. This function is currently only used when we write to sysfs "eject" files, but I wonder if we should use it in acpi_processor_hotplug_notify() as well.

That would get us one step closer to removing this gunk from the drivers and having acpi_bus_notify() look something like this:

     case ACPI_NOTIFY_EJECT_REQUEST:
         driver->ops.remove(device);
         acpi_bus_hot_remove_device(device);
         break;

Quoted 9 lines

There is a description of a CPU hot-remove that does include _EJ0
methods in the "DIG64 Hot-Plug& Partitioning Flows Specification"
[1], sec 2.2.4. I know this document is Itanium-oriented, but this part seems fairly generic and it's the only description of the process I've seen so far.

So would using acpi_bus_hot_remove_device() instead of acpi_bus_trim() also solve your problem, Canquan?

Yes. It can solve my problem.
I fully aggree to replace acpi_bus_hot_remove_device() to acpi_bus_trim(). Initially I insert the acpi_bus_hot_remove_device() in acpi_bus_notify function . lately I think I should give a chance for user,and so send
KOBJ_OFFLINE message to the udvev module.

But why add the driver->ops.remove(device) before acpi_bus_hot_remove_device(device). it can be called in acpi_bus_hot_remove_device code path as bellowing:

Quoted 6 lines

    acpi_bus_trim
        acpi_bus_remove
            device_release_driver
                __device_release_driver
                        acpi_device_remove
                                acpi_drv->ops.remove

OK. Maybe the future acpi_bus_notify() code would be even simpler. The point is that the ACPI core should handle the notification, call the driver's .remove() method, and do whatever namespace cleanup is required (i.e., acpi_bus_trim()). None of this should be in the driver itself.

Can you re-post your patch, using acpi_bus_hot_remove_device() instead of acpi_bus_trim()? Please include Khalid's tweak, too, so we don't print warnings for CPUs that don't supply _EJ0 methods.

Bjorn

.