| Message ID | 20220318152541.7460-5-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot time cpupools | expand |
Hi Luca, I only skimmed through the series. I have one question below: On 18/03/2022 15:25, Luca Fancellu wrote: > +void __init btcpupools_allocate_pools(void) > +{ > + unsigned int i; > + bool add_extra_cpupool = false; > + > + /* > + * If there are no cpupools, the value of next_pool_id is zero, so the code > + * below will assign every cpu to cpupool0 as the default behavior. > + * When there are cpupools, the code below is assigning all the not > + * assigned cpu to a new pool (next_pool_id value is the last id + 1). > + * In the same loop we check if there is any assigned cpu that is not > + * online. > + */ > + for ( i = 0; i < nr_cpu_ids; i++ ) > + if ( cpumask_test_cpu(i, &cpu_online_map) ) > + { > + /* Unassigned cpu gets next_pool_id pool id value */ > + if ( pool_cpu_map[i] < 0 ) > + { > + pool_cpu_map[i] = next_pool_id; > + add_extra_cpupool = true; > + } > + printk(XENLOG_INFO "Logical CPU %u in Pool-%u.\n", i, > + pool_cpu_map[i]); > + } > + else > + { > + if ( pool_cpu_map[i] >= 0 ) > + panic("Pool-%d contains cpu%u that is not online!\n", > + pool_cpu_map[i], i); > + } > + > + if ( add_extra_cpupool ) > + next_pool_id++; > + > + /* Create cpupools with selected schedulers */ > + for ( i = 0; i < next_pool_id; i++ ) > + cpupool_create_pool(i, pool_sched_map[i]); > + > +#ifdef CONFIG_X86 > + /* Cpu0 must be in cpupool0 for x86 */ > + if ( pool_cpu_map[0] != 0 ) > + panic("Cpu0 must be in Pool-0\n"); > +#endif Can you document why this is necessary on x86 but not on other architectures? Cheers,
On 18.03.2022 16:25, Luca Fancellu wrote: > --- 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 By the looks of it you appear to want to specify boot_cpupools.init.o here: All functions there are __init and all data is __initdata. That was string literals (e.g. as used for printk() invocations) will also move to .init.*. > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -1176,6 +1176,25 @@ extern void cf_check dump_runq(unsigned char key); > > void arch_do_physinfo(struct xen_sysctl_physinfo *pi); > > +#ifdef CONFIG_BOOT_TIME_CPUPOOLS > +void btcpupools_allocate_pools(void); > +unsigned int btcpupools_get_cpupool_id(unsigned int cpu); > + > +#ifdef CONFIG_HAS_DEVICE_TREE > +void btcpupools_dtb_parse(void); > +#else > +static inline void btcpupools_dtb_parse(void) {} I think you want to avoid having two stubs for this - one here and ... > +#endif > + > +#else /* !CONFIG_BOOT_TIME_CPUPOOLS */ > +static inline void btcpupools_allocate_pools(void) {} > +static inline void btcpupools_dtb_parse(void) {} ... another one here. Jan
> On 21 Mar 2022, at 09:10, Jan Beulich <jbeulich@suse.com> wrote: > > On 18.03.2022 16:25, Luca Fancellu wrote: >> --- 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 > > By the looks of it you appear to want to specify boot_cpupools.init.o > here: All functions there are __init and all data is __initdata. That > was string literals (e.g. as used for printk() invocations) will also > move to .init.*. > >> --- a/xen/include/xen/sched.h >> +++ b/xen/include/xen/sched.h >> @@ -1176,6 +1176,25 @@ extern void cf_check dump_runq(unsigned char key); >> >> void arch_do_physinfo(struct xen_sysctl_physinfo *pi); >> >> +#ifdef CONFIG_BOOT_TIME_CPUPOOLS >> +void btcpupools_allocate_pools(void); >> +unsigned int btcpupools_get_cpupool_id(unsigned int cpu); >> + >> +#ifdef CONFIG_HAS_DEVICE_TREE >> +void btcpupools_dtb_parse(void); >> +#else >> +static inline void btcpupools_dtb_parse(void) {} > > I think you want to avoid having two stubs for this - one here and ... > >> +#endif >> + >> +#else /* !CONFIG_BOOT_TIME_CPUPOOLS */ >> +static inline void btcpupools_allocate_pools(void) {} >> +static inline void btcpupools_dtb_parse(void) {} > > ... another one here. > Hi Jan, Thank you for your review, yes I will fix your findings in the next serie. Cheers, Luca > Jan > >
> On 18 Mar 2022, at 16:12, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > I only skimmed through the series. I have one question below: > > On 18/03/2022 15:25, Luca Fancellu wrote: >> +void __init btcpupools_allocate_pools(void) >> +{ >> + unsigned int i; >> + bool add_extra_cpupool = false; >> + >> + /* >> + * If there are no cpupools, the value of next_pool_id is zero, so the code >> + * below will assign every cpu to cpupool0 as the default behavior. >> + * When there are cpupools, the code below is assigning all the not >> + * assigned cpu to a new pool (next_pool_id value is the last id + 1). >> + * In the same loop we check if there is any assigned cpu that is not >> + * online. >> + */ >> + for ( i = 0; i < nr_cpu_ids; i++ ) >> + if ( cpumask_test_cpu(i, &cpu_online_map) ) >> + { >> + /* Unassigned cpu gets next_pool_id pool id value */ >> + if ( pool_cpu_map[i] < 0 ) >> + { >> + pool_cpu_map[i] = next_pool_id; >> + add_extra_cpupool = true; >> + } >> + printk(XENLOG_INFO "Logical CPU %u in Pool-%u.\n", i, >> + pool_cpu_map[i]); >> + } >> + else >> + { >> + if ( pool_cpu_map[i] >= 0 ) >> + panic("Pool-%d contains cpu%u that is not online!\n", >> + pool_cpu_map[i], i); >> + } >> + >> + if ( add_extra_cpupool ) >> + next_pool_id++; >> + >> + /* Create cpupools with selected schedulers */ >> + for ( i = 0; i < next_pool_id; i++ ) >> + cpupool_create_pool(i, pool_sched_map[i]); >> + >> +#ifdef CONFIG_X86 >> + /* Cpu0 must be in cpupool0 for x86 */ >> + if ( pool_cpu_map[0] != 0 ) >> + panic("Cpu0 must be in Pool-0\n"); >> +#endif > > Can you document why this is necessary on x86 but not on other architectures? Hi Julien, I received the warning by Juergen here: https://patchwork.kernel.org/comment/24740762/ that at least on x86 there could be some problems if cpu0 is not in cpupool0, I tested it on arm and it was working fine and I didn’t find any restriction. So I don’t know why on x86 we must have cpu0 in cpupool0, maybe x86 maintainer have more knowledge about that and I can put a comment here. Cheers, Luca > > Cheers, > > -- > Julien Grall
Hi, On 21/03/2022 15:58, Luca Fancellu wrote: > > >> On 18 Mar 2022, at 16:12, Julien Grall <julien@xen.org> wrote: >> >> Hi Luca, >> >> I only skimmed through the series. I have one question below: >> >> On 18/03/2022 15:25, Luca Fancellu wrote: >>> +void __init btcpupools_allocate_pools(void) >>> +{ >>> + unsigned int i; >>> + bool add_extra_cpupool = false; >>> + >>> + /* >>> + * If there are no cpupools, the value of next_pool_id is zero, so the code >>> + * below will assign every cpu to cpupool0 as the default behavior. >>> + * When there are cpupools, the code below is assigning all the not >>> + * assigned cpu to a new pool (next_pool_id value is the last id + 1). >>> + * In the same loop we check if there is any assigned cpu that is not >>> + * online. >>> + */ >>> + for ( i = 0; i < nr_cpu_ids; i++ ) >>> + if ( cpumask_test_cpu(i, &cpu_online_map) ) >>> + { >>> + /* Unassigned cpu gets next_pool_id pool id value */ >>> + if ( pool_cpu_map[i] < 0 ) >>> + { >>> + pool_cpu_map[i] = next_pool_id; >>> + add_extra_cpupool = true; >>> + } >>> + printk(XENLOG_INFO "Logical CPU %u in Pool-%u.\n", i, >>> + pool_cpu_map[i]); >>> + } >>> + else >>> + { >>> + if ( pool_cpu_map[i] >= 0 ) >>> + panic("Pool-%d contains cpu%u that is not online!\n", >>> + pool_cpu_map[i], i); >>> + } >>> + >>> + if ( add_extra_cpupool ) >>> + next_pool_id++; >>> + >>> + /* Create cpupools with selected schedulers */ >>> + for ( i = 0; i < next_pool_id; i++ ) >>> + cpupool_create_pool(i, pool_sched_map[i]); >>> + >>> +#ifdef CONFIG_X86 >>> + /* Cpu0 must be in cpupool0 for x86 */ >>> + if ( pool_cpu_map[0] != 0 ) >>> + panic("Cpu0 must be in Pool-0\n"); >>> +#endif >> >> Can you document why this is necessary on x86 but not on other architectures? > > Hi Julien, > > I received the warning by Juergen here: https://patchwork.kernel.org/comment/24740762/ that at least on x86 there could be > some problems if cpu0 is not in cpupool0, I tested it on arm and it was working fine and I didn’t find any restriction. What exactly did you test on Arm? > > So I don’t know why on x86 we must have cpu0 in cpupool0, maybe x86 maintainer have more knowledge about that and > I can put a comment here. On Arm, we are not yet supporting all the CPU features that x86 supports (e.g. CPU hotplug, suspend/resume...). So I a am bit concerned that the restriction is just not there yet (or possibly hidden). Therefore, before lifting the restriction on Arm (and other arch), I would like us to understand why it is necessary on x86. We may not have the answer quickly, so is it going to be a problem to keep the restriction on Arm? Cheers,
Hi Julien, > On 21 Mar 2022, at 17:36, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 21/03/2022 15:58, Luca Fancellu wrote: >>> On 18 Mar 2022, at 16:12, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Luca, >>> >>> I only skimmed through the series. I have one question below: >>> >>> On 18/03/2022 15:25, Luca Fancellu wrote: >>>> +void __init btcpupools_allocate_pools(void) >>>> +{ >>>> + unsigned int i; >>>> + bool add_extra_cpupool = false; >>>> + >>>> + /* >>>> + * If there are no cpupools, the value of next_pool_id is zero, so the code >>>> + * below will assign every cpu to cpupool0 as the default behavior. >>>> + * When there are cpupools, the code below is assigning all the not >>>> + * assigned cpu to a new pool (next_pool_id value is the last id + 1). >>>> + * In the same loop we check if there is any assigned cpu that is not >>>> + * online. >>>> + */ >>>> + for ( i = 0; i < nr_cpu_ids; i++ ) >>>> + if ( cpumask_test_cpu(i, &cpu_online_map) ) >>>> + { >>>> + /* Unassigned cpu gets next_pool_id pool id value */ >>>> + if ( pool_cpu_map[i] < 0 ) >>>> + { >>>> + pool_cpu_map[i] = next_pool_id; >>>> + add_extra_cpupool = true; >>>> + } >>>> + printk(XENLOG_INFO "Logical CPU %u in Pool-%u.\n", i, >>>> + pool_cpu_map[i]); >>>> + } >>>> + else >>>> + { >>>> + if ( pool_cpu_map[i] >= 0 ) >>>> + panic("Pool-%d contains cpu%u that is not online!\n", >>>> + pool_cpu_map[i], i); >>>> + } >>>> + >>>> + if ( add_extra_cpupool ) >>>> + next_pool_id++; >>>> + >>>> + /* Create cpupools with selected schedulers */ >>>> + for ( i = 0; i < next_pool_id; i++ ) >>>> + cpupool_create_pool(i, pool_sched_map[i]); >>>> + >>>> +#ifdef CONFIG_X86 >>>> + /* Cpu0 must be in cpupool0 for x86 */ >>>> + if ( pool_cpu_map[0] != 0 ) >>>> + panic("Cpu0 must be in Pool-0\n"); >>>> +#endif >>> >>> Can you document why this is necessary on x86 but not on other architectures? >> Hi Julien, >> I received the warning by Juergen here: https://patchwork.kernel.org/comment/24740762/ that at least on x86 there could be >> some problems if cpu0 is not in cpupool0, I tested it on arm and it was working fine and I didn’t find any restriction. > > What exactly did you test on Arm? > >> So I don’t know why on x86 we must have cpu0 in cpupool0, maybe x86 maintainer have more knowledge about that and >> I can put a comment here. > > On Arm, we are not yet supporting all the CPU features that x86 supports (e.g. CPU hotplug, suspend/resume...). So I a am bit concerned that the restriction is just not there yet (or possibly hidden). > > Therefore, before lifting the restriction on Arm (and other arch), I would like us to understand why it is necessary on x86. > > We may not have the answer quickly, so is it going to be a problem to keep the restriction on Arm? I am ok to keep the limitation to have dom0 always running on cpu0. Only limitation I can see is that on a big little system, dom0 needs to stay on the type of core of the first booted core. But as the user could modify his firmware to boot on the type of core he wants, this limitation can usually be worked around. So it is not a problem. Maybe we could make that an unsupported feature that one could activate through the configuration ? Cheers Bertrand > > Cheers, > > -- > Julien Grall
Hi Bertrand, On 21/03/2022 17:19, Bertrand Marquis wrote: >> On 21 Mar 2022, at 17:36, Julien Grall <julien@xen.org> wrote: >>> So I don’t know why on x86 we must have cpu0 in cpupool0, maybe x86 maintainer have more knowledge about that and >>> I can put a comment here. >> >> On Arm, we are not yet supporting all the CPU features that x86 supports (e.g. CPU hotplug, suspend/resume...). So I a am bit concerned that the restriction is just not there yet (or possibly hidden). >> >> Therefore, before lifting the restriction on Arm (and other arch), I would like us to understand why it is necessary on x86. >> >> We may not have the answer quickly, so is it going to be a problem to keep the restriction on Arm? > > I am ok to keep the limitation to have dom0 always running on cpu0. > Only limitation I can see is that on a big little system, dom0 needs to stay on the type of core of the first booted core. Where does this limitation come from? Cheers,
On Fri, 18 Mar 2022, 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@arm.com> > --- > Changes in v3: > - Add newline to cpupools.txt and removed "default n" from Kconfig (Jan) > - Fixed comment, moved defines, used global cpu_online_map, use > HAS_DEVICE_TREE instead of ARM and place arch specific code in header > (Juergen) > - Fix brakets, x86 code only panic, get rid of scheduler dt node, don't > save pool pointer and look for it from the pool list (Stefano) > - Changed data structures to allow modification to the code. > 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 | 135 +++++++++++++++++++ > xen/arch/arm/include/asm/smp.h | 3 + > xen/common/Kconfig | 7 + > xen/common/Makefile | 1 + > xen/common/boot_cpupools.c | 178 +++++++++++++++++++++++++ > xen/common/sched/cpupool.c | 9 +- > xen/include/xen/sched.h | 19 +++ > 7 files changed, 351 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..6d7463736b48 > --- /dev/null > +++ b/docs/misc/arm/device-tree/cpupools.txt > @@ -0,0 +1,135 @@ > +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 string having the name of a Xen scheduler, it has no effect when > + used in conjunction of a cpupool-id equal to zero, in that case the > + default Xen scheduler is selected (sched=<...> boot argument). > + Check the sched=<...> boot argument for allowed values. I am happy with this version of the device tree bindings, thanks for your efforts to update them. Only one comment left: please update the description not to include "cpupool-id" given that there is no cpupool-id property anymore :-) > +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 { > + > + 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 = "credit2"; > + }; > + > + [...] > + > +}; > + > + > +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 { > + > + cpupool_a { > + compatible = "xen,cpupool"; > + cpupool-cpus = <&a53_1 &a53_2>; > + }; > + cpupool_b { > + compatible = "xen,cpupool"; > + cpupool-cpus = <&a72_1 &a72_2>; > + cpupool-sched = "null"; > + }; > + > + [...] > + > +}; I think it looks great, thanks! > diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h > index af5a2fe65266..83c0cd69767b 100644 > --- a/xen/arch/arm/include/asm/smp.h > +++ b/xen/arch/arm/include/asm/smp.h > @@ -34,6 +34,9 @@ extern void init_secondary(void); > extern void smp_init_cpus(void); > extern void smp_clear_cpu_maps (void); > extern int smp_get_max_cpus (void); > + > +#define cpu_physical_id(cpu) cpu_logical_map(cpu) > + > #endif > > /* > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index d921c74d615e..70aac5220e75 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -22,6 +22,13 @@ config GRANT_TABLE > > If unsure, say Y. > > +config BOOT_TIME_CPUPOOLS > + bool "Create cpupools at boot time" > + depends on HAS_DEVICE_TREE > + 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..f6f2fa8f2701 > --- /dev/null > +++ b/xen/common/boot_cpupools.c > @@ -0,0 +1,178 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * xen/common/boot_cpupools.c > + * > + * Code to create cpupools at boot time. > + * > + * Copyright (C) 2022 Arm Ltd. > + */ > + > +#include <xen/sched.h> > + > +/* > + * pool_cpu_map: Index is logical cpu number, content is cpupool id, (-1) for > + * unassigned. > + * pool_sched_map: Index is cpupool id, content is scheduler id, (-1) for > + * unassigned. > + */ > +static int __initdata pool_cpu_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; > +static int __initdata pool_sched_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; > +static unsigned int __initdata next_pool_id; > + > +#ifdef CONFIG_HAS_DEVICE_TREE BOOT_TIME_CPUPOOLS depends on HAS_DEVICE_TREE, so it is not possible to have BOOT_TIME_CPUPOOLS but not HAS_DEVICE_TREE ? > +#define BTCPUPOOLS_DT_NODE_NO_REG (-1) > +#define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2) > + > +static int __init get_logical_cpu_from_hw_id(unsigned int hwid) > +{ > + unsigned int i; > + > + for ( i = 0; i < nr_cpu_ids; i++ ) > + if ( cpu_physical_id(i) == hwid ) > + return i; > + > + return -1; > +} I wonder if there is a better way to implement this function but I am not sure. Also, it might be better to avoid premature optimizations. That said, we could check first the simple case where hwid==i. Looking at various existing device tree, it seems to be the most common case. This is not a requirement, just a hand-wavy suggestion. I think the patch is also OK as is. > +static int __init > +get_logical_cpu_from_cpu_node(const struct dt_device_node *cpu_node) > +{ > + int cpu_num; > + const __be32 *prop; > + unsigned int cpu_reg; > + > + prop = dt_get_property(cpu_node, "reg", NULL); > + if ( !prop ) > + return BTCPUPOOLS_DT_NODE_NO_REG; > + > + cpu_reg = dt_read_number(prop, dt_n_addr_cells(cpu_node)); > + > + cpu_num = get_logical_cpu_from_hw_id(cpu_reg); > + if ( cpu_num < 0 ) > + return BTCPUPOOLS_DT_NODE_NO_LOG_CPU; > + > + return cpu_num; > +} > + > +static int __init check_and_get_sched_id(const char* scheduler_name) > +{ > + int sched_id = sched_get_id_by_name(scheduler_name); > + > + if ( sched_id < 0 ) > + panic("Scheduler %s does not exists!\n", scheduler_name); > + > + return sched_id; > +} > + > +void __init btcpupools_dtb_parse(void) > +{ > + const struct dt_device_node *chosen, *node; > + > + chosen = dt_find_node_by_path("/chosen"); > + if ( !chosen ) > + return; > + > + dt_for_each_child_node(chosen, node) > + { > + const struct dt_device_node *phandle_node; > + int sched_id = -1; > + const char* scheduler_name; > + unsigned int i = 0; > + > + if ( !dt_device_is_compatible(node, "xen,cpupool") ) > + continue; > + > + if ( !dt_property_read_string(phandle_node, "cpupool-sched", > + &scheduler_name) ) > + sched_id = check_and_get_sched_id(scheduler_name); > + > + phandle_node = dt_parse_phandle(node, "cpupool-cpus", i++); > + if ( !phandle_node ) > + panic("Missing or empty cpupool-cpus property!\n"); > + > + while ( phandle_node ) > + { > + int cpu_num; > + > + cpu_num = get_logical_cpu_from_cpu_node(phandle_node); > + > + if ( cpu_num < 0 ) > + panic("Error retrieving logical cpu from node %s (%d)\n", > + dt_node_name(node), cpu_num); > + > + if ( pool_cpu_map[cpu_num] != -1 ) > + panic("Logical cpu %d already added to a cpupool!\n", cpu_num); > + > + pool_cpu_map[cpu_num] = next_pool_id; > + pool_sched_map[next_pool_id] = sched_id; > + > + phandle_node = dt_parse_phandle(node, "cpupool-cpus", i++); > + } > + > + /* Let Xen generate pool ids */ > + next_pool_id++; > + } > +} > +#endif > + > +void __init btcpupools_allocate_pools(void) > +{ > + unsigned int i; > + bool add_extra_cpupool = false; > + > + /* > + * If there are no cpupools, the value of next_pool_id is zero, so the code > + * below will assign every cpu to cpupool0 as the default behavior. > + * When there are cpupools, the code below is assigning all the not > + * assigned cpu to a new pool (next_pool_id value is the last id + 1). > + * In the same loop we check if there is any assigned cpu that is not > + * online. > + */ > + for ( i = 0; i < nr_cpu_ids; i++ ) > + if ( cpumask_test_cpu(i, &cpu_online_map) ) Let me take this opportunity to explain the unfortunately unwritten coding style the way I understand it. I know this is tribal knowledge at the moment and I apologize for that. If it is a single line statement, we skip the { }, we keep them in all other cases. So: /* correct */ if ( xxx ) { something; something else; } /* correct */ if ( xxx ) { for ( yyy ) { } } /* correct */ if ( xxx ) something single line or 2 lines like a printk that go beyond 80 chars, never in case of nested ifs /* not correct */ if ( xxx ) something multi line; /* not correct */ if ( xxx ) if ( yyy ) something; So basically we would keep the { } here but we would skip them ... > + { > + /* Unassigned cpu gets next_pool_id pool id value */ > + if ( pool_cpu_map[i] < 0 ) > + { > + pool_cpu_map[i] = next_pool_id; > + add_extra_cpupool = true; > + } > + printk(XENLOG_INFO "Logical CPU %u in Pool-%u.\n", i, > + pool_cpu_map[i]); > + } > + else > + { > + if ( pool_cpu_map[i] >= 0 ) > + panic("Pool-%d contains cpu%u that is not online!\n", > + pool_cpu_map[i], i); ...skip them here > + } > + > + if ( add_extra_cpupool ) > + next_pool_id++; ...and skip them here > + /* Create cpupools with selected schedulers */ > + for ( i = 0; i < next_pool_id; i++ ) > + cpupool_create_pool(i, pool_sched_map[i]); > + > +#ifdef CONFIG_X86 > + /* Cpu0 must be in cpupool0 for x86 */ > + if ( pool_cpu_map[0] != 0 ) > + panic("Cpu0 must be in Pool-0\n"); > +#endif > +} > + > +unsigned int __init btcpupools_get_cpupool_id(unsigned int cpu) > +{ > + return pool_cpu_map[cpu]; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c > index 89a891af7076..e5189c53a321 100644 > --- a/xen/common/sched/cpupool.c > +++ b/xen/common/sched/cpupool.c > @@ -1247,12 +1247,19 @@ static int __init cf_check cpupool_init(void) > cpupool_put(cpupool0); > register_cpu_notifier(&cpu_nfb); > > + btcpupools_dtb_parse(); > + > + btcpupools_allocate_pools(); > + > spin_lock(&cpupool_lock); > > cpumask_copy(&cpupool_free_cpus, &cpu_online_map); > > for_each_cpu ( cpu, &cpupool_free_cpus ) > - cpupool_assign_cpu_locked(cpupool0, cpu); > + { > + unsigned int pool_id = btcpupools_get_cpupool_id(cpu); > + cpupool_assign_cpu_locked(cpupool_find_by_id(pool_id), cpu); > + } > > spin_unlock(&cpupool_lock); > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > index 4050e22544f9..5d83465d3915 100644 > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -1176,6 +1176,25 @@ extern void cf_check dump_runq(unsigned char key); > > void arch_do_physinfo(struct xen_sysctl_physinfo *pi); > > +#ifdef CONFIG_BOOT_TIME_CPUPOOLS > +void btcpupools_allocate_pools(void); > +unsigned int btcpupools_get_cpupool_id(unsigned int cpu); > + > +#ifdef CONFIG_HAS_DEVICE_TREE > +void btcpupools_dtb_parse(void); > +#else > +static inline void btcpupools_dtb_parse(void) {} > +#endif same comment about !CONFIG_HAS_DEVICE_TREE > +#else /* !CONFIG_BOOT_TIME_CPUPOOLS */ > +static inline void btcpupools_allocate_pools(void) {} > +static inline void btcpupools_dtb_parse(void) {} > +static inline unsigned int btcpupools_get_cpupool_id(unsigned int cpu) > +{ > + return 0; > +} > +#endif > + > #endif /* __SCHED_H__ */ > > /*
Hi Julien, > On 21 Mar 2022, at 18:44, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 21/03/2022 17:19, Bertrand Marquis wrote: >>> On 21 Mar 2022, at 17:36, Julien Grall <julien@xen.org> wrote: >>>> So I don’t know why on x86 we must have cpu0 in cpupool0, maybe x86 maintainer have more knowledge about that and >>>> I can put a comment here. >>> >>> On Arm, we are not yet supporting all the CPU features that x86 supports (e.g. CPU hotplug, suspend/resume...). So I a am bit concerned that the restriction is just not there yet (or possibly hidden). >>> >>> Therefore, before lifting the restriction on Arm (and other arch), I would like us to understand why it is necessary on x86. >>> >>> We may not have the answer quickly, so is it going to be a problem to keep the restriction on Arm? >> I am ok to keep the limitation to have dom0 always running on cpu0. >> Only limitation I can see is that on a big little system, dom0 needs to stay on the type of core of the first booted core. > > Where does this limitation come from? If dom0 must run on core0 and core0 is Little then you cannot build a system where dom0 is running on big cores. If the limitation is not there, you can build such a configuration without any dependency to the boot core type. Cheers Bertrand > > Cheers, > > -- > Julien Grall
Hi Julien, > On 22 Mar 2022, at 09:35, Bertrand Marquis <bertrand.marquis@arm.com> wrote: > > Hi Julien, > >> On 21 Mar 2022, at 18:44, Julien Grall <julien@xen.org> wrote: >> >> Hi Bertrand, >> >> On 21/03/2022 17:19, Bertrand Marquis wrote: >>>> On 21 Mar 2022, at 17:36, Julien Grall <julien@xen.org> wrote: >>>>> So I don’t know why on x86 we must have cpu0 in cpupool0, maybe x86 maintainer have more knowledge about that and >>>>> I can put a comment here. >>>> >>>> On Arm, we are not yet supporting all the CPU features that x86 supports (e.g. CPU hotplug, suspend/resume...). So I a am bit concerned that the restriction is just not there yet (or possibly hidden). >>>> >>>> Therefore, before lifting the restriction on Arm (and other arch), I would like us to understand why it is necessary on x86. >>>> >>>> We may not have the answer quickly, so is it going to be a problem to keep the restriction on Arm? >>> I am ok to keep the limitation to have dom0 always running on cpu0. >>> Only limitation I can see is that on a big little system, dom0 needs to stay on the type of core of the first booted core. >> >> Where does this limitation come from? > > If dom0 must run on core0 and core0 is Little then you cannot build a system where dom0 is running on big cores. > If the limitation is not there, you can build such a configuration without any dependency to the boot core type. This might not be completely clear so let me rephrase: In the current system: - dom0 must run on cpupool-0 - cpupool-0 must contain the boot core - consequence: dom0 must run on the boot core If boot core is little, you cannot build as system where dom0 runs only on the big cores. Removing the second limitation (which is not required on arm) is making it possible. Regards Bertrand
>> +- cpupool-sched (optional) >> + >> + Must be a string having the name of a Xen scheduler, it has no effect when >> + used in conjunction of a cpupool-id equal to zero, in that case the >> + default Xen scheduler is selected (sched=<...> boot argument). >> + Check the sched=<...> boot argument for allowed values. > > I am happy with this version of the device tree bindings, thanks for > your efforts to update them. Only one comment left: please update the > description not to include "cpupool-id" given that there is no > cpupool-id property anymore :-) > Hi Stefano, Thank you for your review, Yes I missed that! I will fix in the next serie. >> >> +/* >> + * pool_cpu_map: Index is logical cpu number, content is cpupool id, (-1) for >> + * unassigned. >> + * pool_sched_map: Index is cpupool id, content is scheduler id, (-1) for >> + * unassigned. >> + */ >> +static int __initdata pool_cpu_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; >> +static int __initdata pool_sched_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; >> +static unsigned int __initdata next_pool_id; >> + >> +#ifdef CONFIG_HAS_DEVICE_TREE > > BOOT_TIME_CPUPOOLS depends on HAS_DEVICE_TREE, so it is not possible to > have BOOT_TIME_CPUPOOLS but not HAS_DEVICE_TREE ? Yes you are right, the ifdef is not needed at this stage since only arch with device tree are using it, if x86 would like to implement a command line version then the code will be ifdef-ined later. > > >> +#define BTCPUPOOLS_DT_NODE_NO_REG (-1) >> +#define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2) >> + >> +static int __init get_logical_cpu_from_hw_id(unsigned int hwid) >> +{ >> + unsigned int i; >> + >> + for ( i = 0; i < nr_cpu_ids; i++ ) >> + if ( cpu_physical_id(i) == hwid ) >> + return i; >> + >> + return -1; >> +} > > I wonder if there is a better way to implement this function but I am > not sure. Also, it might be better to avoid premature optimizations. > > That said, we could check first the simple case where hwid==i. Looking > at various existing device tree, it seems to be the most common case. > > This is not a requirement, just a hand-wavy suggestion. I think the > patch is also OK as is. > Not sure to understand here, at least on FVP (the first DT I have around), hwid != i, Or maybe I didn’t understand what you mean > >> +void __init btcpupools_allocate_pools(void) >> +{ >> + unsigned int i; >> + bool add_extra_cpupool = false; >> + >> + /* >> + * If there are no cpupools, the value of next_pool_id is zero, so the code >> + * below will assign every cpu to cpupool0 as the default behavior. >> + * When there are cpupools, the code below is assigning all the not >> + * assigned cpu to a new pool (next_pool_id value is the last id + 1). >> + * In the same loop we check if there is any assigned cpu that is not >> + * online. >> + */ >> + for ( i = 0; i < nr_cpu_ids; i++ ) >> + if ( cpumask_test_cpu(i, &cpu_online_map) ) > > Let me take this opportunity to explain the unfortunately unwritten > coding style the way I understand it. I know this is tribal knowledge at > the moment and I apologize for that. > > If it is a single line statement, we skip the { }, we keep them in all > other cases. > > So: > > /* correct */ > if ( xxx ) { > something; > something else; > } > > /* correct */ > if ( xxx ) { > for ( yyy ) { > } > } > > /* correct */ > if ( xxx ) > something single line or 2 lines like a printk that go beyond 80 > chars, never in case of nested ifs > > /* not correct */ > if ( xxx ) > something > multi > line; > > /* not correct */ > if ( xxx ) > if ( yyy ) > something; > > So basically we would keep the { } here but we would skip them ... Ok this clarifies a lot, thank you, I will check the code and I will fix it where it’s not compliant. > >> +#ifdef CONFIG_HAS_DEVICE_TREE >> +void btcpupools_dtb_parse(void); >> +#else >> +static inline void btcpupools_dtb_parse(void) {} >> +#endif > > same comment about !CONFIG_HAS_DEVICE_TREE Yes I will fix it in the next serie. Cheers, Luca
Hi, On 22/03/2022 08:47, Bertrand Marquis wrote: > Hi Julien, > >> On 22 Mar 2022, at 09:35, Bertrand Marquis <bertrand.marquis@arm.com> wrote: >> >> Hi Julien, >> >>> On 21 Mar 2022, at 18:44, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Bertrand, >>> >>> On 21/03/2022 17:19, Bertrand Marquis wrote: >>>>> On 21 Mar 2022, at 17:36, Julien Grall <julien@xen.org> wrote: >>>>>> So I don’t know why on x86 we must have cpu0 in cpupool0, maybe x86 maintainer have more knowledge about that and >>>>>> I can put a comment here. >>>>> >>>>> On Arm, we are not yet supporting all the CPU features that x86 supports (e.g. CPU hotplug, suspend/resume...). So I a am bit concerned that the restriction is just not there yet (or possibly hidden). >>>>> >>>>> Therefore, before lifting the restriction on Arm (and other arch), I would like us to understand why it is necessary on x86. >>>>> >>>>> We may not have the answer quickly, so is it going to be a problem to keep the restriction on Arm? >>>> I am ok to keep the limitation to have dom0 always running on cpu0. >>>> Only limitation I can see is that on a big little system, dom0 needs to stay on the type of core of the first booted core. >>> >>> Where does this limitation come from? >> >> If dom0 must run on core0 and core0 is Little then you cannot build a system where dom0 is running on big cores. >> If the limitation is not there, you can build such a configuration without any dependency to the boot core type. > > This might not be completely clear so let me rephrase: > In the current system: > - dom0 must run on cpupool-0 I don't think we need this restriction. In fact, with this series it will become more a problem because the cpupool ID will based on how we parse the Device-Tree. So for dom0, we need to specify explicitely the cpupool to be used. > - cpupool-0 must contain the boot core > - consequence: dom0 must run on the boot core > > If boot core is little, you cannot build as system where dom0 runs only on the big cores. > Removing the second limitation (which is not required on arm) is making it possible. IMHO removing the second restriction is a lot more risky than removing the first one. Cheers,
Hi Julien, > On 22 Mar 2022, at 10:16, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 22/03/2022 08:47, Bertrand Marquis wrote: >> Hi Julien, >>> On 22 Mar 2022, at 09:35, Bertrand Marquis <bertrand.marquis@arm.com> wrote: >>> >>> Hi Julien, >>> >>>> On 21 Mar 2022, at 18:44, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi Bertrand, >>>> >>>> On 21/03/2022 17:19, Bertrand Marquis wrote: >>>>>> On 21 Mar 2022, at 17:36, Julien Grall <julien@xen.org> wrote: >>>>>>> So I don’t know why on x86 we must have cpu0 in cpupool0, maybe x86 maintainer have more knowledge about that and >>>>>>> I can put a comment here. >>>>>> >>>>>> On Arm, we are not yet supporting all the CPU features that x86 supports (e.g. CPU hotplug, suspend/resume...). So I a am bit concerned that the restriction is just not there yet (or possibly hidden). >>>>>> >>>>>> Therefore, before lifting the restriction on Arm (and other arch), I would like us to understand why it is necessary on x86. >>>>>> >>>>>> We may not have the answer quickly, so is it going to be a problem to keep the restriction on Arm? >>>>> I am ok to keep the limitation to have dom0 always running on cpu0. >>>>> Only limitation I can see is that on a big little system, dom0 needs to stay on the type of core of the first booted core. >>>> >>>> Where does this limitation come from? >>> >>> If dom0 must run on core0 and core0 is Little then you cannot build a system where dom0 is running on big cores. >>> If the limitation is not there, you can build such a configuration without any dependency to the boot core type. >> This might not be completely clear so let me rephrase: >> In the current system: >> - dom0 must run on cpupool-0 > > I don't think we need this restriction. In fact, with this series it will become more a problem because the cpupool ID will based on how we parse the Device-Tree. > > So for dom0, we need to specify explicitely the cpupool to be used. > >> - cpupool-0 must contain the boot core >> - consequence: dom0 must run on the boot core >> If boot core is little, you cannot build as system where dom0 runs only on the big cores. >> Removing the second limitation (which is not required on arm) is making it possible. > > IMHO removing the second restriction is a lot more risky than removing the first one. So keeping boot core in cpupool-0 but allow Dom-0 to be in any pool. Interesting, we will check that. Cheers Bertrand > > Cheers, > > -- > Julien Grall
>>> >>> Can you document why this is necessary on x86 but not on other architectures? >> Hi Julien, >> I received the warning by Juergen here: https://patchwork.kernel.org/comment/24740762/ that at least on x86 there could be >> some problems if cpu0 is not in cpupool0, I tested it on arm and it was working fine and I didn’t find any restriction. > > What exactly did you test on Arm? > I have tested start/stop of some guest, moving cpus between cpupools, create/destroy cpupools, shutdown of Dom0 [ from your last mail ] >>> >>> If dom0 must run on core0 and core0 is Little then you cannot build a system where dom0 is running on big cores. >>> If the limitation is not there, you can build such a configuration without any dependency to the boot core type. >> This might not be completely clear so let me rephrase: >> In the current system: >> - dom0 must run on cpupool-0 > > I don't think we need this restriction. In fact, with this series it will become more a problem because the cpupool ID will based on how we parse the Device-Tree. > > So for dom0, we need to specify explicitely the cpupool to be used. > >> - cpupool-0 must contain the boot core >> - consequence: dom0 must run on the boot core >> If boot core is little, you cannot build as system where dom0 runs only on the big cores. >> Removing the second limitation (which is not required on arm) is making it possible. > > IMHO removing the second restriction is a lot more risky than removing the first one. I see your point, my concern about moving Dom0 on another cpupool, different from cpupool0, is that we give the opportunity to destroy the cpupool0 (we can’t let that happen), or remove every cpu from cpupool0. Cheers, Luca
Hi, On 22/03/2022 09:52, Luca Fancellu wrote: >>>> >>>> Can you document why this is necessary on x86 but not on other architectures? >>> Hi Julien, >>> I received the warning by Juergen here: https://patchwork.kernel.org/comment/24740762/ that at least on x86 there could be >>> some problems if cpu0 is not in cpupool0, I tested it on arm and it was working fine and I didn’t find any restriction. >> >> What exactly did you test on Arm? >> > > I have tested start/stop of some guest, moving cpus between cpupools, create/destroy cpupools, shutdown of Dom0 > > > [ from your last mail ] >>>> >>>> If dom0 must run on core0 and core0 is Little then you cannot build a system where dom0 is running on big cores. >>>> If the limitation is not there, you can build such a configuration without any dependency to the boot core type. >>> This might not be completely clear so let me rephrase: >>> In the current system: >>> - dom0 must run on cpupool-0 >> >> I don't think we need this restriction. In fact, with this series it will become more a problem because the cpupool ID will based on how we parse the Device-Tree. >> >> So for dom0, we need to specify explicitely the cpupool to be used. >> >>> - cpupool-0 must contain the boot core >>> - consequence: dom0 must run on the boot core >>> If boot core is little, you cannot build as system where dom0 runs only on the big cores. >>> Removing the second limitation (which is not required on arm) is making it possible. >> >> IMHO removing the second restriction is a lot more risky than removing the first one. > > I see your point, my concern about moving Dom0 on another cpupool, different from cpupool0, is that we give the > opportunity to destroy the cpupool0 (we can’t let that happen), or remove every cpu from cpupool0. From my understanding a cpupool can only be destroyed when there are no more CPUs in the pool. Given that cpu0 has to be in pool0 then this should prevent the pool to be destroyed. Now, it is quite possible that we don't have a check to prevent CPU0 to be removed from cpupool0. If so, then I would argue we should add the check otherwise it is pointless to prevent cpu0 to be initially added in another pool than pool0 but can be moved afterwards. Cheers,
On Tue, 22 Mar 2022, Luca Fancellu wrote: > >> +- cpupool-sched (optional) > >> + > >> + Must be a string having the name of a Xen scheduler, it has no effect when > >> + used in conjunction of a cpupool-id equal to zero, in that case the > >> + default Xen scheduler is selected (sched=<...> boot argument). > >> + Check the sched=<...> boot argument for allowed values. > > > > I am happy with this version of the device tree bindings, thanks for > > your efforts to update them. Only one comment left: please update the > > description not to include "cpupool-id" given that there is no > > cpupool-id property anymore :-) > > > > Hi Stefano, > > Thank you for your review, > > Yes I missed that! I will fix in the next serie. > > >> > >> +/* > >> + * pool_cpu_map: Index is logical cpu number, content is cpupool id, (-1) for > >> + * unassigned. > >> + * pool_sched_map: Index is cpupool id, content is scheduler id, (-1) for > >> + * unassigned. > >> + */ > >> +static int __initdata pool_cpu_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; > >> +static int __initdata pool_sched_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; > >> +static unsigned int __initdata next_pool_id; > >> + > >> +#ifdef CONFIG_HAS_DEVICE_TREE > > > > BOOT_TIME_CPUPOOLS depends on HAS_DEVICE_TREE, so it is not possible to > > have BOOT_TIME_CPUPOOLS but not HAS_DEVICE_TREE ? > > Yes you are right, the ifdef is not needed at this stage since only arch with device tree are > using it, if x86 would like to implement a command line version then the code will be ifdef-ined > later. > > > > > > >> +#define BTCPUPOOLS_DT_NODE_NO_REG (-1) > >> +#define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2) > >> + > >> +static int __init get_logical_cpu_from_hw_id(unsigned int hwid) > >> +{ > >> + unsigned int i; > >> + > >> + for ( i = 0; i < nr_cpu_ids; i++ ) > >> + if ( cpu_physical_id(i) == hwid ) > >> + return i; > >> + > >> + return -1; > >> +} > > > > I wonder if there is a better way to implement this function but I am > > not sure. Also, it might be better to avoid premature optimizations. > > > > That said, we could check first the simple case where hwid==i. Looking > > at various existing device tree, it seems to be the most common case. > > > > This is not a requirement, just a hand-wavy suggestion. I think the > > patch is also OK as is. > > > > Not sure to understand here, at least on FVP (the first DT I have around), hwid != i, > Or maybe I didn’t understand what you mean I am not surprised. In many boards hwid == i, but it is not a guarantee at all. To be honest mine was not really a concrete suggestion, more like the beginning of a discussion on the subject. The goal would be to avoid having to scan the __cpu_logical_map array every time without adding a second data structure. I don't feel strongly about it but I thought I would mention it anyway just in case you (or someone else) gets a better idea on how to do this.
> On 22 Mar 2022, at 14:01, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 22/03/2022 09:52, Luca Fancellu wrote: >>>>> >>>>> Can you document why this is necessary on x86 but not on other architectures? >>>> Hi Julien, >>>> I received the warning by Juergen here: https://patchwork.kernel.org/comment/24740762/ that at least on x86 there could be >>>> some problems if cpu0 is not in cpupool0, I tested it on arm and it was working fine and I didn’t find any restriction. >>> >>> What exactly did you test on Arm? >>> >> I have tested start/stop of some guest, moving cpus between cpupools, create/destroy cpupools, shutdown of Dom0 >> [ from your last mail ] >>>>> >>>>> If dom0 must run on core0 and core0 is Little then you cannot build a system where dom0 is running on big cores. >>>>> If the limitation is not there, you can build such a configuration without any dependency to the boot core type. >>>> This might not be completely clear so let me rephrase: >>>> In the current system: >>>> - dom0 must run on cpupool-0 >>> >>> I don't think we need this restriction. In fact, with this series it will become more a problem because the cpupool ID will based on how we parse the Device-Tree. >>> >>> So for dom0, we need to specify explicitely the cpupool to be used. >>> >>>> - cpupool-0 must contain the boot core >>>> - consequence: dom0 must run on the boot core >>>> If boot core is little, you cannot build as system where dom0 runs only on the big cores. >>>> Removing the second limitation (which is not required on arm) is making it possible. >>> >>> IMHO removing the second restriction is a lot more risky than removing the first one. >> I see your point, my concern about moving Dom0 on another cpupool, different from cpupool0, is that we give the >> opportunity to destroy the cpupool0 (we can’t let that happen), or remove every cpu from cpupool0. > > From my understanding a cpupool can only be destroyed when there are no more CPUs in the pool. Given that cpu0 has to be in pool0 then this should prevent the pool to be destroyed. > > Now, it is quite possible that we don't have a check to prevent CPU0 to be removed from cpupool0. If so, then I would argue we should add the check otherwise it is pointless to prevent cpu0 to be initially added in another pool than pool0 but can be moved afterwards. > Hi Julien, I’ve done a test on fvp, first finding is that cpu0 can be removed from Pool-0, there is no check. Afterwards I’ve created another pool and I’ve assigned a cpu to it, I’ve called xl cpupool-destroy and the tool removes every cpu from the pool before destroying. Do you think the check that prevents CPU0 to be removed from Pool-0 should be done in the tools or in Xen? With this change it could be possible to protect cpu0: diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c index a6da4970506a..703005839dd6 100644 --- a/xen/common/sched/cpupool.c +++ b/xen/common/sched/cpupool.c @@ -585,6 +585,12 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu) if ( !cpu_online(cpu) ) return -EINVAL; + if ( !c->cpupool_id && !cpu ) + { + debugtrace_printk("Cpu0 must be in pool with id 0.\n"); + return -EINVAL; + } + master_cpu = sched_get_resource_cpu(cpu); ret = cpupool_unassign_cpu_start(c, master_cpu); if ( ret ) Cheers, Luca > Cheers, > > -- > Julien Grall
Hi, On 23/03/2022 13:58, Luca Fancellu wrote: >> On 22 Mar 2022, at 14:01, Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> On 22/03/2022 09:52, Luca Fancellu wrote: >>>>>> >>>>>> Can you document why this is necessary on x86 but not on other architectures? >>>>> Hi Julien, >>>>> I received the warning by Juergen here: https://patchwork.kernel.org/comment/24740762/ that at least on x86 there could be >>>>> some problems if cpu0 is not in cpupool0, I tested it on arm and it was working fine and I didn’t find any restriction. >>>> >>>> What exactly did you test on Arm? >>>> >>> I have tested start/stop of some guest, moving cpus between cpupools, create/destroy cpupools, shutdown of Dom0 >>> [ from your last mail ] >>>>>> >>>>>> If dom0 must run on core0 and core0 is Little then you cannot build a system where dom0 is running on big cores. >>>>>> If the limitation is not there, you can build such a configuration without any dependency to the boot core type. >>>>> This might not be completely clear so let me rephrase: >>>>> In the current system: >>>>> - dom0 must run on cpupool-0 >>>> >>>> I don't think we need this restriction. In fact, with this series it will become more a problem because the cpupool ID will based on how we parse the Device-Tree. >>>> >>>> So for dom0, we need to specify explicitely the cpupool to be used. >>>> >>>>> - cpupool-0 must contain the boot core >>>>> - consequence: dom0 must run on the boot core >>>>> If boot core is little, you cannot build as system where dom0 runs only on the big cores. >>>>> Removing the second limitation (which is not required on arm) is making it possible. >>>> >>>> IMHO removing the second restriction is a lot more risky than removing the first one. >>> I see your point, my concern about moving Dom0 on another cpupool, different from cpupool0, is that we give the >>> opportunity to destroy the cpupool0 (we can’t let that happen), or remove every cpu from cpupool0. >> >> From my understanding a cpupool can only be destroyed when there are no more CPUs in the pool. Given that cpu0 has to be in pool0 then this should prevent the pool to be destroyed. >> >> Now, it is quite possible that we don't have a check to prevent CPU0 to be removed from cpupool0. If so, then I would argue we should add the check otherwise it is pointless to prevent cpu0 to be initially added in another pool than pool0 but can be moved afterwards. >> > > Hi Julien, > > I’ve done a test on fvp, first finding is that cpu0 can be removed from Pool-0, there is no check. > Afterwards I’ve created another pool and I’ve assigned a cpu to it, I’ve called xl cpupool-destroy and the tool removes every cpu from the pool before destroying. > > Do you think the check that prevents CPU0 to be removed from Pool-0 should be done in the tools or in Xen? I think we want a check at least in Xen (so we don't trust the tools to do the right thing). We could also add one in the tools to provide better diagnostics to the user (this tends to be a request from Andrew). Cheers,
diff --git a/docs/misc/arm/device-tree/cpupools.txt b/docs/misc/arm/device-tree/cpupools.txt new file mode 100644 index 000000000000..6d7463736b48 --- /dev/null +++ b/docs/misc/arm/device-tree/cpupools.txt @@ -0,0 +1,135 @@ +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 string having the name of a Xen scheduler, it has no effect when + used in conjunction of a cpupool-id equal to zero, in that case the + default Xen scheduler is selected (sched=<...> boot argument). + 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 { + + 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 = "credit2"; + }; + + [...] + +}; + + +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 { + + cpupool_a { + compatible = "xen,cpupool"; + cpupool-cpus = <&a53_1 &a53_2>; + }; + cpupool_b { + compatible = "xen,cpupool"; + cpupool-cpus = <&a72_1 &a72_2>; + cpupool-sched = "null"; + }; + + [...] + +}; diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h index af5a2fe65266..83c0cd69767b 100644 --- a/xen/arch/arm/include/asm/smp.h +++ b/xen/arch/arm/include/asm/smp.h @@ -34,6 +34,9 @@ extern void init_secondary(void); extern void smp_init_cpus(void); extern void smp_clear_cpu_maps (void); extern int smp_get_max_cpus (void); + +#define cpu_physical_id(cpu) cpu_logical_map(cpu) + #endif /* diff --git a/xen/common/Kconfig b/xen/common/Kconfig index d921c74d615e..70aac5220e75 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -22,6 +22,13 @@ config GRANT_TABLE If unsure, say Y. +config BOOT_TIME_CPUPOOLS + bool "Create cpupools at boot time" + depends on HAS_DEVICE_TREE + 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..f6f2fa8f2701 --- /dev/null +++ b/xen/common/boot_cpupools.c @@ -0,0 +1,178 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * xen/common/boot_cpupools.c + * + * Code to create cpupools at boot time. + * + * Copyright (C) 2022 Arm Ltd. + */ + +#include <xen/sched.h> + +/* + * pool_cpu_map: Index is logical cpu number, content is cpupool id, (-1) for + * unassigned. + * pool_sched_map: Index is cpupool id, content is scheduler id, (-1) for + * unassigned. + */ +static int __initdata pool_cpu_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; +static int __initdata pool_sched_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; +static unsigned int __initdata next_pool_id; + +#ifdef CONFIG_HAS_DEVICE_TREE + +#define BTCPUPOOLS_DT_NODE_NO_REG (-1) +#define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2) + +static int __init get_logical_cpu_from_hw_id(unsigned int hwid) +{ + unsigned int i; + + for ( i = 0; i < nr_cpu_ids; i++ ) + if ( cpu_physical_id(i) == hwid ) + return i; + + return -1; +} + +static int __init +get_logical_cpu_from_cpu_node(const struct dt_device_node *cpu_node) +{ + int cpu_num; + const __be32 *prop; + unsigned int cpu_reg; + + prop = dt_get_property(cpu_node, "reg", NULL); + if ( !prop ) + return BTCPUPOOLS_DT_NODE_NO_REG; + + cpu_reg = dt_read_number(prop, dt_n_addr_cells(cpu_node)); + + cpu_num = get_logical_cpu_from_hw_id(cpu_reg); + if ( cpu_num < 0 ) + return BTCPUPOOLS_DT_NODE_NO_LOG_CPU; + + return cpu_num; +} + +static int __init check_and_get_sched_id(const char* scheduler_name) +{ + int sched_id = sched_get_id_by_name(scheduler_name); + + if ( sched_id < 0 ) + panic("Scheduler %s does not exists!\n", scheduler_name); + + return sched_id; +} + +void __init btcpupools_dtb_parse(void) +{ + const struct dt_device_node *chosen, *node; + + chosen = dt_find_node_by_path("/chosen"); + if ( !chosen ) + return; + + dt_for_each_child_node(chosen, node) + { + const struct dt_device_node *phandle_node; + int sched_id = -1; + const char* scheduler_name; + unsigned int i = 0; + + if ( !dt_device_is_compatible(node, "xen,cpupool") ) + continue; + + if ( !dt_property_read_string(phandle_node, "cpupool-sched", + &scheduler_name) ) + sched_id = check_and_get_sched_id(scheduler_name); + + phandle_node = dt_parse_phandle(node, "cpupool-cpus", i++); + if ( !phandle_node ) + panic("Missing or empty cpupool-cpus property!\n"); + + while ( phandle_node ) + { + int cpu_num; + + cpu_num = get_logical_cpu_from_cpu_node(phandle_node); + + if ( cpu_num < 0 ) + panic("Error retrieving logical cpu from node %s (%d)\n", + dt_node_name(node), cpu_num); + + if ( pool_cpu_map[cpu_num] != -1 ) + panic("Logical cpu %d already added to a cpupool!\n", cpu_num); + + pool_cpu_map[cpu_num] = next_pool_id; + pool_sched_map[next_pool_id] = sched_id; + + phandle_node = dt_parse_phandle(node, "cpupool-cpus", i++); + } + + /* Let Xen generate pool ids */ + next_pool_id++; + } +} +#endif + +void __init btcpupools_allocate_pools(void) +{ + unsigned int i; + bool add_extra_cpupool = false; + + /* + * If there are no cpupools, the value of next_pool_id is zero, so the code + * below will assign every cpu to cpupool0 as the default behavior. + * When there are cpupools, the code below is assigning all the not + * assigned cpu to a new pool (next_pool_id value is the last id + 1). + * In the same loop we check if there is any assigned cpu that is not + * online. + */ + for ( i = 0; i < nr_cpu_ids; i++ ) + if ( cpumask_test_cpu(i, &cpu_online_map) ) + { + /* Unassigned cpu gets next_pool_id pool id value */ + if ( pool_cpu_map[i] < 0 ) + { + pool_cpu_map[i] = next_pool_id; + add_extra_cpupool = true; + } + printk(XENLOG_INFO "Logical CPU %u in Pool-%u.\n", i, + pool_cpu_map[i]); + } + else + { + if ( pool_cpu_map[i] >= 0 ) + panic("Pool-%d contains cpu%u that is not online!\n", + pool_cpu_map[i], i); + } + + if ( add_extra_cpupool ) + next_pool_id++; + + /* Create cpupools with selected schedulers */ + for ( i = 0; i < next_pool_id; i++ ) + cpupool_create_pool(i, pool_sched_map[i]); + +#ifdef CONFIG_X86 + /* Cpu0 must be in cpupool0 for x86 */ + if ( pool_cpu_map[0] != 0 ) + panic("Cpu0 must be in Pool-0\n"); +#endif +} + +unsigned int __init btcpupools_get_cpupool_id(unsigned int cpu) +{ + return pool_cpu_map[cpu]; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c index 89a891af7076..e5189c53a321 100644 --- a/xen/common/sched/cpupool.c +++ b/xen/common/sched/cpupool.c @@ -1247,12 +1247,19 @@ static int __init cf_check cpupool_init(void) cpupool_put(cpupool0); register_cpu_notifier(&cpu_nfb); + btcpupools_dtb_parse(); + + btcpupools_allocate_pools(); + spin_lock(&cpupool_lock); cpumask_copy(&cpupool_free_cpus, &cpu_online_map); for_each_cpu ( cpu, &cpupool_free_cpus ) - cpupool_assign_cpu_locked(cpupool0, cpu); + { + unsigned int pool_id = btcpupools_get_cpupool_id(cpu); + cpupool_assign_cpu_locked(cpupool_find_by_id(pool_id), cpu); + } spin_unlock(&cpupool_lock); diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 4050e22544f9..5d83465d3915 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -1176,6 +1176,25 @@ extern void cf_check dump_runq(unsigned char key); void arch_do_physinfo(struct xen_sysctl_physinfo *pi); +#ifdef CONFIG_BOOT_TIME_CPUPOOLS +void btcpupools_allocate_pools(void); +unsigned int btcpupools_get_cpupool_id(unsigned int cpu); + +#ifdef CONFIG_HAS_DEVICE_TREE +void btcpupools_dtb_parse(void); +#else +static inline void btcpupools_dtb_parse(void) {} +#endif + +#else /* !CONFIG_BOOT_TIME_CPUPOOLS */ +static inline void btcpupools_allocate_pools(void) {} +static inline void btcpupools_dtb_parse(void) {} +static inline unsigned int btcpupools_get_cpupool_id(unsigned int cpu) +{ + return 0; +} +#endif + #endif /* __SCHED_H__ */ /*
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@arm.com> --- Changes in v3: - Add newline to cpupools.txt and removed "default n" from Kconfig (Jan) - Fixed comment, moved defines, used global cpu_online_map, use HAS_DEVICE_TREE instead of ARM and place arch specific code in header (Juergen) - Fix brakets, x86 code only panic, get rid of scheduler dt node, don't save pool pointer and look for it from the pool list (Stefano) - Changed data structures to allow modification to the code. 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 | 135 +++++++++++++++++++ xen/arch/arm/include/asm/smp.h | 3 + xen/common/Kconfig | 7 + xen/common/Makefile | 1 + xen/common/boot_cpupools.c | 178 +++++++++++++++++++++++++ xen/common/sched/cpupool.c | 9 +- xen/include/xen/sched.h | 19 +++ 7 files changed, 351 insertions(+), 1 deletion(-) create mode 100644 docs/misc/arm/device-tree/cpupools.txt create mode 100644 xen/common/boot_cpupools.c