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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 19-Oct-26 03:57, 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>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: Juergen Gross <jgross@xxxxxxxx> # scheduler part
> ---
>  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  | 65 ++++++++++++++++++++++++++
>  xen/common/sched/credit2.c             |  6 +++
>  xen/common/sysctl.c                    |  1 +
>  xen/drivers/acpi/topology.c            |  3 ++
>  xen/include/xen/cpu-topology.h         | 41 +++++++++++++++-
>  9 files changed, 115 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/processor.h 
> b/xen/arch/arm/include/asm/processor.h
> index 8ee8f88fb4..fe4125cd75 100644
> --- a/xen/arch/arm/include/asm/processor.h
> +++ b/xen/arch/arm/include/asm/processor.h
> @@ -614,10 +614,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 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.

>  
>      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 b1745c1071..c5340a924f 100644
> --- a/xen/arch/riscv/include/asm/processor.h
> +++ b/xen/arch/riscv/include/asm/processor.h
> @@ -52,10 +52,6 @@ struct cpu_user_regs
>      unsigned long hstatus;
>  };
>  
> -/* 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 bfcf153cc4..b7b1acc73d 100644
> --- a/xen/common/device-tree/cpu-topology.c
> +++ b/xen/common/device-tree/cpu-topology.c
> @@ -344,6 +344,67 @@ static int __init parse_dt_topology(void)
>      return parse_package(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);
> +        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.

> +
> +        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.

> +        }
> +
> +        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.

> +
> +        topo->num_siblings = cpumask_weight(topo->thread_sibling);
> +    }
> +}
> +
>  int __init dt_init_cpu_topology(void)
>  {
>      unsigned int cpu;
> @@ -353,9 +414,13 @@ int __init dt_init_cpu_topology(void)
>  
>      ret = parse_dt_topology();
>      if ( ret == 0 )
> +    {
>          for_each_possible_cpu(cpu)
>              setup_siblings_masks(cpu);
>  
> +        setup_cpu_topology_ids();
> +    }
> +
>      return ret;
>  }
>  
> diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
> index c32e753bb4..335cadb8b1 100644
> --- 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>
> @@ -35,6 +36,11 @@
>   */
>  static unsigned int cpu_nr_siblings(unsigned int cpu)
>  {
> +#ifdef CONFIG_GENERIC_CPU_TOPOLOGY
> +    if ( cpu_topology )
> +        return cpu_topology[cpu].num_siblings;
> +#endif
> +
>  #ifdef CONFIG_X86
>      return cpu_data[cpu].x86_num_siblings;
>  #else
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 8fb5ff0af3..6f1839b359 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -28,6 +28,7 @@
>  #include <xen/pmstat.h>
>  #include <xen/livepatch.h>
>  #include <xen/coverage.h>
> +#include <xen/cpu-topology.h>
>  
>  long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>  {
> diff --git a/xen/drivers/acpi/topology.c b/xen/drivers/acpi/topology.c
> index f9836447b2..32f5b97c3e 100644
> --- a/xen/drivers/acpi/topology.c
> +++ b/xen/drivers/acpi/topology.c
> @@ -24,6 +24,9 @@ int __init acpi_init_cpu_topology(void)
>      {
>          struct cpu_topology *topo = &cpu_topology[cpu];
>  
> +        topo->phys_core_id = cpu;
> +        topo->num_siblings = 1;
> +
>          cpumask_set_cpu(cpu, topo->thread_sibling);
>          cpumask_copy(topo->core_sibling, &cpu_possible_map);
>          cpumask_copy(topo->cluster_sibling, &cpu_possible_map);
> diff --git a/xen/include/xen/cpu-topology.h b/xen/include/xen/cpu-topology.h
> index 27b9bd443e..52ee93d4d0 100644
> --- a/xen/include/xen/cpu-topology.h
> +++ b/xen/include/xen/cpu-topology.h
> @@ -3,23 +3,60 @@
>  #ifndef XEN_CPU_TOPOLOGY_H
>  #define XEN_CPU_TOPOLOGY_H
>  
> -#ifdef CONFIG_GENERIC_CPU_TOPOLOGY
> -
>  #include <xen/cpumask.h>
> +#include <xen/percpu.h>
> +
> +#include <asm/processor.h>
> +#include <asm/smp.h>
>  
>  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?

> +    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.

~Michal