Re: [PATCH 1/2] acpi : cpu hot-remove returns error number when cpu_down() fails

[Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>]

Hi Srivatsa,

2012/07/10 16:57, Srivatsa S. Bhat wrote:
> On 07/10/2012 10:27 AM, Yasuaki Ishimatsu wrote:
>> Hi Toshi,
>>
>> 2012/07/10 6:15, Toshi Kani wrote:
>>> On Mon, 2012-07-09 at 16:55 +0530, Srivatsa S. Bhat wrote:
>>>> On 07/09/2012 08:01 AM, Yasuaki Ishimatsu wrote:
>>>>> Hi Srivatsa,
>>>>>
>>>>> Thank you for your reviewing.
>>>>>
>>>>> 2012/07/06 18:51, Srivatsa S. Bhat wrote:
>>>>>> On 07/06/2012 08:46 AM, Yasuaki Ishimatsu wrote:
>>>>>>> Even if cpu_down() fails, acpi_processor_remove() continues to
>>>>>>> remove the cpu.
>>>>>>
>>>>>> Ouch!
>>>>>>
>>>>>>> But in this case, it should return error number since some process
>>>>>>> may run on
>>>>>>> the cpu. If the cpu has a running process and the cpu is turned
>>>>>>> the power off,
>>>>>>> the system cannot work well.
>>>>>>>
>>>>>>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>>>>>
>>>>>>> ---
>>>>>>>     drivers/acpi/processor_driver.c |   18 ++++++++++++------
>>>>>>>     1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> Index: linux-3.5-rc4/drivers/acpi/processor_driver.c
>>>>>>> ===================================================================
>>>>>>> --- linux-3.5-rc4.orig/drivers/acpi/processor_driver.c
>>>>>>> 2012-06-25 04:53:04.000000000 +0900
>>>>>>> +++ linux-3.5-rc4/drivers/acpi/processor_driver.c    2012-07-05
>>>>>>> 21:02:58.711285382 +0900
>>>>>>> @@ -610,7 +610,7 @@ err_free_pr:
>>>>>>>     static int acpi_processor_remove(struct acpi_device *device,
>>>>>>> int type)
>>>>>>>     {
>>>>>>>         struct acpi_processor *pr = NULL;
>>>>>>> -
>>>>>>> +    int ret;
>>>>>>>
>>>>>>>         if (!device || !acpi_driver_data(device))
>>>>>>>             return -EINVAL;
>>>>>>> @@ -621,8 +621,9 @@ static int acpi_processor_remove(struct
>>>>>>>             goto free;
>>>>>>>
>>>>>>>         if (type == ACPI_BUS_REMOVAL_EJECT) {
>>>>>>> -        if (acpi_processor_handle_eject(pr))
>>>>>>> -            return -EINVAL;
>>>>>>> +        ret = acpi_processor_handle_eject(pr);
>>>>>>> +        if (ret)
>>>>>>> +            return ret;
>>>>>>>         }
>>>>>>>
>>>>>>>         acpi_processor_power_exit(pr, device);
>>>>>>> @@ -841,12 +842,17 @@ static acpi_status acpi_processor_hotadd
>>>>>>>
>>>>>>>     static int acpi_processor_handle_eject(struct acpi_processor *pr)
>>>>>>>     {
>>>>>>> -    if (cpu_online(pr->id))
>>>>>>> -        cpu_down(pr->id);
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    if (cpu_online(pr->id)) {
>>>>>>> +        ret = cpu_down(pr->id);
>>>>>>> +        if (ret)
>>>>>>> +            return ret;
>>>>>>> +    }
>>>>>>>
>>>>>>
>>>>>> Strictly speaking, this is not thorough enough. What prevents someone
>>>>>> from onlining that same cpu again, at this point?
>>>>>> So, IMHO, you need to wrap the contents of this function inside a
>>>>>> get_online_cpus()/put_online_cpus() block, to prevent anyone else
>>>>>> from messing with CPU hotplug at the same time.
>>>>>
>>>>> If I understand your comment by mistake, please let me know.
>>>>> If the contents is wrapped a inside
>>>>> get_online_cpus()/put_online_cpus() block
>>>>> as below, cpu_down() will stop since cpu_down() calls
>>>>> cpu_hotplug_begin() and
>>>>> cpu_hotplug_begin() waits for cpu_hotplug.refcount to become 0.
>>>>>
>>>>
>>>> You are right. Sorry, I overlooked that.
>>>>
>>>>> +    get_online_cpus()
>>>>> +    if (cpu_online(pr->id)) {
>>>>> +        ret = cpu_down(pr->id);
>>>>> +        if (ret)
>>>>> +            return ret;
>>>>> +    }
>>>>> +    put_online_cpus()
>>>>>
>>>>> I think following patch can prevent it correctly. How about the patch?
>>>>>
>>>>> ---
>>>>>    drivers/acpi/processor_driver.c |   12 ++++++++++++
>>>>>    kernel/cpu.c                    |    8 +++++---
>>>>>    2 files changed, 17 insertions(+), 3 deletions(-)
>>>>>
>>>>> Index: linux-3.5-rc4/drivers/acpi/processor_driver.c
>>>>> ===================================================================
>>>>> --- linux-3.5-rc4.orig/drivers/acpi/processor_driver.c    2012-07-09
>>>>> 09:59:01.280211202 +0900
>>>>> +++ linux-3.5-rc4/drivers/acpi/processor_driver.c    2012-07-09
>>>>> 11:05:34.559859236 +0900
>>>>> @@ -844,14 +844,26 @@ static int acpi_processor_handle_eject(s
>>>>>    {
>>>>>        int ret;
>>>>>
>>>>> +retry:
>>>>>        if (cpu_online(pr->id)) {
>>>>>            ret = cpu_down(pr->id);
>>>>>            if (ret)
>>>>>                return ret;
>>>>>        }
>>>>>
>>>>> +    get_online_cpus();
>>>>> +    /*
>>>>> +     * Someone might online the cpu again at this point. So we
>>>>> check that
>>>>> +     * cpu has been onlined or not. If cpu is online, we try to
>>>>> offline
>>>>> +     * the cpu again.
>>>>> +     */
>>>>> +    if (cpu_online(pr->id)) {
>>>>
>>>> How about this:
>>>>      if (unlikely(cpu_online(pr->id)) {
>>>> since the probability of this happening is quite small...
>>>>
>>>>> +        put_online_cpus();
>>>>> +        goto retry;
>>>>> +    }
>>>>>        arch_unregister_cpu(pr->id);
>>>>>        acpi_unmap_lsapic(pr->id);
>>>>> +    put_online_cpus();
>>>>>        return ret;
>>>>>    }
>>>>
>>>> This retry logic doesn't look elegant, but I don't see any better
>>>> method :-(
>>>
>>> Another possible option is to fail the request instead of retrying it.
>>
>> Good idea!! I'll update it.
>>
>>>
>>> It would be quite challenging to allow on-lining and off-lining
>>> operations to run concurrently.  In fact, even if we close this window,
>>> there is yet another window right after the new put_online_cpus() call.
>>
>> I think if we close the window, another window does not open.
>> acpi_unmap_lsapic() sets cpu_present mask to false before new
>> put_online_cpus()
>> is called. So even if _cpu_up() is called, the function returns -EINAVL by
>> following added code.
>>
>> @@ -343,11 +343,13 @@ static int __cpuinit _cpu_up(unsigned in
>>       unsigned long mod = tasks_frozen ? CPU_TASKS_FROZEN : 0;
>>       struct task_struct *idle;
>>
>> -    if (cpu_online(cpu) || !cpu_present(cpu))
>> -        return -EINVAL;
>> -
>>       cpu_hotplug_begin();
>>
>> +    if (cpu_online(cpu) || !cpu_present(cpu)) {
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>>
>
> Right. Yasuaki's patch will ensure that there are no more race conditions
> because it does the cpu_present() check after taking the cpu_hotplug.lock.
> So I think it is safe and still doable from the kernel's perspective.
>
> But the question is, "should we do it?" I think Toshi's suggestion of failing
> the hot-remove request (if we find that the cpu has been onlined again by some
> other task) sounds like a good idea for another reason: cpu hotplug is not
> initiated by the OS by itself; its requested by the user; so if something is
> onlining the cpu back again, the user better take a second look and decide
> what exactly he wants to do with that cpu - whether keep it online or
> hot-remove it out.

I think so too. Failing the hot-remove request is good idea.

>
> Trying to online as well as hot-remove the same cpu simultaneously sounds like
> a crazy thing to do, and returning -EBUSY or -EAGAIN in the hot-remove case
> (ie., failing that request) would give a warning to the user and a chance to
> reflect upon what exactly he wants to do with the cpu.
>
> So, IMHO, we should protect against the race condition (between cpu_up and
> hot-remove) but choose to fail the hot-remove request, and add a comment saying
> why we chose to fail the request, even though we could have gone ahead and
> completed it.

I have already sent 2nd version of the patch. But the warning message is
not included in the patch. So I will add the warning message into 3rd
version.

Thanks,
Yasuaki Ishimatsu

> Regards,
> Srivatsa S. Bhat
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>