Re: [Xen-devel] [PATCH v6 01/14] x86: add socket_to_cpumask

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.04.15 at 11:55, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> @@ -301,6 +304,8 @@ static void set_cpu_sibling_map(int cpu)
>              }
>          }
>      }
> +
> +    cpumask_set_cpu(cpu, &socket_to_cpumask[cpu_to_socket(cpu)]);
>  }

There is an early return path in this function, which you need to
deal with.

> @@ -717,6 +722,14 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>  
>      stack_base[0] = stack_start;
>  
> +    nr_sockets = DIV_ROUND_UP(nr_cpu_ids, boot_cpu_data.x86_max_cores *
> +                                          boot_cpu_data.x86_num_siblings);

I think this is going to be problematic when there are more CPUs
in the system than Xen can (or is told to) handle.

> +    ASSERT(nr_sockets > 0);

Due to the DIV_ROUND_UP() I think this can never trigger.

> +    socket_to_cpumask = xzalloc_array(cpumask_t, nr_sockets);

This is going to be very wasteful when Xen was built for very many
CPUs. I think the array should be of cpumask_var_t type, and the
array elements be initialized individually.

> --- a/xen/include/asm-x86/smp.h
> +++ b/xen/include/asm-x86/smp.h
> @@ -58,6 +58,14 @@ int hard_smp_processor_id(void);
>  
>  void __stop_this_cpu(void);
>  
> +/*
> + * This value is considered to not change from the initial startup.
> + * Otherwise all the relevant places need to be retrofitted.
> + */
> +extern unsigned int nr_sockets;
> +
> +/* Representing HT and core siblings in each socket */
> +extern cpumask_t *socket_to_cpumask;
>  #endif /* !__ASSEMBLY__ */

Please retain the blank line before the #endif.

Jan


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