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

[Stefan Bader <stefan.bader@canonical.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3193 bytes --]

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.

> 
> 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)?


> +            {
> +                ret = 0;
> +                break;
> +            }
> +        }
> +    }
> +    if ( !ret )
>          cpumask_set_cpu(cpu, &cpupool_locked_cpus);
>      spin_unlock(&cpupool_lock);
> 
> 
> 
> Juergen



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel