[PATCH v4] acpi: Fix CPU hot removal problem

[PATCH v4] acpi: Fix CPU hot removal problem

[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.

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 <shencanquan@huawei.com>
---
  drivers/acpi/processor_driver.c |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/processor_driver.c 
b/drivers/acpi/processor_driver.c
index a4e0f1b..03d92d6 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -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) {
@@ -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

Re: [PATCH v4] acpi: Fix CPU hot removal problem

[Bjorn Helgaas]

On Wed, Sep 14, 2011 at 7:06 PM, canquan.shen <shencanquan@huawei.com> wrote:
> 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 <shencanquan@huawei.com>

Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/acpi/processor_driver.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/acpi/processor_driver.c
> b/drivers/acpi/processor_driver.c
> index a4e0f1b..03d92d6 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -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) {
> @@ -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
>
>

Re: [PATCH v4] acpi: Fix CPU hot removal problem

[Bjorn Helgaas]

On Wed, Sep 14, 2011 at 8:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Sep 14, 2011 at 7:06 PM, canquan.shen <shencanquan@huawei.com> wrote:
>> 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 <shencanquan@huawei.com>
>
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

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

>> ---
>>  drivers/acpi/processor_driver.c |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_driver.c
>> b/drivers/acpi/processor_driver.c
>> index a4e0f1b..03d92d6 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -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) {
>> @@ -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
>>
>>
>

Re: [PATCH v4] acpi: Fix CPU hot removal problem

[Khalid Aziz]

On Thu, 2011-09-22 at 10:53 -0600, Bjorn Helgaas wrote:
> On Wed, Sep 14, 2011 at 8:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Wed, Sep 14, 2011 at 7:06 PM, canquan.shen <shencanquan@huawei.com> wrote:
> >> 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 <shencanquan@huawei.com>
> >
> > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> 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?

I have been looking at this code and I have been thinking along the same
lines. Using acpi_bus_trim() to remove CPU does not power down the CPU
and allow firmware to deconfigure it. Calling
acpi_bus_hot_remove_device() is a better approach. While we are at it,
we should also fix the conditional in acpi_bus_hot_remove_device() after
executing _EJ0 to make sure we do not print warning if _EJ0 is not
supported by firmware:

--- scan.c.orig	2011-09-22 11:14:52.801074429 -0600
+++ scan.c	2011-09-22 11:15:24.061699647 -0600
@@ -129,7 +129,7 @@ static void acpi_bus_hot_remove_device(v
 	 * TBD: _EJD support.
 	 */
 	status = acpi_evaluate_object(handle, "_EJ0", &arg_list, NULL);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
 		printk(KERN_WARNING PREFIX
 				"Eject device failed\n");
 

--
Khalid


> Bjorn
> 
> [1] http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf
> 
> >> ---
> >>  drivers/acpi/processor_driver.c |    6 ++++++
> >>  1 files changed, 6 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/acpi/processor_driver.c
> >> b/drivers/acpi/processor_driver.c
> >> index a4e0f1b..03d92d6 100644
> >> --- a/drivers/acpi/processor_driver.c
> >> +++ b/drivers/acpi/processor_driver.c
> >> @@ -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) {
> >> @@ -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
> >>
> >>
> >

-- 
====================================================================
Khalid Aziz                          Server Solutions Technology Lab
(970)898-9214                                        Hewlett-Packard
khalid.aziz@hp.com                                  Fort Collins, CO

Re: [PATCH v4] acpi: Fix CPU hot removal problem

[canquan.shen]

On 2011/9/23 0:53, Bjorn Helgaas wrote:
> On Wed, Sep 14, 2011 at 8:56 PM, Bjorn Helgaas<bhelgaas@google.com>  wrote:
>> On Wed, Sep 14, 2011 at 7:06 PM, canquan.shen<shencanquan@huawei.com>  wrote:
>>> 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<shencanquan@huawei.com>
>>
>> Reviewed-by: Bjorn Helgaas<bhelgaas@google.com>
>
> 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?
>
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

> Bjorn
>
> [1] http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf
>
>>> ---
>>>   drivers/acpi/processor_driver.c |    6 ++++++
>>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/acpi/processor_driver.c
>>> b/drivers/acpi/processor_driver.c
>>> index a4e0f1b..03d92d6 100644
>>> --- a/drivers/acpi/processor_driver.c
>>> +++ b/drivers/acpi/processor_driver.c
>>> @@ -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) {
>>> @@ -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
>>>
>>>
>>
>
> .
>

Re: [PATCH v4] acpi: Fix CPU hot removal problem

[Bjorn Helgaas]

On Fri, Sep 23, 2011 at 1:49 AM, canquan.shen <shencanquan@huawei.com> wrote:
> On 2011/9/23 0:53, Bjorn Helgaas wrote:
>>
>> On Wed, Sep 14, 2011 at 8:56 PM, Bjorn Helgaas<bhelgaas@google.com>
>>  wrote:
>>>
>>> On Wed, Sep 14, 2011 at 7:06 PM, canquan.shen<shencanquan@huawei.com>
>>>  wrote:
>>>>
>>>> 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<shencanquan@huawei.com>
>>>
>>> Reviewed-by: Bjorn Helgaas<bhelgaas@google.com>
>>
>> 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?
>>
> 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

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

Re: [PATCH v4] acpi: Fix CPU hot removal problem

[canquan.shen]

On 2011/9/23 22:16, Bjorn Helgaas wrote:
> On Fri, Sep 23, 2011 at 1:49 AM, canquan.shen<shencanquan@huawei.com>  wrote:
>> On 2011/9/23 0:53, Bjorn Helgaas wrote:
>>>
>>> On Wed, Sep 14, 2011 at 8:56 PM, Bjorn Helgaas<bhelgaas@google.com>
>>>   wrote:
>>>>
>>>> On Wed, Sep 14, 2011 at 7:06 PM, canquan.shen<shencanquan@huawei.com>
>>>>   wrote:
>>>>>
>>>>> 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<shencanquan@huawei.com>
>>>>
>>>> Reviewed-by: Bjorn Helgaas<bhelgaas@google.com>
>>>
>>> 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?
>>>
>> 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
>
> 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
>
> .
>
OK. I will modify it and re-post the patch which will include Khalid's 
tweak.  and Thank Khalid for attention this patch.