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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hello,

> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -140,6 +140,9 @@ config HAS_EX_TABLE
> >  config HAS_FAST_MULTIPLY
> >     bool
> >
> > +config HAS_GENERIC_CPU_TOPOLOGY
> > +   bool
> 
> You've got indentation right here and ...
> 
> > @@ -191,6 +194,25 @@ config VM_EVENT
> >  config NEEDS_LIBELF
> >     bool
> >
> > +config GENERIC_CPU_TOPOLOGY
> > +   bool
> 
> ... here. Why not ...
> 
> > +config DT_CPU_TOPOLOGY
> > +    bool "Device tree based CPU topology support (UNSUPPORTED)" if 
> > UNSUPPORTED
> > +    depends on HAS_GENERIC_CPU_TOPOLOGY && DEVICE_TREE_PARSE
> > +    select GENERIC_CPU_TOPOLOGY
> > +    help
> > +      Retrieve CPU topology information from the device tree to optimize
> > +      virtual CPU scheduling.
> > +
> > +config ACPI_CPU_TOPOLOGY
> > +    bool "ACPI based CPU topology support (UNSUPPORTED)" if UNSUPPORTED
> > +    depends on HAS_GENERIC_CPU_TOPOLOGY && ACPI
> > +    select GENERIC_CPU_TOPOLOGY
> > +    help
> > +      Retrieve CPU topology information from the ACPI PPTT to optimize
> > +      virtual CPU scheduling.
> 
> ... throughout here?

I will fix it.

> I'm also a little puzzled by the "if UNSUPPORTED" on the prompts. Imo that
> would better be normal "depends on UNSUPPORTED". The situation is
> different

Okay, I will replace it with "depends on UNSUPPORTED"

> in e.g. common/sched/Kconfig, where the default value may be Y (with the
> prompt being invisible making it impossible to turn off the option).
> 
> Also may I suggest s/virtual CPU/vCPU/ ?

Okay.
 
> > --- /dev/null
> > +++ b/xen/common/cpu-topology.c
> > @@ -0,0 +1,65 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <xen/acpi.h>
> > +#include <xen/cpu-topology.h>
> > +#include <xen/cpumask.h>
> > +#include <xen/dt-cpu-topology.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);
> 
> XVFREE(), as it wants to be ...
> 
> > +}
> > +
> > +void __init init_cpu_topology(void)
> > +{
> > +    unsigned int cpu;
> > +    int ret;
> > +
> > +    cpu_topology = xzalloc_array(struct cpu_topology, nr_cpu_ids);
> 
> --- xvzalloc_array() here.

Okay.

> > +    if ( !cpu_topology )
> > +    {
> > +        printk(XENLOG_ERR "Failed to allocate memory for cpu_topology 
> > table\n");
> 
> Is this really an error (irrespective of whether this then also needs
> logging)? There may not be any topology information to retrieve, in which
> case the allocation failure is benign.

Okay, I will remove this line.

> > +        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");
> 
> Same here then obviously. Also please respect line length constraints. Don't
> split format strings, but splitting XENLOG_* off the string literal is fine.

Okay.

> > +#ifdef CONFIG_GENERIC_CPU_TOPOLOGY
> > +struct cpu_topology;
> 
> Why would this be needed?

I will remove the line.

> > +struct cpu_topology *__ro_after_init cpu_topology;
> 
> This includes the same effect.

Okay.

> > +#define INVALID_TOPO_ID (~0U)
> > +
> > +struct cpu_map {
> > +    unsigned int thread_id;
> > +    unsigned int core_id;
> > +    unsigned int cluster_id;
> > +    unsigned int package_id;
> > +};
> > +
> > +static struct cpu_map __initdata cpu_map[NR_CPUS] = {
> > +    [0 ... NR_CPUS - 1] = {INVALID_TOPO_ID, INVALID_TOPO_ID,
> > +                           INVALID_TOPO_ID, INVALID_TOPO_ID}
> 
> I think it would be nice if this properly used designated initializers
> throughout. A trailing comma (twice) would also be nice.

I will replace it with:

static struct cpu_map __initdata cpu_map[NR_CPUS] = {
    [0 ... NR_CPUS - 1] = {
        .thread_id = INVALID_TOPO_ID,
        .core_id = INVALID_TOPO_ID,
        .cluster_id = INVALID_TOPO_ID,
        .package_id = INVALID_TOPO_ID,
    },
};

> > +static struct dt_device_node *__init dt_find_child_node_by_name(
> > +    const struct dt_device_node *dt,
> > +    const char *name)
> > +{
> > +    struct dt_device_node *np;
> 
> Is there a reason this cannot be pointer-to-const?

I will make it const.

> > --- a/xen/drivers/acpi/Makefile
> > +++ b/xen/drivers/acpi/Makefile
> > @@ -10,3 +10,4 @@ obj-$(CONFIG_PM_OP) += pm-op.o
> >
> >  obj-$(CONFIG_X86) += hwregs.o
> >  obj-$(CONFIG_X86) += reboot.o
> > +obj-$(CONFIG_ACPI_CPU_TOPOLOGY) += topology.init.o
> 
> As before, I'd prefer if this was appended to the earlier group of objects.

Okay.

Thank you,
Hirokazu Takahashi.