RE: [PATCH v10 1/4] xen/device-tree: Parse 'cpu-map' node for CPU topology exploration

[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 1806c47a08..904130efdf 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -9,10 +9,12 @@
> >
> >  #include <xen/acpi.h>
> >  #include <xen/cpu.h>
> > +#include <xen/cpu-topology.h>
> >  #include <xen/cpumask.h>
> >  #include <xen/delay.h>
> >  #include <xen/device_tree.h>
> >  #include <xen/domain_page.h>
> > +#include <xen/dt-cpu-topology.h>
> >  #include <xen/errno.h>
> >  #include <xen/init.h>
> >  #include <xen/mm.h>
> > @@ -244,6 +246,9 @@ static void __init dt_smp_init_cpus(void)
> >          }
> >          else
> >              tmp_map[i] = hwid;
> > +
> > +        /* Pass the info to dt_init_cpu_topology() */
> > +        map_cpu_to_dt_node(i, cpu);
> This should be moved to the above else branch which is taken on a success 
> only.

Okay.

> > +config GENERIC_CPU_TOPOLOGY
> > +   bool
> > +
> > +config DT_CPU_TOPOLOGY
> > +   bool "Device tree based CPU topology support (UNSUPPORTED)"
> Can you please explain why unsupported?

I marked it as UNSUPPORTED simply because it is a new feature. However, if 
possible,
I would prefer to drop UNSUPPORTED so that everyone can make use of it.

> > +   depends on HAS_GENERIC_CPU_TOPOLOGY && DEVICE_TREE_PARSE && UNSUPPORTED
> > +   select GENERIC_CPU_TOPOLOGY
> > +   help
> > +     Retrieve CPU topology information from the device tree to optimize
> > +     vCPU scheduling.
> > +
> > +config ACPI_CPU_TOPOLOGY
> > +   bool "ACPI based CPU topology support (UNSUPPORTED)"
> > +   depends on HAS_GENERIC_CPU_TOPOLOGY && ACPI && UNSUPPORTED
> > +   select GENERIC_CPU_TOPOLOGY
> > +   help
> > +     Retrieve CPU topology information from the ACPI PPTT to optimize
> > +     vCPU scheduling.
> > +

> > +void __init init_cpu_topology(void)
> > +{
> > +    unsigned int cpu;
> > +    int ret;
> > +
> > +    cpu_topology = xvzalloc_array(struct cpu_topology, nr_cpu_ids);
> > +    if ( !cpu_topology )
> > +        return;
> I don't think this is ok for this function not to let the caller about the
> error. That's especially important for failures from actual DT/ACPI topology
> initialization. You don't even print anything.

I will add an error message when memory allocation fails.

> > +static void __init setup_siblings_masks(unsigned int target_cpu)
> > +{
> > +    const struct cpu_topology *target_topo = &cpu_topology[target_cpu];
> > +    const struct cpu_map *target_map = &cpu_map[target_cpu];
> > +    unsigned int cpu;
> > +
> > +    /* Update cluster, core and thread sibling masks */
> > +    for_each_possible_cpu(cpu)
> > +    {
> > +        const struct cpu_topology *cpu_topo = &cpu_topology[cpu];
> > +        const struct cpu_map *map = &cpu_map[cpu];
> > +
> > +        if ( target_cpu > cpu )
> > +            continue;
> > +
> > +        if ( target_map->socket_id != map->socket_id )
> > +            continue;
> > +
> > +        cpumask_set_cpu(target_cpu, cpu_topo->core_sibling);
> > +        cpumask_set_cpu(cpu, target_topo->core_sibling);
> This does not compile.

I will fix this. In my local environment, NR_CPUS was set to a large value 
where the bitmap area
becomes an indirect pointer, so I didn't catch this compilation issue.

> > +
> > +        if ( target_map->cluster_id != map->cluster_id )
> > +            continue;
> > +
> > +        cpumask_set_cpu(target_cpu, cpu_topo->cluster_sibling);
> > +        cpumask_set_cpu(cpu, target_topo->cluster_sibling);
> > +
> > +        if ( target_map->core_id != map->core_id )
> > +            continue;
> > +
> > +        cpumask_set_cpu(target_cpu, cpu_topo->thread_sibling);
> > +        cpumask_set_cpu(cpu, target_topo->thread_sibling);
> > +    }
> > +}
> > +
> > +static const struct dt_device_node *__init dt_find_child_node_by_name(
> > +    const struct dt_device_node *dt,
> > +    const char *name)
> > +{
> > +    const struct dt_device_node *np;
> > +
> > +    dt_for_each_child_node(dt, np)
> > +        if ( np->name && (dt_node_cmp(np->name, name) == 0) )
> > +            return np;
> Please use dt_node_name_is_equal() here.

Okay.


> > +
> > +static bool __init is_cpu_map_empty( unsigned int cpu )
> Stray space at the end "cpu )"

Okay.

> > +{
> > +    return (cpu_map[cpu].socket_id == INVALID_TOPO_ID) &&
> > +           (cpu_map[cpu].cluster_id == INVALID_TOPO_ID) &&
> > +           (cpu_map[cpu].core_id == INVALID_TOPO_ID) &&
> > +           (cpu_map[cpu].thread_id == INVALID_TOPO_ID);
> > +}
> > +
> > +static int __init parse_core(const struct dt_device_node *core,
> > +                             unsigned int socket_id,
> > +                             unsigned int cluster_id,
> > +                             unsigned int core_id)
> > +{
> > +    bool leaf = true;
> > +    unsigned int thread_id;
> > +    unsigned int cpu;
> > +
> > +    for ( thread_id = 0; ; thread_id++ )
> > +    {
> > +        const struct dt_device_node *thread;
> > +        char name[20];
> > +
> > +        snprintf(name, sizeof(name), "thread%u", thread_id);
> > +        thread = dt_find_child_node_by_name(core, name);
> > +
> > +        if ( !thread )
> > +            break;
> > +
> > +        leaf = false;
> > +        cpu = get_cpu_for_node(thread);
> > +
> > +        if ( cpu == INVALID_TOPO_ID )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "ERROR: %s: Can't get CPU for thread\n", 
> > dt_node_name(thread));
> No need for the ERROR/WARNING prefixes. You already use correct xenlog levels.

Okay.


> > +static int __init parse_cluster(const struct dt_device_node *cluster,
> > +                                unsigned int socket_id,
> > +                                unsigned int cluster_id)
> > +{
> > +    bool has_cores = false;
> > +    int ret = 0;
> > +
> > +    if ( dt_find_child_node_by_name(cluster, "cluster0") )
> > +    {
> > +        printk(XENLOG_WARNING
> XENLOG_ERROR?

Okay.

> > +               "WARNING: Topology for clusters of clusters not yet 
> > supported\n");
> > +        return -EINVAL;
> > +    }
> > +

> > +static int __init parse_dt_topology(void)
> > +{
> > +    const struct dt_device_node *cpus;
> > +    const struct dt_device_node *map;
> > +
> > +    cpus = dt_find_node_by_path("/cpus");
> > +    if ( !cpus )
> > +        return -ENOENT;
> > +
> > +    map = dt_find_child_node_by_name(cpus, "cpu-map");
> > +    if ( !map )
> > +        return -ENOENT;
> > +
> > +    return parse_package(map);
> Linux ends this function with for_each_possible_cpu(). What's the reason for
> dropping it for our case?

In the Linux kernel, parse_dt_topology() checks at the end that no CPU's 
package_id remains uninitialized.
In the current Xen CPU topology patch, I modified this logic as follows:
 - Renamed Linux's package_id to socket_id.
 - If there is no socket node definition in the Device Tree cpu-map node, all 
CPUs are assumed to reside on
  socket 0 (socket_id = 0). As a result, socket_id will no longer remain -1 
(INVALID_TOPO_ID).

If you think an explicit error check is still needed here, would adding an 
ASSERT() or BUG_ON() to verify
that no CPU has socket_id == INVALID_TOPO_ID be the preferred approach?

+
> > diff --git a/xen/drivers/acpi/topology.c b/xen/drivers/acpi/topology.c
> > new file mode 100644
> > index 0000000000..090c793f33
> > --- /dev/null
> > +++ b/xen/drivers/acpi/topology.c
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <xen/acpi.h>
> > +#include <xen/cpu-topology.h>
> > +#include <xen/cpumask.h>
> > +#include <xen/init.h>
> > +
> > +/*
> > + * TODO: Populate the topology information by scanning the ACPI
> > + *       PPTT (Processor Properties Topology Table).
> > + */
> > +int __init acpi_init_cpu_topology(void)
> > +{
> > +    unsigned int cpu;
> The correspondong DT function has two ASSERTs. Why the divergence here? At 
> least
> the one for cpu_topology existing is a good one (I don't consider the first
> ASSERT very useful).

Okay.

> > diff --git a/xen/include/xen/cpu-topology.h
> b/xen/include/xen/cpu-topology.h
> > new file mode 100644
> > index 0000000000..7cfe3752cd
> > --- /dev/null
> > +++ b/xen/include/xen/cpu-topology.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef XEN_CPU_TOPOLOGY_H
> > +#define XEN_CPU_TOPOLOGY_H
> > +
> > +#include <xen/cpumask.h>
> You can move it under #ifdef where you actually use these types.

okay.

> > --- /dev/null
> > +++ b/xen/include/xen/dt-cpu-topology.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef XEN_DT_CPU_TOPOLOGY_H
> > +#define XEN_DT_CPU_TOPOLOGY_H
> > +
> > +#include <xen/errno.h>
> You can move it under #else which is where you actually use the errno code.

Okay.

Thank you,
Hirokazu Takahashi.