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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.