Re: [PATCH v2 4/6] xen/cpupool: Create different cpupools at boot time

[Juergen Gross <jgross@xxxxxxxx>]

On 11.03.22 09:56, Luca Fancellu wrote:
> Hi Juergen,
>
> Thanks for your review
>
> > On 11 Mar 2022, at 08:09, Juergen Gross <jgross@xxxxxxxx> wrote:
> >
> > On 10.03.22 18:10, Luca Fancellu wrote:
> > > Introduce a way to create different cpupools at boot time, this is
> > > particularly useful on ARM big.LITTLE system where there might be the
> > > need to have different cpupools for each type of core, but also
> > > systems using NUMA can have different cpu pools for each node.
> > > The feature on arm relies on a specification of the cpupools from the
> > > device tree to build pools and assign cpus to them.
> > > Documentation is created to explain the feature.
> > > Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > > ---
> > > Changes in v2:
> > > - Move feature to common code (Juergen)
> > > - Try to decouple dtb parse and cpupool creation to allow
> > >    more way to specify cpupools (for example command line)
> > > - Created standalone dt node for the scheduler so it can
> > >    be used in future work to set scheduler specific
> > >    parameters
> > > - Use only auto generated ids for cpupools
> > > ---
> > >   docs/misc/arm/device-tree/cpupools.txt | 156 ++++++++++++++++++
> > >   xen/common/Kconfig                     |   8 +
> > >   xen/common/Makefile                    |   1 +
> > >   xen/common/boot_cpupools.c             | 212 +++++++++++++++++++++++++
> > >   xen/common/sched/cpupool.c             |   6 +-
> > >   xen/include/xen/sched.h                |  19 +++
> > >   6 files changed, 401 insertions(+), 1 deletion(-)
> > >   create mode 100644 docs/misc/arm/device-tree/cpupools.txt
> > >   create mode 100644 xen/common/boot_cpupools.c
> > > diff --git a/docs/misc/arm/device-tree/cpupools.txt 
> > > b/docs/misc/arm/device-tree/cpupools.txt
> > > new file mode 100644
> > > index 000000000000..d5a82ed0d45a
> > > --- /dev/null
> > > +++ b/docs/misc/arm/device-tree/cpupools.txt
> > > @@ -0,0 +1,156 @@
> > > +Boot time cpupools
> > > +==================
> > > +
> > > +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible to
> > > +create cpupools during boot phase by specifying them in the device tree.
> > > +
> > > +Cpupools specification nodes shall be direct childs of /chosen node.
> > > +Each cpupool node contains the following properties:
> > > +
> > > +- compatible (mandatory)
> > > +
> > > +    Must always include the compatiblity string: "xen,cpupool".
> > > +
> > > +- cpupool-cpus (mandatory)
> > > +
> > > +    Must be a list of device tree phandle to nodes describing cpus (e.g. having
> > > +    device_type = "cpu"), it can't be empty.
> > > +
> > > +- cpupool-sched (optional)
> > > +
> > > +    Must be a device tree phandle to a node having "xen,scheduler" compatible
> > > +    (description below), it has no effect when the cpupool refers to the 
> > > cpupool
> > > +    number zero, in that case the default Xen scheduler is selected 
> > > (sched=<...>
> > > +    boot argument).
> > > +
> > > +
> > > +A scheduler specification node is a device tree node that contains the 
> > > following
> > > +properties:
> > > +
> > > +- compatible (mandatory)
> > > +
> > > +    Must always include the compatiblity string: "xen,scheduler".
> > > +
> > > +- sched-name (mandatory)
> > > +
> > > +    Must be a string having the name of a Xen scheduler, check the sched=<...>
> > > +    boot argument for allowed values.
> > > +
> > > +
> > > +Constraints
> > > +===========
> > > +
> > > +If no cpupools are specified, all cpus will be assigned to one cpupool
> > > +implicitly created (Pool-0).
> > > +
> > > +If cpupools node are specified, but not every cpu brought up by Xen is 
> > > assigned,
> > > +all the not assigned cpu will be assigned to an additional cpupool.
> > > +
> > > +If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen will
> > > +stop.
> > > +
> > > +
> > > +Examples
> > > +========
> > > +
> > > +A system having two types of core, the following device tree specification will
> > > +instruct Xen to have two cpupools:
> > > +
> > > +- The cpupool with id 0 will have 4 cpus assigned.
> > > +- The cpupool with id 1 will have 2 cpus assigned.
> > > +
> > > +The following example can work only if hmp-unsafe=1 is passed to Xen boot
> > > +arguments, otherwise not all cores will be brought up by Xen and the cpupool
> > > +creation process will stop Xen.
> > > +
> > > +
> > > +a72_1: cpu@0 {
> > > +        compatible = "arm,cortex-a72";
> > > +        reg = <0x0 0x0>;
> > > +        device_type = "cpu";
> > > +        [...]
> > > +};
> > > +
> > > +a72_2: cpu@1 {
> > > +        compatible = "arm,cortex-a72";
> > > +        reg = <0x0 0x1>;
> > > +        device_type = "cpu";
> > > +        [...]
> > > +};
> > > +
> > > +a53_1: cpu@100 {
> > > +        compatible = "arm,cortex-a53";
> > > +        reg = <0x0 0x100>;
> > > +        device_type = "cpu";
> > > +        [...]
> > > +};
> > > +
> > > +a53_2: cpu@101 {
> > > +        compatible = "arm,cortex-a53";
> > > +        reg = <0x0 0x101>;
> > > +        device_type = "cpu";
> > > +        [...]
> > > +};
> > > +
> > > +a53_3: cpu@102 {
> > > +        compatible = "arm,cortex-a53";
> > > +        reg = <0x0 0x102>;
> > > +        device_type = "cpu";
> > > +        [...]
> > > +};
> > > +
> > > +a53_4: cpu@103 {
> > > +        compatible = "arm,cortex-a53";
> > > +        reg = <0x0 0x103>;
> > > +        device_type = "cpu";
> > > +        [...]
> > > +};
> > > +
> > > +chosen {
> > > +
> > > +    sched: sched_a {
> > > +        compatible = "xen,scheduler";
> > > +        sched-name = "credit2";
> > > +    };
> > > +    cpupool_a {
> > > +        compatible = "xen,cpupool";
> > > +        cpupool-cpus = <&a53_1 &a53_2 &a53_3 &a53_4>;
> > > +    };
> > > +    cpupool_b {
> > > +        compatible = "xen,cpupool";
> > > +        cpupool-cpus = <&a72_1 &a72_2>;
> > > +        cpupool-sched = <&sched>;
> > > +    };
> > > +
> > > +    [...]
> > > +
> > > +};
> > > +
> > > +
> > > +A system having the cpupools specification below will instruct Xen to have 
> > > three
> > > +cpupools:
> > > +
> > > +- The cpupool Pool-0 will have 2 cpus assigned.
> > > +- The cpupool Pool-1 will have 2 cpus assigned.
> > > +- The cpupool Pool-2 will have 2 cpus assigned (created by Xen with all the not
> > > +  assigned cpus a53_3 and a53_4).
> > > +
> > > +chosen {
> > > +
> > > +    sched: sched_a {
> > > +        compatible = "xen,scheduler";
> > > +        sched-name = "null";
> > > +    };
> > > +    cpupool_a {
> > > +        compatible = "xen,cpupool";
> > > +        cpupool-cpus = <&a53_1 &a53_2>;
> > > +    };
> > > +    cpupool_b {
> > > +        compatible = "xen,cpupool";
> > > +        cpupool-cpus = <&a72_1 &a72_2>;
> > > +        cpupool-sched = <&sched>;
> > > +    };
> > > +
> > > +    [...]
> > > +
> > > +};
> > > \ No newline at end of file
> > > diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> > > index 64439438891c..dc9eed31682f 100644
> > > --- a/xen/common/Kconfig
> > > +++ b/xen/common/Kconfig
> > > @@ -22,6 +22,14 @@ config GRANT_TABLE
> > >           If unsure, say Y.
> > >   +config BOOT_TIME_CPUPOOLS
> > > +       bool "Create cpupools at boot time"
> > > +       depends on HAS_DEVICE_TREE
> > > +       default n
> > > +       help
> > > +         Creates cpupools during boot time and assigns cpus to them. Cpupools
> > > +         options can be specified in the device tree.
> > > +
> > >   config ALTERNATIVE_CALL
> > >         bool
> > >   diff --git a/xen/common/Makefile b/xen/common/Makefile
> > > index dc8d3a13f5b8..c5949785ab28 100644
> > > --- a/xen/common/Makefile
> > > +++ b/xen/common/Makefile
> > > @@ -1,5 +1,6 @@
> > >   obj-$(CONFIG_ARGO) += argo.o
> > >   obj-y += bitmap.o
> > > +obj-$(CONFIG_BOOT_TIME_CPUPOOLS) += boot_cpupools.o
> > >   obj-$(CONFIG_HYPFS_CONFIG) += config_data.o
> > >   obj-$(CONFIG_CORE_PARKING) += core_parking.o
> > >   obj-y += cpu.o
> > > diff --git a/xen/common/boot_cpupools.c b/xen/common/boot_cpupools.c
> > > new file mode 100644
> > > index 000000000000..e8529a902d21
> > > --- /dev/null
> > > +++ b/xen/common/boot_cpupools.c
> > > @@ -0,0 +1,212 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * xen/common/boot_cpupools.c
> > > + *
> > > + * Code to create cpupools at boot time for arm architecture.
> > Please drop the arm reference here.
> >
> > > + *
> > > + * Copyright (C) 2022 Arm Ltd.
> > > + */
> > > +
> > > +#include <xen/sched.h>
> > > +
> > > +#define BTCPUPOOLS_DT_NODE_NO_REG     (-1)
> > > +#define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2)
> > Move those inside the #ifdef below, please
> >
> > > +
> > > +struct pool_map {
> > > +    int pool_id;
> > > +    int sched_id;
> > > +    struct cpupool *pool;
> > > +};
> > > +
> > > +static struct pool_map __initdata pool_cpu_map[NR_CPUS] =
> > > +    { [0 ... NR_CPUS-1] = {.pool_id = -1, .sched_id = -1, .pool = NULL} };
> > > +static unsigned int __initdata next_pool_id;
> > > +
> > > +#ifdef CONFIG_ARM
> > Shouldn't this be CONFIG_HAS_DEVICE_TREE?
> Yes, the only problem is that in get_logical_cpu_from_hw_id I use the arm 
> specific
> cpu_logical_map(…), so what do you think it’s the better way here?
> Do you think I should have everything under CONFIG_HAS_DEVICE_TREE
> and get_logical_cpu_from_hw_id under CONFIG_ARM like in this way below?
Hmm, what is the hwid used for on Arm? I guess this could be similar
to the x86 acpi-id?

So I'd rather put get_logical_cpu_from_hw_id() into Arm specific code
and add a related x86 function to x86 code. Depending on the answer to
above question this could either be get_cpu_id(), or maybe an identity
function.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature