RE: [PATCH v12 2/4] xen/sched: Link CPU topology to scheduler

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hi Michal,

Thank you for the review.

> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index d34396f9b1..cf4858eb37 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -93,13 +93,7 @@ static int setup_cpu_sibling_map(int cpu)
> >           !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) )
> >          return -ENOMEM;
> >
> > -    /*
> > -     * 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).
> > -     */
> > -    cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
> > -    cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map);
> > +    init_cpu_sibling_map(cpu);
> The comment above the cpu_sibling_mask definition is now stale.

Yes. This patch removes the comment.


> > +    for_each_possible_cpu(cpu)
> > +    {
> > +        unsigned int first_cpu;
> > +        struct cpu_topology *topo = &cpu_topology[cpu];
> > +
> > +        first_cpu = cpumask_first(topo->thread_sibling);
> > +        ASSERT(first_cpu < nr_cpu_ids);
> > +
> > +        if ( first_cpu == cpu )
> > +        {
> > +            topo->phys_core_id = next_core_id;
> > +            next_core_id++;
> > +        }
> > +        else
> > +        {
> > +            topo->phys_core_id = cpu_topology[first_cpu].phys_core_id;
> > +        }
> No braces for single line statements.

Okay.

> > +
> > +        first_cpu = cpumask_first(topo->cluster_sibling);
> > +        if ( first_cpu >= nr_cpu_ids )
> > +        {
> > +            /* Clustering is not supported */
> > +            topo->phys_cluster_id = 0;
> > +        }
> > +        else
> > +        {
> > +            if ( first_cpu == cpu )
> > +            {
> > +                topo->phys_cluster_id = next_cluster_id;
> > +                next_cluster_id++;
> > +            }
> > +            else
> > +            {
> > +                topo->phys_cluster_id = 
> > cpu_topology[first_cpu].phys_cluster_id;
> > +            }
> No braces for single line statements.

Okay.

> > +        }
> > +
> > +        first_cpu = cpumask_first(topo->core_sibling);
> > +        ASSERT(first_cpu < nr_cpu_ids);
> > +
> > +        if ( first_cpu == cpu )
> > +        {
> > +            topo->phys_socket_id = next_socket_id;
> > +            next_socket_id++;
> > +        }
> > +        else
> > +        {
> > +            topo->phys_socket_id = cpu_topology[first_cpu].phys_socket_id;
> > +        }
> No braces for single line statements.

Okay.

> >  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;
> phys_cluster_id and cluster_sibling are only used to compute each other. Are
> they really needed?

I kept them because I plan to make it possible to specify "cluster" for the
credit2_runqueue parameter.


> > +    unsigned int phys_socket_id;
> > +    unsigned int num_siblings;
> >  };
> >
> >  extern struct cpu_topology *cpu_topology;
> > +
> > +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);
> cpu_topology[] is built from all CPUs, so a CPU that then fails to boot
> stays in its siblings masks. I think a CPU should be added to these masks only
> once it is online.

The original code prior to this patch initialized the CPU sibling map in the 
CPU_UP_PREPARE
Notifier before bringing up a secondary core. I agree that it is appropriate to 
initialize it after
the core is actually brought up, similar to how x86 Xen does it.

Thank you,
Hirokazu Takahashi