Re: [RFC PATCH 1/2] xen/cpupool: Create different cpupools at boot time

[Juergen Gross <jgross@xxxxxxxx>]

On 18.11.21 04:01, Stefano Stabellini wrote:
> On Wed, 17 Nov 2021, Juergen Gross wrote:
> > On 17.11.21 10:57, Luca Fancellu wrote:
> > > Introduce an architecture specific way to create different
> > > cpupool at boot time, this is particularly useful on ARM
> > > biglittle 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 pool for each node.
> > >
> > > Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > > ---
> > >    xen/arch/arm/Kconfig       | 15 +++++++
> > >    xen/arch/arm/Makefile      |  1 +
> > >    xen/arch/arm/cpupool.c     | 84 ++++++++++++++++++++++++++++++++++++++
> > >    xen/common/sched/cpupool.c |  5 ++-
> > >    xen/include/xen/cpupool.h  | 30 ++++++++++++++
> > >    5 files changed, 134 insertions(+), 1 deletion(-)
> > >    create mode 100644 xen/arch/arm/cpupool.c
> > >    create mode 100644 xen/include/xen/cpupool.h
> > >
> > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > index ecfa6822e4..4d7cc9f3bc 100644
> > > --- a/xen/arch/arm/Kconfig
> > > +++ b/xen/arch/arm/Kconfig
> > > @@ -33,6 +33,21 @@ config ACPI
> > >           Advanced Configuration and Power Interface (ACPI) support for Xen is
> > >           an alternative to device tree on ARM64.
> > >    +config BOOT_TIME_CPUPOOLS
> > > +       bool "Create cpupools per cpu type at boot time."
> > > +       depends on ARM
> > > +       default n
> > > +       help
> > > +
> > > +         Creates, during boot, a cpu pool for each type of CPU found on
> > > +         the system. This option is useful on system with heterogeneous
> > > +         types of core.
> > > +
> > > +config MAX_BOOT_TIME_CPUPOOLS
> > > +       depends on BOOT_TIME_CPUPOOLS
> > > +       int "Maximum number of cpupools that can be created at boot time."
> > > +       default 16
> > > +
> > >    config GICV3
> > >         bool "GICv3 driver"
> > >         depends on ARM_64 && !NEW_VGIC
> > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > > index 07f634508e..0bb8b84750 100644
> > > --- a/xen/arch/arm/Makefile
> > > +++ b/xen/arch/arm/Makefile
> > > @@ -13,6 +13,7 @@ obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
> > >    obj-y += bootfdt.init.o
> > >    obj-y += cpuerrata.o
> > >    obj-y += cpufeature.o
> > > +obj-$(CONFIG_BOOT_TIME_CPUPOOLS) += cpupool.o
> > >    obj-y += decode.o
> > >    obj-y += device.o
> > >    obj-$(CONFIG_IOREQ_SERVER) += dm.o
> > > diff --git a/xen/arch/arm/cpupool.c b/xen/arch/arm/cpupool.c
> > > new file mode 100644
> > > index 0000000000..550521e352
> > > --- /dev/null
> > > +++ b/xen/arch/arm/cpupool.c
> > > @@ -0,0 +1,84 @@
> > > +/******************************************************************************
> > > + * cpupool.c
> > > + *
> > > + * (C) 2021, arm
> > > + */
> > > +
> > > +#include <xen/cpumask.h>
> > > +#include <xen/cpupool.h>
> > > +#include <xen/err.h>
> > > +#include <xen/sched.h>
> > > +#include <asm/cpufeature.h>
> > > +#include <asm/processor.h>
> > > +#include "../../common/sched/private.h"
> > No, please don't do that.
> >
> > You should try to add architecture agnostic service functions in
> > common/sched/cpupool.c removing the need to include that file here.
> >
> > > +
> > > +typedef struct {
> > > +    register_t     midr;
> > > +    struct cpupool *pool;
> > > +} pool_t;
> > > +
> > > +static pool_t __initdata pool_cpu_map[CONFIG_MAX_BOOT_TIME_CPUPOOLS];
> > > +
> > > +void __init arch_allocate_cpupools(const cpumask_t *cpu_online_map,
> > > +                                   cpupool_create_t cpupool_create)
> > Drop the cpupool_create parameter here and ...
> >
> > > +{
> > > +    unsigned int cpu, i;
> > > +
> > > +    /* First pool is the default pool0 associated with midr of the boot
> > > core */
> > > +    pool_cpu_map[0].midr = system_cpuinfo.midr.bits;
> > > +    pool_cpu_map[0].pool = cpupool0;
> > > +
> > > +    for_each_cpu ( cpu, cpu_online_map )
> > > +    {
> > > +        for ( i = 0; i < CONFIG_MAX_BOOT_TIME_CPUPOOLS; i++ )
> > > +            if ( !pool_cpu_map[i].pool ||
> > > +                 (cpu_data[cpu].midr.bits == pool_cpu_map[i].midr) )
> > > +                break;
> > > +
> > > +        if ( i < CONFIG_MAX_BOOT_TIME_CPUPOOLS )
> > > +        {
> > > +            if ( !pool_cpu_map[i].pool )
> > > +            {
> > > +                /* There is no pool for this cpu midr, create it */
> > > +                pool_cpu_map[i].midr = cpu_data[cpu].midr.bits;
> > > +                debugtrace_printk("Create pool %u for cpu MIDR: 0x%"
> > > +                                  PRIregister"\n", i,
> > > pool_cpu_map[i].midr);
> > > +                pool_cpu_map[i].pool =
> > > +                    cpupool_create(i, scheduler_get_default()->sched_id);
> > > +                BUG_ON(IS_ERR(pool_cpu_map[i].pool));
> > > +                cpupool_put(pool_cpu_map[i].pool);
> > ... call a new wrapper in common/sched/cpupool.c taking just the id as
> > parameter (e.g. "cpupool *cpupool_create_default(unsigned int id)")
> > which will use the default scheduler and do the cpupool_create() and
> > cpupool_put() calls internally.
> What if sched=something is also passed on the command line? Shouldn't
> that be used for all cpupools? Or maybe sched=something works because it
> changes the "default scheduler"?
Yes.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature