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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hello,

> Subject: Re: [PATCH 2/3] xen/sched: Link CPU topology to scheduler
> 
> On 10.06.2026 13:13, Hirokazu Takahashi wrote:
> > Make CPU topology information available to the Xen scheduler.
> > Additionally, ensure that this topology information is displayed
> > when executing the 'xl info -n' command.
> 
> Both in title and description you're pretty generic, yet then ...
> 
> >  xen/arch/arm/include/asm/processor.h  |  4 ---
> >  xen/arch/arm/smpboot.c                | 10 ++++--
> >  xen/common/device-tree/cpu-topology.c | 51
> +++++++++++++++++++++++++++
> >  xen/common/sched/credit2.c            |  3 ++
> >  xen/common/sysctl.c                   |  1 +
> >  xen/include/xen/cpu-topology.h        | 10 ++++++
> >  6 files changed, 72 insertions(+), 7 deletions(-)
> 
> ... only the credit2 scheduler is actually enabled.

While it is true that this patch is focused on Credit2, the CPU topology
information is also reflected in `cpu_sibling_mask` and `cpu_core_mask`.
These masks are globally populated and are already referenced by other
schedulers' code as well.

> > --- a/xen/common/device-tree/cpu-topology.c
> > +++ b/xen/common/device-tree/cpu-topology.c
> > @@ -325,6 +325,55 @@ int __init parse_dt_topology(void)
> >      return parse_socket(map);
> >  }
> >
> > +static void __init setup_cpu_topology_ids(void)
> > +{
> > +    unsigned int cpu;
> > +    unsigned int next_core_id = 0U;
> > +    unsigned int next_cluster_id = 0U;
> > +    unsigned int next_socket_id = 0U;
> > +
> > +    for_each_possible_cpu( cpu )
> 
> Nit (style): Either you deem for_each_possible_cpu a (pseudo-)keyword
> (then there's a blank missing) or you don't (then there are excess
> blanks).

Okay, I will fix them.

> > +    {
> > +        unsigned int first_cpu;
> > +        struct cpu_topology *topo = &cpu_topology[cpu];
> > +
> > +        first_cpu = cpumask_first(&topo->thread_sibling);
> > +        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;
> > +        }
> 
> Nit, here and below: Please omit unnecessary figure braces.

Okay, I will remove them.

> > @@ -339,4 +388,6 @@ void __init dt_init_cpu_topology(void)
> >
> >      for_each_possible_cpu( cpu )
> >          setup_siblings_masks(cpu);
> As to the earlier remark: Bad pre-existing examples don't count.

Okay.

> > @@ -19,11 +23,17 @@ extern struct cpu_topology *cpu_topology;
> >  void map_cpuid_to_node(unsigned int cpuid, struct dt_device_node
> *cpu_node);
> >  void dt_init_cpu_topology(void);
> >
> > +#define cpu_to_core(_cpu)   (cpu_topology[_cpu].phys_core_id)
> > +#define cpu_to_socket(_cpu)   (cpu_topology[_cpu].phys_socket_id)
> 
> Please can you avoid introducing new name space violations (identifiers
> with leading underscores should name file scope entities); also again
> below.

Ok, I will remove the leading underscores from the macro, which were
originally defined in xen/arch/arm/include/asm/processor.h.
Is it okay to leave the macro definitions in x86's 
`xen/arch/x86/include/asm/processor.h` untouched?

Thank you,
Hirokazu Takahashi.