Re: [PATCH] xen/arm: Fix sibling mask setup to only include online CPUs

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hello Hirokazu-san,

Hirokazu Takahashi <taka@xxxxxxxxxxxxx> writes:

> Populate the CPU sibling map only with CPUs that are brought
> online. Currently, all CPUs in cpu_possible_map are added,
> which leaves CPUs that failed to boot in the sibling masks and
> may lead the scheduler to make incorrect decisions.

I think that this is a scheduler issue then. A CPU has siblings
regardless of if they are running or not. Also, your change will affect
CPU Hotplug functionality, as with your patch sibling mask will change
in runtime. But partially - only in cpu_up, but not on cpu_down. I am
not sure that schedulers can handle updates to siblings mask in runtime.

Could you please elaborate how it affects scheduler behaviour right now?

>
> Fixes: 701190abb3d5 ("xen/arm: Set correct per-cpu cpu_core_mask")
> Signed-off-by: Hirokazu Takahashi <taka@xxxxxxxxxxxxx>
> ---
>  xen/arch/arm/smpboot.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 8c9072f3c1..2d01896cf0 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -91,15 +91,24 @@ static int setup_cpu_sibling_map(int cpu)
>           !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) )
>          return -ENOMEM;
>  
> +    return 0;
> +}
> +
> +static void init_cpu_sibling_map(int cpu)
> +{
> +    unsigned int sibling;
> +
>      /*
>       * Currently we assume there is no multithread and NUMA, so
>       * a CPU is a sibling with itself, and the all possible CPUs
> -     * are supposed to belong to the same socket (NUMA node).
> +     * are supposed to belong to the same socket.
>       */
>      cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
> -    cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map);
> +    cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_online_map);
> +    cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
>  
> -    return 0;
> +    for_each_online_cpu(sibling)
> +        cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, sibling));
>  }
>  
>  static void remove_cpu_sibling_map(int cpu)
> @@ -314,6 +323,7 @@ smp_prepare_cpus(void)
>      if ( rc )
>          panic("Unable to allocate CPU sibling/core maps\n");
>  
> +    init_cpu_sibling_map(0);
>  }
>  
>  /* Boot the current CPU */
> @@ -385,6 +395,8 @@ void asmlinkage noreturn start_secondary(void)
>  
>      set_current(idle_vcpu[cpuid]);
>  
> +    init_cpu_sibling_map(cpuid);
> +
>      /* Run local notifiers */
>      notify_cpu_starting(cpuid);
>      /*

-- 
WBR, Volodymyr