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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.07.2026 11:15, 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.
> 
> Signed-off-by: Hirokazu Takahashi <taka@xxxxxxxxxxxxx>
> ---
> Changes in v4
>  - Explicitly #include <asm/processor.h> in cpu-topology.h to guarantee
>    that arch-specific definitions of cpu_to_core() and cpu_to_socket()
>    take precedence over the generic fallbacks.
>  - Introduce inline initialization functions for cpu_sibling_mask and
>    cpu_core_mask in cpu-topology.h, providing separate variants for both
>    when CONFIG_GENERIC_CPU_TOPOLOGY is enabled and disabled.
> 
> Changes in v3
>  - Remove the temporary definitions of cpu_to_core() and cpu_to_socket()
>    from RISC-V and PPC processor.h.
>  - Minimize the use of #ifdef blocks, leveraging compiler Dead Code
>    Elimination (DCE) where possible.
> 
>  xen/arch/arm/include/asm/processor.h   |  4 --
>  xen/arch/arm/smpboot.c                 |  8 +---
>  xen/arch/ppc/include/asm/processor.h   |  4 --
>  xen/arch/riscv/include/asm/processor.h |  4 --
>  xen/common/device-tree/cpu-topology.c  | 51 ++++++++++++++++++++++++++
>  xen/common/sched/credit2.c             |  3 ++
>  xen/common/sysctl.c                    |  1 +
>  xen/drivers/acpi/topology.c            |  3 ++
>  xen/include/xen/cpu-topology.h         | 45 ++++++++++++++++++++++-
>  9 files changed, 103 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/processor.h 
> b/xen/arch/arm/include/asm/processor.h
> index a3753c317f..41fa73cfc4 100644
> --- a/xen/arch/arm/include/asm/processor.h
> +++ b/xen/arch/arm/include/asm/processor.h
> @@ -613,10 +613,6 @@ void show_stack(const struct cpu_user_regs *regs);
>  
>  #define cpu_relax() barrier() /* Could yield? */
>  
> -/* All a bit UP for the moment */
> -#define cpu_to_core(_cpu)   (0)
> -#define cpu_to_socket(_cpu) (0)
> -
>  struct vcpu;
>  void vcpu_regs_hyp_to_user(const struct vcpu *vcpu,
>                             struct vcpu_guest_core_regs *regs);
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 5ce2bcf6ec..3c9f2a5c53 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -92,13 +92,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);
>  
>      return 0;
>  }
> diff --git a/xen/arch/ppc/include/asm/processor.h 
> b/xen/arch/ppc/include/asm/processor.h
> index 242346cab9..1bf6f6c66c 100644
> --- a/xen/arch/ppc/include/asm/processor.h
> +++ b/xen/arch/ppc/include/asm/processor.h
> @@ -141,10 +141,6 @@
>  /* Macro to adjust thread priority for hardware multithreading */
>  #define HMT_very_low()  asm volatile ( "or %r31, %r31, %r31" )
>  
> -/* TODO: This isn't correct */
> -#define cpu_to_core(cpu)   (0)
> -#define cpu_to_socket(cpu) (0)
> -
>  /*
>   * User-accessible registers: most of these need to be saved/restored
>   * for every nested Xen invocation.
> diff --git a/xen/arch/riscv/include/asm/processor.h 
> b/xen/arch/riscv/include/asm/processor.h
> index 6b89df4a2d..d478ffb76b 100644
> --- a/xen/arch/riscv/include/asm/processor.h
> +++ b/xen/arch/riscv/include/asm/processor.h
> @@ -54,10 +54,6 @@ struct cpu_user_regs
>      unsigned long pregs;
>  };
>  
> -/* TODO: need to implement */
> -#define cpu_to_core(cpu)   0
> -#define cpu_to_socket(cpu) 0
> -
>  static inline void cpu_relax(void)
>  {
>  #ifdef __riscv_zihintpause
> diff --git a/xen/common/device-tree/cpu-topology.c 
> b/xen/common/device-tree/cpu-topology.c
> index b653227ef4..43322a153f 100644
> --- a/xen/common/device-tree/cpu-topology.c
> +++ b/xen/common/device-tree/cpu-topology.c
> @@ -330,6 +330,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 = 0;
> +    unsigned int next_cluster_id = 0;
> +    unsigned int next_socket_id = 0;
> +
> +    for_each_possible_cpu(cpu)
> +    {
> +        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;

Not even an assertion to make sure first_cpu is within bounds, i.e.
topo->thread_sibling isn't (by mistake) empty? (Same again further down.)

> +        /* Reuse the calculated core id if clustering is not supported */
> +        if ( cpumask_empty(topo->cluster_sibling) )
> +            topo->phys_cluster_id = topo->phys_core_id;

Wouldn't the cluster better be uniformly the same value (perhaps 0) for
all CPUs when there's no clustering?

> +        else
> +        {
> +            first_cpu = cpumask_first(topo->cluster_sibling);

No need to use cpumask_empty() and cpumask_first(). The return value of the
latter allows to identify the "empty" case.

> --- a/xen/common/sched/credit2.c
> +++ b/xen/common/sched/credit2.c
> @@ -9,6 +9,7 @@
>   * Based on an earlier verson by Emmanuel Ackaouy.
>   */
>  
> +#include <xen/cpu-topology.h>
>  #include <xen/errno.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
> @@ -37,6 +38,8 @@ static unsigned int cpu_nr_siblings(unsigned int cpu)
>  {
>  #ifdef CONFIG_X86
>      return cpu_data[cpu].x86_num_siblings;
> +#elif defined(CONFIG_CPU_TOPOLOGY)
> +    return cpu_topology ? cpu_topology[cpu].num_siblings : 1;

Apart from the naming issue I continue to think that the generic case should
come first, and x86'es special case second. Yet then I'm not a maintainer of
this code ...

> --- a/xen/include/xen/cpu-topology.h
> +++ b/xen/include/xen/cpu-topology.h
> @@ -4,6 +4,9 @@
>  #define XEN_CPU_TOPOLOGY_H
>  
>  #include <xen/cpumask.h>
> +#include <xen/percpu.h>
> +#include <asm/processor.h>
> +#include <asm/smp.h>

Blank line please between the xen/ group and the asm/ one.

> @@ -11,16 +14,56 @@ 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
> +    {
> +        cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
> +        cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map);

Isn't this the same as ...

> +    }
> +}
> +
> +#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 */
>  
> -#define cpu_topology ((struct cpu_topology *)NULL)
>  static inline void init_cpu_topology(void) {}
>  
> +static inline void init_cpu_sibling_map(unsigned int cpu)
> +{
> +    /*
> +     * If CONFIG_GENERIC_CPU_TOPOLOGY is disabled, it is assumed that
> +     * all CPUs reside in the same socket and that SMT is not used.
> +     */
> +    cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
> +    cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map);

... this? Would be nice to avoid the duplication.

Jan