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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hello,

> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -101,6 +101,16 @@ endchoice
> >
> >  source "arch/Kconfig"
> >
> > +config ARM_CPU_TOPOLOGY
> > +   bool "CPU topology support (UNSUPPORTED)" if UNSUPPORTED
> > +   select CPU_TOPOLOGY
> > +   help
> > +     Retrieve CPU topology information from the device tree to optimize
> > +     virtual CPU scheduling.
> > +
> > +     Note: Implementation for parsing CPU topology from the ACPI PPTT
> > +     is currently missing.
> 
> This option isn't itself used anywhere; it exists solely for its "select"
> effect. Since topology is an arch-independent concept, I'd suggest
> CPU_TOPOLOGY to have the prompt and help text, and there being a per-arch
> HAS_CPU_TOPOLOGY (or maybe HAS_GENERIC_CPU_TOPOLOGY, to avoid the false
> impression that x86 doesn't deal with topology) which CPU_TOPOLOGY then
> depends on.

That is a good idea.
I will use this approach.

> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -188,6 +188,14 @@ config VM_EVENT
> >  config NEEDS_LIBELF
> >     bool
> >
> > +config DT_CPU_TOPOLOGY
> > +   bool
> > +
> > +config CPU_TOPOLOGY
> > +   bool
> > +   select DT_CPU_TOPOLOGY if DEVICE_TREE_PARSE
> > +   select ACPI_CPU_TOPOLOGY if ACPI
> 
> As per part of what I said above, this may also want to be
> GENERIC_CPU_TOPOLOGY
> or some such. Other maintainers' input may be wanted here.

Okay.

> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o
> >  obj-$(CONFIG_HYPFS_CONFIG) += config_data.o
> >  obj-$(CONFIG_CORE_PARKING) += core_parking.o
> >  obj-y += cpu.o
> > +obj-$(CONFIG_CPU_TOPOLOGY) += cpu-topology.o
> 
> As least for now this may want to be
> 
> obj-bin-$(CONFIG_CPU_TOPOLOGY) += cpu-topology.init.o

Okay.

> seeing that ...
> 
> > --- /dev/null
> > +++ b/xen/common/cpu-topology.c
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <xen/acpi.h>
> > +#include <xen/cpu-topology.h>
> > +#include <xen/cpumask.h>
> > +#include <xen/init.h>
> > +
> > +static void __init free_topology_table(void)
> > +{
> > +    unsigned int cpu;
> > +
> > +    for ( cpu = 0; cpu < nr_cpu_ids; cpu++ )
> > +    {
> > +        free_cpumask_var(cpu_topology[cpu].thread_sibling);
> > +        free_cpumask_var(cpu_topology[cpu].core_sibling);
> > +        free_cpumask_var(cpu_topology[cpu].cluster_sibling);
> > +    }
> > +
> > +    XFREE(cpu_topology);
> > +}
> > +
> > +void __init init_cpu_topology(void)
> > +{
> > +    unsigned int cpu;
> > +
> > +    cpu_topology = xzalloc_array(struct cpu_topology, nr_cpu_ids);
> > +    if ( !cpu_topology )
> > +    {
> > +        printk(XENLOG_ERR "Failed to allocate memory for cpu_topology 
> > table\n");
> > +        return;
> > +    }
> > +
> > +    for ( cpu = 0; cpu < nr_cpu_ids; cpu++ )
> > +    {
> > +        if ( !zalloc_cpumask_var(&cpu_topology[cpu].thread_sibling) ||
> > +             !zalloc_cpumask_var(&cpu_topology[cpu].core_sibling) ||
> > +             !zalloc_cpumask_var(&cpu_topology[cpu].cluster_sibling) )
> > +        {
> > +            free_topology_table();
> > +            printk(XENLOG_ERR "Failed to allocate memory for cpu_topology 
> > table\n");
> > +            return;
> > +        }
> > +    }
> > +
> > +    if ( acpi_disabled )
> > +        dt_init_cpu_topology();
> > +    else
> > +        acpi_init_cpu_topology();
> > +}
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> 
> ... there are only __init functions here.
> 
> > --- a/xen/common/device-tree/Makefile
> > +++ b/xen/common/device-tree/Makefile
> > @@ -1,6 +1,7 @@
> >  obj-y += bootfdt.init.o
> >  obj-$(CONFIG_HAS_DEVICE_TREE_DISCOVERY) += bootinfo-fdt.init.o
> >  obj-$(CONFIG_HAS_DEVICE_TREE_DISCOVERY) += bootinfo.init.o
> > +obj-$(CONFIG_DT_CPU_TOPOLOGY) += cpu-topology.o
> 
> Same here, albeit requiring ...

Okay. 

> > --- /dev/null
> > +++ b/xen/common/device-tree/cpu-topology.c
> > @@ -0,0 +1,352 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Derived from Linux kernel 7.0's $drivers/base/arch_topology.c
> > + * Parse cpu topology information.
> > + */
> > +
> > +#include <xen/acpi.h>
> > +#include <xen/cpu-topology.h>
> > +#include <xen/cpumask.h>
> > +#include <xen/device_tree.h>
> > +#include <xen/errno.h>
> > +#include <xen/init.h>
> > +#include <xen/numa.h>
> > +#include <xen/xvmalloc.h>
> > +
> > +struct cpu_map {
> > +    unsigned int thread_id;
> > +    unsigned int core_id;
> > +    unsigned int cluster_id;
> > +    unsigned int package_id;
> > +};
> > +
> > +struct cpu_topology *cpu_topology;
> 
> ... this (which ought to be __ro_after_init anyway) to be moved elsewhere.
> 
> > +static const unsigned int __initdata invalid_topo_id = (~0U);
> 
> Nit: What use are the parentheses here? This may want to be a #define (with
> the identifier all uppercase), in which case the parentheses need keeping.

Okay.

> > +void __init map_cpuid_to_node(unsigned int cpuid,
> > +                              struct dt_device_node *cpu_node)
> > +{
> > +    if ( cpuid < NR_CPUS )
> 
> Better use ARRAY_SIZE() when ...

Okay.

> > +        dt_cpu_table[cpuid] = cpu_node;
> 
> ... an array access is guarded.
> 
> Just for my own understanding: It is deliberate for cpuid >= NR_CPUS to go
> entirely silently here?

I will add printk to report the error.

> As to "cpuid" - please can this be "cpu", as you have it elsewhere. "cpuid"
> is a misleading term here when x86 comes into play.

Okay.

> > +static unsigned int __init cpu_node_to_id(struct dt_device_node *cpu_node)
> > +{
> > +    unsigned int cpu;
> > +    bool found = false;
> 
> Pointless initializer; in fact ...
> 
> > +    for_each_possible_cpu(cpu)
> > +    {
> > +        found = (cpu_node == dt_cpu_table[cpu]);
> 
> ... the declaration could move here, or - better yet - could be omitted
> altogether, as the variable is used ...
> 
> > +        if ( found )
> 
> ... exacly once.

Okay, I will remove this variable.
 
> > +/*
> > + * This function returns the logic cpu number of the node.
> 
> Nit: "logical"? Also "of the node" is misleading (towards NUMA), "of the DT
> node" would be unambiguous.

Okay.
 
> > + */
> > +static unsigned int __init get_cpu_for_node(struct dt_device_node *node)
> 
> Pointer-to-const?

Do you mean this should be replaced with the following line?
static unsigned int __init get_cpu_for_node(struct dt_device_node *const node)
 
> > +{
> > +    struct dt_device_node *cpu_node = dt_parse_phandle(node, "cpu", 0);
> 
> Again? Generally everywhere that it is possible and sensible.

Okay. I will check if other codes have pointers that can be const.

> > +    if ( !cpu_node )
> > +        return invalid_topo_id;
> > +
> > +    return cpu_node_to_id(cpu_node);
> > +}
> > +
> > +static int __init parse_core(struct dt_device_node *core,
> > +                             unsigned int package_id,
> > +                             unsigned int cluster_id,
> > +                             unsigned int core_id)
> > +{
> > +    char name[20];
> 
> Move to the more narrow scope it's used in? (Again please take as a general 
> remark.)

Okay.

> > +    bool leaf = true;
> > +    unsigned int i = 0;
> > +    unsigned int cpu;
> > +
> > +    do {
> > +        struct dt_device_node *t;
> > +
> > +        snprintf(name, sizeof(name), "thread%u", i);
> > +        t = dt_find_child_node_by_name(core, name);
> > +
> > +        if ( !t )
> > +            break;
> > +
> > +        leaf = false;
> > +        cpu = get_cpu_for_node(t);
> > +        if ( cpu != invalid_topo_id )
> > +        {
> > +            cpu_map[cpu].package_id = package_id;
> > +            cpu_map[cpu].cluster_id = cluster_id;
> > +            cpu_map[cpu].core_id = core_id;
> > +            cpu_map[cpu].thread_id = i;
> > +        }
> > +        else
> > +        {
> > +            printk(XENLOG_ERR "ERROR: %pOF: Can't get CPU for thread\n", 
> > t);
> 
> I don't think we support %pOF (just yet).

I will use dt_node_name() instead.
 
> > --- a/xen/drivers/acpi/Makefile
> > +++ b/xen/drivers/acpi/Makefile
> > @@ -10,3 +10,5 @@ obj-$(CONFIG_PM_OP) += pm-op.o
> >
> >  obj-$(CONFIG_X86) += hwregs.o
> >  obj-$(CONFIG_X86) += reboot.o
> > +
> > +obj-$(CONFIG_ACPI_CPU_TOPOLOGY) += topology.o
> 
> See earlier remarks as to object containing only __init code. Also this may
> better be appended (without a blank line) to the earlier block of objects.

Okay.
 
> > --- /dev/null
> > +++ b/xen/include/xen/cpu-topology.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef XEN_CPU_TOPOLOGY_H
> > +#define XEN_CPU_TOPOLOGY_H
> > +
> > +#include <xen/dt-cpu-topology.h>
> > +
> > +struct cpu_topology {
> > +    cpumask_var_t thread_sibling;
> > +    cpumask_var_t core_sibling;
> > +    cpumask_var_t cluster_sibling;
> 
> For these to compile independent of what #include-s occurred earlier in the
> top-level .c file, don't you need to also "#include <xen/cpumask.h>"? Otoh
> I can't quite spot why you #include xen/dt-cpu-topology.h here.

Okay.

> Further shouldn't this move ...
> 
> > +};
> > +
> > +#ifdef CONFIG_CPU_TOPOLOGY
> 
> ... here?

Okay.

> > +extern struct cpu_topology *cpu_topology;
> > +void init_cpu_topology(void);
> > +
> > +#else /* CONFIG_CPU_TOPOLOGY */
> > +
> > +#define cpu_topology ((struct cpu_topology *)NULL)
> 
> Why exactly is this needed? It very much looks as if its presence may hide 
> bugs.

In the current code, the condition if ( cpu_topology ) is used to cover both 
cases:
when CONFIG_CPU_TOPOLOGY is disabled, and when memory allocation for
topology table fails. 

The next patch uses if ( cpu_topology ).

static int setup_cpu_sibling_map(int cpu)
{
    if ( !zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) ||
         !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) )
        return -ENOMEM;

    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
    {
        /*
         * If CONFIG_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);
    }

    return 0;
}

> > --- /dev/null
> > +++ b/xen/include/xen/dt-cpu-topology.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef XEN_DT_CPU_TOPOLOGY_H
> > +#define XEN_DT_CPU_TOPOLOGY_H
> > +
> > +#include <xen/device_tree.h>
> 
> Again I can't quite see why this would be needed. You need a forward-decl of 
> ...
> 
> > +#ifdef CONFIG_DT_CPU_TOPOLOGY
> > +
> > +void map_cpuid_to_node(unsigned int cpuid, struct dt_device_node 
> > *cpu_node);
> 
> ... struct dt_device_node, yes, but that's all.

Okay.

Thank you,
Hirokazu Takahashi.