Re: Shutdown panic in disable_nonboot_cpus after cpupool-numa-split

["Jürgen Groß" <jgross@suse.com>]

On 07/28/2014 10:36 AM, Stefan Bader wrote:
> On 07.07.2014 16:43, Stefan Bader wrote:
>> On 07.07.2014 16:28, Juergen Gross wrote:
>>> On 07/07/2014 04:08 PM, Stefan Bader wrote:
>>>> On 07.07.2014 15:03, Jürgen Groß wrote:
>>>>> On 07/07/2014 02:49 PM, Stefan Bader wrote:
>>>>>> On 07.07.2014 14:38, Jürgen Groß wrote:
>>>>>>> On 07/07/2014 02:00 PM, Andrew Cooper wrote:
>>>>>>>> On 07/07/14 12:33, Stefan Bader wrote:
>>>>>>>>> I recently noticed that I get a  panic (rebooting the system) on shutdown in
>>>>>>>>> some
>>>>>>>>     > cases. This happened only on my AMD system and also not all the time.
>>>>>>>> Finally
>>>>>>>>     > realized that it is related to the use of using cpupool-numa-split
>>>>>>>>     > (libxl with xen-4.4 maybe, but not 100% sure 4.3 as well).
>>>>>>>>     >
>>>>>>>>     > What happens is that on shutdown the hypervisor runs
>>>>>>>> disable_nonboot_cpus which
>>>>>>>>     > call cpu_down for each online cpu. There is a BUG_ON in the code for
>>>>>>>> the case of
>>>>>>>>     > cpu_down returning -EBUSY. This happens in my case as soon as the
>>>>>>>> first cpu that
>>>>>>>>     > has been moved to pool-1 by cpupool-numa-split is attempted. The
>>>>>>>> error is
>>>>>>>>     > returned by running the notifier_call_chain and I suspect that ends
>>>>>>>> up calling
>>>>>>>>     > cpupool_cpu_remove which always returns EBUSY for cpus not in pool0.
>>>>>>>>     >
>>>>>>>>     > I am not sure which end needs to be fixed but looping over all online
>>>>>>>> cpus in
>>>>>>>>     > disable_nonboot_cpus sounds plausible. So maybe the check for pool-0 in
>>>>>>>>     > cpupool_cpu_remove is wrong...?
>>>>>>>>     >
>>>>>>>>     > -Stefan
>>>>>>>>
>>>>>>>> Hmm yes - this looks completely broken.
>>>>>>>>
>>>>>>>> cpupool_cpu_remove() only has a single caller which is from cpu_down(),
>>>>>>>> and will unconditionally fail for cpus outside of the default pool.
>>>>>>>>
>>>>>>>> It is not obvious at all how this is supposed to work, and the comment
>>>>>>>> beside cpupool_cpu_remove() doesn't help.
>>>>>>>>
>>>>>>>> Can you try the following (only compile tested) patch, which looks
>>>>>>>> plausibly like it might DTRT.  The for_each_cpupool() is a little nastly
>>>>>>>> but there appears to be no cpu_to_cpupool mapping available.
>>>>>>>
>>>>>>> Your patch has the disadvantage to support hot-unplug of the last cpu in
>>>>>>> a cpupool. The following should work, however:
>>>>>>
>>>>>> Disadvantage and support sounded a bit confusing. But I think it means
>>>>>> hot-unplugging the last cpu of a pool is bad and should not be working.
>>>>>
>>>>> Correct.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
>>>>>>> index 4a0e569..73249d3 100644
>>>>>>> --- a/xen/common/cpupool.c
>>>>>>> +++ b/xen/common/cpupool.c
>>>>>>> @@ -471,12 +471,24 @@ static void cpupool_cpu_add(unsigned int cpu)
>>>>>>>      */
>>>>>>>     static int cpupool_cpu_remove(unsigned int cpu)
>>>>>>>     {
>>>>>>> -    int ret = 0;
>>>>>>> +    int ret = -EBUSY;
>>>>>>> +    struct cpupool **c;
>>>>>>>
>>>>>>>         spin_lock(&cpupool_lock);
>>>>>>> -    if ( !cpumask_test_cpu(cpu, cpupool0->cpu_valid))
>>>>>>> -        ret = -EBUSY;
>>>>>>> +    if ( cpumask_test_cpu(cpu, cpupool0->cpu_valid) )
>>>>>>> +        ret = 0;
>>>>>>>         else
>>>>>>> +    {
>>>>>>> +        for_each_cpupool(c)
>>>>>>> +        {
>>>>>>> +            if ( cpumask_test_cpu(cpu, (*c)->cpu_suspended ) )
>>>>>>
>>>>>> The rest seems to keep the semantics the same as before (though does that mean
>>>>>> unplugging the last cpu of pool-0 is ok?) But why testing for suspended here to
>>>>>> succeed (and not valid)?
>>>>>
>>>>> Testing valid would again enable to remove the last cpu of a cpupool in
>>>>> case of hotplugging. cpu_suspended is set if all cpus are to be removed
>>>>> due to shutdown, suspend to ram/disk, ...
>>>>
>>>> Ah, ok. Thanks for the detail explanation. So I was trying this change in
>>>> parallel and can confirm that it gets rid of the panic on shutdown. But when I
>>>> try to offline any cpu in pool1 (if echoing 0 into /sys/devices/xen_cpu/xen_cpu?
>>>> is the correct method) I always get EBUSY. IOW I cannot hot-unplug any cpu that
>>>> is in a pool other than 0. It does only work after removing it from pool1, then
>>>> add it to pool0 and then echo 0 into online.
>>>
>>> That's how it was designed some years ago. I don't want to change the
>>> behavior in the hypervisor. Adding some tool support could make sense,
>>> however.
>>
>> Ok, so in that case everything works as expected and the change fixes the
>> currently broken shutdown and could be properly submitted for inclusion (with my
>> tested-by).
>>
>
> Is this needing something from my side to do? I could re-submit the whole patch
> but it since it is Juergen's work it felt a little rude to do so.

Patch is already in xen-unstable/staging.


Juergen