Re : [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem

Re : [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem

[Shen Canquan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb2312, Size: 2441 bytes --]

Hi Len,
    I have tested more than 10000 times of cpu remove and  add function in the latest kernel. The result of test is ok. The flow of  my test program is bellow:
    (1)  get random number of cpu .
    (2)  call xm vcpu-set <n> . depend on current cpu number. it simulate the function of add or remove cpu at random.
    (3) get the random number between 1 and 10.  
    (4) sleep random seconds which get from step 3.
    (5) repeat from step 1 to step 4.

   what's your opinion of this patch?

--
shencanquan
________________________________________
·¢¼þÈË: linux-kernel-owner@vger.kernel.org [linux-kernel-owner@vger.kernel.org] ´ú±í Shen Canquan [shencanquan@huawei.com]
·¢ËÍʱ¼ä: 2011Äê8ÔÂ30ÈÕ 14:35
µ½: lenb@kernel.org; rui.zhao@intel.com; shemminger@vyatta.com; yakui.zhao@intel.com
Cc: xiaowei.yang@huawei.com; hanweidong; linux-kernel@vger.kernel.org; i@vger.kernel.org
Ö÷Ìâ: [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem

In Xen virtualization environment, When I used xen tools (xm vcpu-set vcpu_number ) to test the vcpu add and remove, I found it is failure on vcpu remove, I found the reason is that nothing to do when cpu remove in acpi_processor_hotplug_notify function, so I add the code of send the OFFLINE message to udev and add the rule of udev. it is ok on vcpu remove.
Signed-off-by: Shen canquan <shencanquan@huawei.com>
---
 drivers/acpi/processor_driver.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index a4e0f1b..a1c564f 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -677,6 +677,8 @@ static void acpi_processor_hotplug_notify(acpi_handle handle,
                                     "Driver data is NULL, dropping EJECT\n");
                         return;
                 }
+
+               kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
                 break;
         default:
                 ACPI_DEBUG_PRINT((ACPI_DB_INFO,
--
1.7.6.msysgit.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re : [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem

[Bjorn Helgaas]

> 主题: [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem
>
> In Xen virtualization environment, When I used xen tools (xm vcpu-set vcpu_number ) to test the vcpu add and remove, I found it is failure on vcpu remove, I found the reason is that nothing to do when cpu remove in acpi_processor_hotplug_notify function, so I add the code of send the OFFLINE message to udev and add the rule of udev. it is ok on vcpu remove.
> Signed-off-by: Shen canquan <shencanquan@huawei.com>
> ---
>  drivers/acpi/processor_driver.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index a4e0f1b..a1c564f 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -677,6 +677,8 @@ static void acpi_processor_hotplug_notify(acpi_handle handle,
>                                     "Driver data is NULL, dropping EJECT\n");
>                         return;
>                 }
> +
> +               kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
>                 break;
>         default:
>                 ACPI_DEBUG_PRINT((ACPI_DB_INFO,

The processor driver used to generate ONLINE and OFFLINE messages.  I
removed them with c1815e0740.  According to the changelog, the driver
core still generates KOBJ_ADD and KOBJ_REMOVE events.

Could you fix the Xen vcpu add/remove problem by looking for
ADD/REMOVE events rather than ONLINE/OFFLINE?

I removed ONLINE/OFFLINE because I think hotplug should be handled in
the ACPI core, not in the drivers.  Removing ONLINE/OFFLINE got rid of
one small piece of driver-specific hotplug handling.  Obviously,
there's still a lot to do.

If using ADD/REMOVE is not sufficient, what is the justification for
needing both REMOVE and OFFLINE, and how would they be different?

Bjorn

Re: Re : [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem

[canquan.shen]

On 2011/9/6 12:19, Bjorn Helgaas wrote:
>> 主题: [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem
>>
>> In Xen virtualization environment, When I used xen tools (xm vcpu-set vcpu_number ) to test the vcpu add and remove, I found it is failure on vcpu remove, I found the reason is that nothing to do when cpu remove in acpi_processor_hotplug_notify function, so I add the code of send the OFFLINE message to udev and add the rule of udev. it is ok on vcpu remove.
>> Signed-off-by: Shen canquan<shencanquan@huawei.com>
>> ---
>>   drivers/acpi/processor_driver.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index a4e0f1b..a1c564f 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -677,6 +677,8 @@ static void acpi_processor_hotplug_notify(acpi_handle handle,
>>                                      "Driver data is NULL, dropping EJECT\n");
>>                          return;
>>                  }
>> +
>> +               kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
>>                  break;
>>          default:
>>                  ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>
> The processor driver used to generate ONLINE and OFFLINE messages.  I
> removed them with c1815e0740.  According to the changelog, the driver
> core still generates KOBJ_ADD and KOBJ_REMOVE events.
>

Thanks for your answer.
When I used xen tools (xm vcpu-set vcpu_numer) to reduce the cpu number. 
I don't found any event . I use the following tool to capture the event: 
udevadm monitor --env --kernel --udev.

If I used xen tools to add the cpu number , udev module will receive the 
KOBJ_ADD event.

In my patch ,It is more fine to replace KOBJ_OFFLINE to KOBJ_REMOVE event .

btw: how to find changelog of the c1815e0740.

> Could you fix the Xen vcpu add/remove problem by looking for
> ADD/REMOVE events rather than ONLINE/OFFLINE?
>

Xen vcpu add/remove module has send the sci interrupt to linux ACPI Core 
module. the sci interrupt handler function (acpi_ev_gpe_xrupt_handler) 
has hanled it and called to AML code of processor to add and remove cpu. 
and then call back acpi_processor_hotplug_notify function to notify 
linux acpi processor driver .



> I removed ONLINE/OFFLINE because I think hotplug should be handled in
> the ACPI core, not in the drivers.  Removing ONLINE/OFFLINE got rid of
> one small piece of driver-specific hotplug handling.  Obviously,
> there's still a lot to do.
>
> If using ADD/REMOVE is not sufficient, what is the justification for
> needing both REMOVE and OFFLINE, and how would they be different?
>

I think the device of processor add and remove should be handle in 
processor driver . The ONLINE/OFFLINE function of CPU should be handle 
in linux kernel. so when removing cpu by Xen. at first it should be 
notify linux kernel to offline the cpu and then remove the device of 
processor.

btw: I found acpi_eject_store function in driver/acpi/scan.c will deal 
the offline cpu and remove the device of processor.


> Bjorn
>
>

Re: Re : [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem

[Bjorn Helgaas]

On Mon, Sep 5, 2011 at 11:48 PM, canquan.shen <shencanquan@huawei.com> wrote:
> On 2011/9/6 12:19, Bjorn Helgaas wrote:
>>>
>>> 主题: [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem
>>>
>>> In Xen virtualization environment, When I used xen tools (xm vcpu-set
>>> vcpu_number ) to test the vcpu add and remove, I found it is failure on vcpu
>>> remove, I found the reason is that nothing to do when cpu remove in
>>> acpi_processor_hotplug_notify function, so I add the code of send the
>>> OFFLINE message to udev and add the rule of udev. it is ok on vcpu remove.
>>> Signed-off-by: Shen canquan<shencanquan@huawei.com>
>>> ---
>>>  drivers/acpi/processor_driver.c |    2 ++
>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>> diff --git a/drivers/acpi/processor_driver.c
>>> b/drivers/acpi/processor_driver.c
>>> index a4e0f1b..a1c564f 100644
>>> --- a/drivers/acpi/processor_driver.c
>>> +++ b/drivers/acpi/processor_driver.c
>>> @@ -677,6 +677,8 @@ static void acpi_processor_hotplug_notify(acpi_handle
>>> handle,
>>>                                     "Driver data is NULL, dropping
>>> EJECT\n");
>>>                         return;
>>>                 }
>>> +
>>> +               kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
>>>                 break;
>>>         default:
>>>                 ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>>
>> The processor driver used to generate ONLINE and OFFLINE messages.  I
>> removed them with c1815e0740.  According to the changelog, the driver
>> core still generates KOBJ_ADD and KOBJ_REMOVE events.
>
> Thanks for your answer.
> When I used xen tools (xm vcpu-set vcpu_numer) to reduce the cpu number. I
> don't found any event . I use the following tool to capture the event:
> udevadm monitor --env --kernel --udev.
>
> If I used xen tools to add the cpu number , udev module will receive the
> KOBJ_ADD event.
>
> In my patch ,It is more fine to replace KOBJ_OFFLINE to KOBJ_REMOVE event .

I don't think we should emit KOBJ_REMOVE from the ACPI processor
driver.  KOBJ_ADD is emitted by the driver model core
(device_register() -> device_add() path), and I think KOBJ_REMOVE
should also be emitted from the driver model core.

Is acpi_processor_remove() called when you remove a processor?  I see
a path where it will be called via acpi_eject_store():

    acpi_eject_store
      acpi_os_hotplug_execute(acpi_bus_hot_remove_device)
      acpi_bus_hot_remove_device
        acpi_bus_trim
          acpi_bus_remove
            device_release_driver
              dev->driver->remove (acpi_processor_remove())
            acpi_device_unregister
              device_unregister
                device_del
                  kobject_uevent(KOBJ_REMOVE)

but as far as I can tell, this path is only used when we write
something to the "eject" sysfs file.  I would think we'd want to use
most of this same path when we hot remove a CPU via the ACPI SCI
mechanism.

If you change acpi_processor_hotplug_notify() to use acpi_bus_trim()
for the removal case, you should get KOBJ_REMOVE events.  Would that
be enough to make the xen vcpu remove work, or at least get you
closer?

> btw: how to find changelog of the c1815e0740.

git show c1815e0740

Bjorn

Re: Re : [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem

[canquan.shen]

On 2011/9/7 2:38, Bjorn Helgaas wrote:
> On Mon, Sep 5, 2011 at 11:48 PM, canquan.shen<shencanquan@huawei.com>  wrote:
>> On 2011/9/6 12:19, Bjorn Helgaas wrote:
>>>>
>>>> 主题: [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem
>>>>
>>>> In Xen virtualization environment, When I used xen tools (xm vcpu-set
>>>> vcpu_number ) to test the vcpu add and remove, I found it is failure on vcpu
>>>> remove, I found the reason is that nothing to do when cpu remove in
>>>> acpi_processor_hotplug_notify function, so I add the code of send the
>>>> OFFLINE message to udev and add the rule of udev. it is ok on vcpu remove.
>>>> Signed-off-by: Shen canquan<shencanquan@huawei.com>
>>>> ---
>>>>   drivers/acpi/processor_driver.c |    2 ++
>>>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>>> diff --git a/drivers/acpi/processor_driver.c
>>>> b/drivers/acpi/processor_driver.c
>>>> index a4e0f1b..a1c564f 100644
>>>> --- a/drivers/acpi/processor_driver.c
>>>> +++ b/drivers/acpi/processor_driver.c
>>>> @@ -677,6 +677,8 @@ static void acpi_processor_hotplug_notify(acpi_handle
>>>> handle,
>>>>                                      "Driver data is NULL, dropping
>>>> EJECT\n");
>>>>                          return;
>>>>                  }
>>>> +
>>>> +               kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
>>>>                  break;
>>>>          default:
>>>>                  ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>>>
>>> The processor driver used to generate ONLINE and OFFLINE messages.  I
>>> removed them with c1815e0740.  According to the changelog, the driver
>>> core still generates KOBJ_ADD and KOBJ_REMOVE events.
>>
>> Thanks for your answer.
>> When I used xen tools (xm vcpu-set vcpu_numer) to reduce the cpu number. I
>> don't found any event . I use the following tool to capture the event:
>> udevadm monitor --env --kernel --udev.
>>
>> If I used xen tools to add the cpu number , udev module will receive the
>> KOBJ_ADD event.
>>
>> In my patch ,It is more fine to replace KOBJ_OFFLINE to KOBJ_REMOVE event .
>
> I don't think we should emit KOBJ_REMOVE from the ACPI processor
> driver.  KOBJ_ADD is emitted by the driver model core
> (device_register() ->  device_add() path), and I think KOBJ_REMOVE
> should also be emitted from the driver model core.
>
> Is acpi_processor_remove() called when you remove a processor?  I see
> a path where it will be called via acpi_eject_store():
>
>      acpi_eject_store
>        acpi_os_hotplug_execute(acpi_bus_hot_remove_device)
>        acpi_bus_hot_remove_device
>          acpi_bus_trim
>            acpi_bus_remove
>              device_release_driver
>                dev->driver->remove (acpi_processor_remove())
>              acpi_device_unregister
>                device_unregister
>                  device_del
>                    kobject_uevent(KOBJ_REMOVE)
>
> but as far as I can tell, this path is only used when we write
> something to the "eject" sysfs file.  I would think we'd want to use
> most of this same path when we hot remove a CPU via the ACPI SCI
> mechanism.
>

Because in my patch will send the KOBJ_REMOVE event to udev module. and 
I write a udev rule like the following:
ACTION=="remove",DRIVER=="processor",SUBSYSTEM=="acpi",RUN+="/bin/bash 
-c 'echo 1 > /sys%p/eject'"
This rule will write "1" to the "eject" sysfs file. and then call 
acpi_eject_store function.

> If you change acpi_processor_hotplug_notify() to use acpi_bus_trim()
> for the removal case, you should get KOBJ_REMOVE events.  Would that
> be enough to make the xen vcpu remove work, or at least get you
> closer?
>

Xen vcpu remove will work if add the acpi_bus_trim() in the 
acpi_processor_hotplug_notify() function.

But I have two question:
1) If acpi processor driver send the KOBJ_REMOVE(or KOBJ_OFFLINE_CPU) to 
udev module. user has a chance to decide to remove or not remove the cpu 
?  The default is remove cpu if user does not write any udev rule .
2) In the acpi_bus_trim function,  the call patch is following:
      acpi_bus_trim
       acpi_bus_remove
         device_release_driver
            dev->driver->remove (acpi_processor_remove())
                acpi_processor_handle_eject
                       cpu_down
          acpi_device_unregister
             device_unregister
                device_del
                  kobject_uevent(KOBJ_REMOVE)

   I think this call patch is not clear, and I think when acpi processor 
driver receive the eject event. at first it send KOBJ_OFFLINE event to 
udev module and udev rule decide to offline or not offline the cpu. and 
if offline cpu  and then acpi processor driver remove the processor 
device(by listen CPU_DEAD event can know the cpu had offlined)
and so I think it is fine if has the following call path:
     acpi_processor_hotplug_notify
         register_hotcpu_notifier
         kobject_uevent(KOBJ_OFFLINE)


and udev rule will offline the cpu
       store_online
          cpu_down
            _cpu_down
	       cpu_notify_nofail (CPU_DEAD event)

and  in acpi processor driver receive CPU_DEAD
      acpi_cpu_soft_notify
           acpi_bus_trim
               acpi_bus_remove
                device_release_driver
                   dev->driver->remove (acpi_processor_remove())
                acpi_device_unregister
                  device_unregister
                      device_del
                         kobject_uevent(KOBJ_REMOVE)


--
canquan.shen

>> btw: how to find changelog of the c1815e0740.
>
> git show c1815e0740
>
> Bjorn
>
> .
>

Re: Re : [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem

[Bjorn Helgaas]

On Tue, Sep 6, 2011 at 7:40 PM, canquan.shen <shencanquan@huawei.com> wrote:
> On 2011/9/7 2:38, Bjorn Helgaas wrote:

>> Is acpi_processor_remove() called when you remove a processor?  I see
>> a path where it will be called via acpi_eject_store():
>>
>>     acpi_eject_store
>>       acpi_os_hotplug_execute(acpi_bus_hot_remove_device)
>>       acpi_bus_hot_remove_device
>>         acpi_bus_trim
>>           acpi_bus_remove
>>             device_release_driver
>>               dev->driver->remove (acpi_processor_remove())
>>             acpi_device_unregister
>>               device_unregister
>>                 device_del
>>                   kobject_uevent(KOBJ_REMOVE)
>>
>> but as far as I can tell, this path is only used when we write
>> something to the "eject" sysfs file.  I would think we'd want to use
>> most of this same path when we hot remove a CPU via the ACPI SCI
>> mechanism.
>>
>
> Because in my patch will send the KOBJ_REMOVE event to udev module. and I
> write a udev rule like the following:
> ACTION=="remove",DRIVER=="processor",SUBSYSTEM=="acpi",RUN+="/bin/bash -c
> 'echo 1 > /sys%p/eject'"
> This rule will write "1" to the "eject" sysfs file. and then call
> acpi_eject_store function.

Hmmm.  I think I understand your proposal, but it seems like a
convoluted path to me.

I guess the real question is whether we must give userspace a chance
to decide whether to actually do the remove or not.  Is there a
requirement to do that?  Neither the dynamic device removal flow (ACPI
spec 4.0a, sec 6.3) nor the ejection flow example (fig 6-5) mentions
doing that.

I mentioned before that I think the ACPI hotplug code should be ripped
out of the drivers and consolidated in the ACPI core.  I think it's
pretty clear from the spec that the 0-0x7f notifications (Bus Check,
Device Check, Eject Request, etc.) are designed to be handled by the
core, not by individual drivers.  We handle hotplug in the drivers
today, but I think that's mainly because we never implemented support
in the Linux ACPI core.  There are comments in acpi_bus_check_device()
and acpi_bus_check_scope() about what we *should* be doing there.

I am opposed to adding more hotplug support to individual drivers
because I still hope that someday we'll support it in the ACPI core.
Many ACPI drivers don't support hotplug at all, and the ones that do
support hotplug do it in a variety of ways.  It's all quite a mess.

Bjorn

Re: Re : [PATCH] acpi: Fix hot cpu remove problem on acpi subsystem

[canquan.shen]

On 2011/9/7 14:57, Bjorn Helgaas wrote:
> On Tue, Sep 6, 2011 at 7:40 PM, canquan.shen<shencanquan@huawei.com>  wrote:
>> On 2011/9/7 2:38, Bjorn Helgaas wrote:
>
>>> Is acpi_processor_remove() called when you remove a processor?  I see
>>> a path where it will be called via acpi_eject_store():
>>>
>>>      acpi_eject_store
>>>        acpi_os_hotplug_execute(acpi_bus_hot_remove_device)
>>>        acpi_bus_hot_remove_device
>>>          acpi_bus_trim
>>>            acpi_bus_remove
>>>              device_release_driver
>>>                dev->driver->remove (acpi_processor_remove())
>>>              acpi_device_unregister
>>>                device_unregister
>>>                  device_del
>>>                    kobject_uevent(KOBJ_REMOVE)
>>>
>>> but as far as I can tell, this path is only used when we write
>>> something to the "eject" sysfs file.  I would think we'd want to use
>>> most of this same path when we hot remove a CPU via the ACPI SCI
>>> mechanism.
>>>
>>
>> Because in my patch will send the KOBJ_REMOVE event to udev module. and I
>> write a udev rule like the following:
>> ACTION=="remove",DRIVER=="processor",SUBSYSTEM=="acpi",RUN+="/bin/bash -c
>> 'echo 1>  /sys%p/eject'"
>> This rule will write "1" to the "eject" sysfs file. and then call
>> acpi_eject_store function.
>
> Hmmm.  I think I understand your proposal, but it seems like a
> convoluted path to me.
>
> I guess the real question is whether we must give userspace a chance
> to decide whether to actually do the remove or not.  Is there a
> requirement to do that?  Neither the dynamic device removal flow (ACPI
> spec 4.0a, sec 6.3) nor the ejection flow example (fig 6-5) mentions
> doing that.
>
    I think we should give userspace a chance to decide whether do  the 
remove or not. About the cpu remove, it has two part, one in the linux 
kernel which mainly online/offline cpu, another is acpi core driver, 
which mainly add and remove the device of processor.
    giving userspace a chance is not acpi spec requirement. but it is 
flexible for linux kernel. many driver use the udev mechanism to has a 
chance for user to decide how to handle the event of kernel.

> I mentioned before that I think the ACPI hotplug code should be ripped
> out of the drivers and consolidated in the ACPI core.  I think it's
> pretty clear from the spec that the 0-0x7f notifications (Bus Check,
> Device Check, Eject Request, etc.) are designed to be handled by the
> core, not by individual drivers.  We handle hotplug in the drivers
> today, but I think that's mainly because we never implemented support
> in the Linux ACPI core.  There are comments in acpi_bus_check_device()
> and acpi_bus_check_scope() about what we *should* be doing there.
>
> I am opposed to adding more hotplug support to individual drivers
> because I still hope that someday we'll support it in the ACPI core.
> Many ACPI drivers don't support hotplug at all, and the ones that do
> support hotplug do it in a variety of ways.  It's all quite a mess.
>
> Bjorn
>
> .
>

I admit it is convoluted path for hot cpu remove. and the acpi processor 
driver will be consolidated in the acip core. but how to do in the acpi 
core ? I think it maybe directly call acpi_bus_hot_remove_device or send 
KOBJ_OFFLINE event to linux kernel.

I will modify the processor driver by add the acpi_bus_trim function in 
acpi_processor_hotplug_notify. and create the patch for fix this problem.
Could you help me to merge to latest linux kernel? Thanks for your 
answer again.

---
canquan.shen