RE: [PATCH v6 2/5] xen/sched: Link CPU topology to scheduler

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hi Jan,

> >  static unsigned int cpu_nr_siblings(unsigned int cpu)
> >  {
> > -#ifdef CONFIG_X86
> > +#if defined(CONFIG_GENERIC_CPU_TOPOLOGY)
> > +    return cpu_topology ? cpu_topology[cpu].num_siblings : 1;
> > +#elif defined(CONFIG_X86)
> >      return cpu_data[cpu].x86_num_siblings;
> >  #else
> >      return 1;
> 
> Now that this is ordered more sensibly, an issue is becoming apparent: If and
> when x86 also supports GENERIC_CPU_TOPOLOGY, the present x86 logic
> should also
> be engaged when !cpu_topology. By re-arranging accordingly, you also avoid the
> need to duplicate the literal 1.

Okay.

> > @@ -11,15 +22,48 @@ struct cpu_topology {
> >      cpumask_var_t thread_sibling;
> >      cpumask_var_t core_sibling;
> >      cpumask_var_t cluster_sibling;
> > +    unsigned int phys_core_id;
> > +    unsigned int phys_cluster_id;
> > +    unsigned int phys_socket_id;
> > +    unsigned int num_siblings;
> >  };
> >
> >  extern struct cpu_topology *cpu_topology;
> >  void init_cpu_topology(void);
> >
> > +static inline void init_cpu_sibling_map(unsigned int cpu)
> > +{
> > +    if ( cpu_topology )
> > +    {
> > +        cpumask_copy(per_cpu(cpu_sibling_mask, cpu),
> > +                     cpu_topology[cpu].thread_sibling);
> > +        cpumask_copy(per_cpu(cpu_core_mask, cpu),
> > +                     cpu_topology[cpu].core_sibling);
> > +    }
> > +    else
> > +        init_cpu_sibling_map_default(cpu);
> > +}
> 
> Personally I consider this already being too redundant with ...
> 
> > +#define cpu_to_core(cpu) (cpu_topology ? cpu_topology[cpu].phys_core_id : 
> > 0)
> > +#define cpu_to_socket(cpu) (cpu_topology ? 
> > cpu_topology[cpu].phys_socket_id : 0)
> > +
> >  #else /* CONFIG_GENERIC_CPU_TOPOLOGY */
> >
> >  static inline void init_cpu_topology(void) {}
> >
> > +static inline void init_cpu_sibling_map(unsigned int cpu)
> > +{
> > +    init_cpu_sibling_map_default(cpu);
> > +}
> 
> ... this. Imo it would better be
> 
> static inline void init_cpu_sibling_map(unsigned int cpu)
> {
>     if ( IS_ENABLED(CONFIG_GENERIC_CPU_TOPOLOGY) && cpu_topology )
>     {
>         cpumask_copy(per_cpu(cpu_sibling_mask, cpu),
>                      cpu_topology[cpu].thread_sibling);
>         cpumask_copy(per_cpu(cpu_core_mask, cpu),
>                      cpu_topology[cpu].core_sibling);
>     }
>     else
>         init_cpu_sibling_map_default(cpu);
> }

Okay, I will use this approach.
 
> Which then of course requires the decl of cpu_topology (not its definition) to
> always be visible.

Thank you,
Hirokazu Takahashi.