On 6 March 2014 01:09, Dietmar Eggemann dietmar.eggemann@arm.com wrote:

On 05/03/14 07:18, Vincent Guittot wrote:

...

We replace the old way to configure the scheduler topology with a new method which enables a platform to declare additionnal level (if needed).

We still have a default topology table definition that can be used by platform that don't want more level than the SMT, MC, CPU and NUMA ones. This table can be overwritten by an arch which wants to add new level where a load balance make sense like BOOK or powergating level.

For each level, we need a function pointer that returns cpumask for each cpu, the flags configuration and a name. Each level must be a subset on

Maybe it's worth mentioning here that those flags are from the set of the sd topology flags to distinguish them from the set of sd behavioural flags. Latter can't be set via this interface.

yes, i will add the list of flags that can be set with the table

...

the next one. The build sequence of the sched_domain will take care of removing useless levels like those with 1 CPU and those with the same CPU span and relevant information for load balancing than its child .

Signed-off-by: Vincent Guittot vincent.guittot@linaro.org

arch/ia64/include/asm/topology.h | 24 ---- arch/s390/include/asm/topology.h | 2 - arch/tile/include/asm/topology.h | 33 ------ include/linux/sched.h | 29 +++++ include/linux/topology.h | 128 +++------------------ kernel/sched/core.c | 227 +++++++++++++++++++------------------- 6 files changed, 156 insertions(+), 287 deletions(-)

[snip]

...

-#define for_each_sd_topology(tl) \

•   for (tl = sched_domain_topology; tl->init; tl++)
  

Why is sched_domains_curr_level now outside #ifdef CONFIG_NUMA?

it should not as well as its use in sd_init

...

+static int sched_domains_curr_level;

#ifdef CONFIG_NUMA

• static int sched_domains_numa_levels; static int *sched_domains_numa_distance; static struct cpumask ***sched_domains_numa_masks;

-static int sched_domains_curr_level;

-static inline int sd_local_flags(int level) -{

•   if (sched_domains_numa_distance[level] > RECLAIM_DISTANCE)
  

•           return 0;
  

+#endif

•   return SD_BALANCE_EXEC | SD_BALANCE_FORK | SD_WAKE_AFFINE;
  

-} +/*

• • SD_flags allowed in topology descriptions.
• • SD_SHARE_CPUPOWER - describes SMT topologies
• • SD_SHARE_PKG_RESOURCES - describes shared caches
• • SD_NUMA - describes NUMA topologies
• • Odd one out:
• • SD_ASYM_PACKING - describes SMT quirks
• */

+#define TOPOLOGY_SD_FLAGS \

•   (SD_SHARE_CPUPOWER |            \
  

•    SD_SHARE_PKG_RESOURCES |       \
  

•    SD_NUMA |                      \
  

•    SD_ASYM_PACKING)
  
  

  static struct sched_domain *

-sd_numa_init(struct sched_domain_topology_level *tl, int cpu) +sd_init(struct sched_domain_topology_level *tl, int cpu) { struct sched_domain *sd = *per_cpu_ptr(tl->data.sd, cpu);

•   int level = tl->numa_level;
  

•   int sd_weight = cpumask_weight(
  

sched_domains_numa_masks[level][cpu_to_node(cpu)]);

•   int sd_weight;
  

Next line could be guared by #ifdef CONFIG_NUMA. We still use #ifdef CONFIG_NUMA later in sd_init() though.

...

•   /*
  

•    * Ugly hack to pass state to sd_numa_mask()...
  

•    */
  

•   sched_domains_curr_level = tl->numa_level;
  

•   sd_weight = cpumask_weight(tl->mask(cpu));
  

•   if (WARN_ONCE(tl->sd_flags & ~TOPOLOGY_SD_FLAGS,
  

•                   "wrong sd_flags in topology description\n"))
  

•           tl->sd_flags &= ~TOPOLOGY_SD_FLAGS;
  
     *sd = (struct sched_domain){
             .min_interval           = sd_weight,
             .max_interval           = 2*sd_weight,
             .busy_factor            = 32,
             .imbalance_pct          = 125,
  

•           .cache_nice_tries       = 2,
  

•           .busy_idx               = 3,
  

•           .idle_idx               = 2,
  

•           .cache_nice_tries       = 0,
  

•           .busy_idx               = 0,
  

•           .idle_idx               = 0,
             .newidle_idx            = 0,
             .wake_idx               = 0,
             .forkexec_idx           = 0,
  

Why we want to explicitly set those indexes to 0 here? IMHO, the memory for *sd is zeroed out before. This is true for all data members which are set to 0 later in this function including the | 0*SD_FOO . IMHO, would make the code more readable.

I would say that it makes the configuration more readable and modifiable because you have the list of possible flag to set

...

             .flags                  = 1*SD_LOAD_BALANCE
                                     | 1*SD_BALANCE_NEWIDLE

•                                   | 0*SD_BALANCE_EXEC
  

•                                   | 0*SD_BALANCE_FORK
  

•                                   | 1*SD_BALANCE_EXEC
  

•                                   | 1*SD_BALANCE_FORK
                                     | 0*SD_BALANCE_WAKE
  

•                                   | 0*SD_WAKE_AFFINE
  

•                                   | 1*SD_WAKE_AFFINE
                                     | 0*SD_SHARE_CPUPOWER
                                     | 0*SD_SHARE_PKG_RESOURCES
  

•                                   | 1*SD_SERIALIZE
  

•                                   | 0*SD_SERIALIZE
                                     | 0*SD_PREFER_SIBLING
  

•                                   | 1*SD_NUMA
  

•                                   | sd_local_flags(level)
  

•                                   | 0*SD_NUMA
  

•                                   | tl->sd_flags
                                     ,
  

•            .last_balance           = jiffies,
             .balance_interval       = sd_weight,
  

•           .smt_gain               = 0,
  

•           .max_newidle_lb_cost    = 0,
  

•           .next_decay_max_lb_cost = jiffies,
  

+#ifdef CONFIG_SCHED_DEBUG

•           .name                   = tl->name,
  

+#endif };

•   SD_INIT_NAME(sd, NUMA);
  

•   sd->private = &tl->data;
  
  

[snip]

...

+/*

• • Topology list, bottom-up.
• */

+static struct sched_domain_topology_level default_topology[] = { +#ifdef CONFIG_SCHED_SMT

•   { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES,
  

SD_INIT_NAME(SMT) }, +#endif +#ifdef CONFIG_SCHED_MC

•   { cpu_coregroup_mask, SD_SHARE_PKG_RESOURCES, SD_INIT_NAME(MC) },
  

+#endif +#ifdef CONFIG_SCHED_BOOK

•   { cpu_book_mask, SD_INIT_NAME(BOOK) },
  

+#endif

Never got the new name DIE for CPU? Might confuse people when they use /proc/sys/kernel/sched_domain/cpuX/domainY/name or sched_domain_debug_one().

In fact, CPU is also confusing because it's used for different things. But if it makes things even more confusing, i can come back to CPU

...

•   { cpu_cpu_mask, SD_INIT_NAME(DIE) },
  

•   { NULL, },
  

+};

+struct sched_domain_topology_level *sched_domain_topology = default_topology;

On 11 March 2014 11:31, Peter Zijlstra peterz@infradead.org wrote:

On Thu, Mar 06, 2014 at 04:32:35PM +0800, Vincent Guittot wrote:

...

...

Never got the new name DIE for CPU? Might confuse people when they use /proc/sys/kernel/sched_domain/cpuX/domainY/name or sched_domain_debug_one().

In fact, CPU is also confusing because it's used for different things. But if it makes things even more confusing, i can come back to CPU

Yeah, not sure DIE is the right thing either; because there's multi-die packages that get classified under CPU :-)

Take for example the Core 2 Quad, which was 2 dual core dies glued together in a single package.

There's also the AMD bulldozer which glued two dies into a single package; but for those its not a problem because each die is a separate numa node, so there DIE would actually be the correct term and PACKAGE would be wrong.

So while CPU sucks, I'm not sure we can come up with anything that's actually correct. That said; we could try for something less wrong than CPU :-)

OK

Dietmar,

Have you got another naming that DIE that could suit better ? otherwise i will keep it

Vincent

I'm not sure there are a lot of people who see/know the names of these domains to be bothered by a change in them; it might be limited to just us for all I know.

---

linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 03/11/2014 06:48 PM, Vincent Guittot wrote:

On 11 March 2014 11:08, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:

...

Hi Vincent,

On 03/05/2014 12:48 PM, Vincent Guittot wrote:

...

Create a dedicated topology table for handling asymetric feature. The current proposal creates a new level which describes which groups of CPUs take adavantge of SD_ASYM_PACKING. The useless level will be removed during the build of the sched_domain topology.

Another solution would be to set SD_ASYM_PACKING in the sd_flags of SMT level during the boot sequence and before the build of the sched_domain topology.

Is the below what you mean as the other solution? If it is so, I would strongly recommend this approach rather than adding another level to the topology level to represent the asymmetric behaviour.

+static struct sched_domain_topology_level powerpc_topology[] = { +#ifdef CONFIG_SCHED_SMT

•   { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES,
  

SD_INIT_NAME(SMT) | arch_sd_sibling_asym_packing() }, +#endif

•   { cpu_cpu_mask, SD_INIT_NAME(DIE) },
  

•   { NULL, },
  

+};

Not exactly like that but something like below

+static struct sched_domain_topology_level powerpc_topology[] = { +#ifdef CONFIG_SCHED_SMT

• { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES,

SD_INIT_NAME(SMT) }, +#endif

• { cpu_cpu_mask, SD_INIT_NAME(DIE) },
• { NULL, },

+};

+static void __init set_sched_topology(void) +{ +#ifdef CONFIG_SCHED_SMT

• powerpc_topology[0].sd_flags |= arch_sd_sibling_asym_packing();

+#endif

• sched_domain_topology = powerpc_topology;

+}

I think we can set it in powerpc_topology[] and not bother about setting additional flags outside of this array. It is clearer this way.

On an additional note, on powerpc we would want to clear the SD_SHARE_POWERDOMAIN flag at the CPU domain. On Power8, considering we have 8 threads per core, we would want to consolidate tasks atleast upto 4 threads without significant performance impact before spilling over to the other cores. By doing so, besides making use of the higher power of the core we could do cpuidle management at the core level for the remaining idle cores as a result of this consolidation.

So the powerpc_topology[] would be something like the below:

+static struct sched_domain_topology_level powerpc_topology[] = { +#ifdef CONFIG_SCHED_SMT + { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES, SD_INIT_NAME(SMT) | arch_sd_sibling_asym_packing() | SD_SHARE_POWERDOMAIN }, +#endif + { cpu_cpu_mask, SD_INIT_NAME(DIE) }, + { NULL, }, +};

The amount of consolidation to the threads in a core, we will probably take care of it in cpu capacity or a similar parameter, but the above topology level would be required to request the scheduler to try consolidating tasks to cores till the cpu capacity(3/4/5 threads) has exceeded.

Regards Preeti U Murthy

On 12/03/14 07:44, Vincent Guittot wrote:

On 12 March 2014 05:42, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:

...

On 03/11/2014 06:48 PM, Vincent Guittot wrote:

...

On 11 March 2014 11:08, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:

...

Hi Vincent,

On 03/05/2014 12:48 PM, Vincent Guittot wrote:

...

Create a dedicated topology table for handling asymetric feature. The current proposal creates a new level which describes which groups of CPUs take adavantge of SD_ASYM_PACKING. The useless level will be removed during the build of the sched_domain topology.

Another solution would be to set SD_ASYM_PACKING in the sd_flags of SMT level during the boot sequence and before the build of the sched_domain topology.

Is the below what you mean as the other solution? If it is so, I would strongly recommend this approach rather than adding another level to the topology level to represent the asymmetric behaviour.

+static struct sched_domain_topology_level powerpc_topology[] = { +#ifdef CONFIG_SCHED_SMT

•   { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES,
  

SD_INIT_NAME(SMT) | arch_sd_sibling_asym_packing() }, +#endif

•   { cpu_cpu_mask, SD_INIT_NAME(DIE) },
  

•   { NULL, },
  

+};

Not exactly like that but something like below

+static struct sched_domain_topology_level powerpc_topology[] = { +#ifdef CONFIG_SCHED_SMT

• { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES,

SD_INIT_NAME(SMT) }, +#endif

• { cpu_cpu_mask, SD_INIT_NAME(DIE) },
• { NULL, },

+};

+static void __init set_sched_topology(void) +{ +#ifdef CONFIG_SCHED_SMT

• powerpc_topology[0].sd_flags |= arch_sd_sibling_asym_packing();

+#endif

• sched_domain_topology = powerpc_topology;

+}

I think we can set it in powerpc_topology[] and not bother about setting additional flags outside of this array. It is clearer this way.

IIRC, the arch_sd_sibling_asym_packing of powerpc can be set at runtime which prevents it from being put directly in the table. Or it means that we should use a function pointer in the table for setting flags instead of a static value like the current proposal.

Right,

static struct sched_domain_topology_level powerpc_topology[] = { #ifdef CONFIG_SCHED_SMT { cpu_asmt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING, SD_INIT_NAME(ASMT) }, { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES | arch_sd_sibling_asym_packing() | SD_SHARE_POWERDOMAIN, SD_INIT_NAME(SMT) }, #endif { cpu_cpu_mask, SD_INIT_NAME(DIE) }, { NULL, }, };

is not compiling:

CC arch/powerpc/kernel/smp.o arch/powerpc/kernel/smp.c:787:2: error: initializer element is not constant arch/powerpc/kernel/smp.c:787:2: error: (near initialization for 'powerpc_topology[1].sd_flags')

So I'm in favour of a function pointer, even w/o the 'int cpu' parameter to circumvent the issue that it is too easy to create broken sd setups.

-- Dietmar

...

On an additional note, on powerpc we would want to clear the SD_SHARE_POWERDOMAIN flag at the CPU domain. On Power8, considering we have 8 threads per core, we would want to consolidate tasks atleast upto 4 threads without significant performance impact before spilling over to the other cores. By doing so, besides making use of the higher power of the core we could do cpuidle management at the core level for the remaining idle cores as a result of this consolidation.

OK. i will add the SD_SHARE_POWERDOMAIN like below

Thanks Vincent

...

So the powerpc_topology[] would be something like the below:

+static struct sched_domain_topology_level powerpc_topology[] = { +#ifdef CONFIG_SCHED_SMT

•   { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES,
  

SD_INIT_NAME(SMT) | arch_sd_sibling_asym_packing() | SD_SHARE_POWERDOMAIN }, +#endif

•   { cpu_cpu_mask, SD_INIT_NAME(DIE) },
  

•   { NULL, },
  

+};

The amount of consolidation to the threads in a core, we will probably take care of it in cpu capacity or a similar parameter, but the above topology level would be required to request the scheduler to try consolidating tasks to cores till the cpu capacity(3/4/5 threads) has exceeded.

Regards Preeti U Murthy

On 03/12/2014 01:14 PM, Vincent Guittot wrote:

On 12 March 2014 05:42, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:

...

On 03/11/2014 06:48 PM, Vincent Guittot wrote:

...

On 11 March 2014 11:08, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:

...

Hi Vincent,

On 03/05/2014 12:48 PM, Vincent Guittot wrote:

...

Create a dedicated topology table for handling asymetric feature. The current proposal creates a new level which describes which groups of CPUs take adavantge of SD_ASYM_PACKING. The useless level will be removed during the build of the sched_domain topology.

Another solution would be to set SD_ASYM_PACKING in the sd_flags of SMT level during the boot sequence and before the build of the sched_domain topology.

Is the below what you mean as the other solution? If it is so, I would strongly recommend this approach rather than adding another level to the topology level to represent the asymmetric behaviour.

+static struct sched_domain_topology_level powerpc_topology[] = { +#ifdef CONFIG_SCHED_SMT

•   { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES,
  

SD_INIT_NAME(SMT) | arch_sd_sibling_asym_packing() }, +#endif

•   { cpu_cpu_mask, SD_INIT_NAME(DIE) },
  

•   { NULL, },
  

+};

Not exactly like that but something like below

+static struct sched_domain_topology_level powerpc_topology[] = { +#ifdef CONFIG_SCHED_SMT

• { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES,

SD_INIT_NAME(SMT) }, +#endif

• { cpu_cpu_mask, SD_INIT_NAME(DIE) },
• { NULL, },

+};

+static void __init set_sched_topology(void) +{ +#ifdef CONFIG_SCHED_SMT

• powerpc_topology[0].sd_flags |= arch_sd_sibling_asym_packing();

+#endif

• sched_domain_topology = powerpc_topology;

+}

I think we can set it in powerpc_topology[] and not bother about setting additional flags outside of this array. It is clearer this way.

IIRC, the arch_sd_sibling_asym_packing of powerpc can be set at runtime which prevents it from being put directly in the table. Or it means that we should use a function pointer in the table for setting flags instead of a static value like the current proposal.

Oh yes you are right. Then the above looks good to me. So we define the table as usual and set the asymmetric flag in set_sched_topology().

...

On an additional note, on powerpc we would want to clear the SD_SHARE_POWERDOMAIN flag at the CPU domain. On Power8, considering we have 8 threads per core, we would want to consolidate tasks atleast upto 4 threads without significant performance impact before spilling over to the other cores. By doing so, besides making use of the higher power of the core we could do cpuidle management at the core level for the remaining idle cores as a result of this consolidation.

OK. i will add the SD_SHARE_POWERDOMAIN like below

Thanks!

Regards Preeti U Murthy

Thanks Vincent

...

So the powerpc_topology[] would be something like the below:

+static struct sched_domain_topology_level powerpc_topology[] = { +#ifdef CONFIG_SCHED_SMT

•   { cpu_smt_mask, SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES,
  

SD_INIT_NAME(SMT) | arch_sd_sibling_asym_packing() | SD_SHARE_POWERDOMAIN }, +#endif

•   { cpu_cpu_mask, SD_INIT_NAME(DIE) },
  

•   { NULL, },
  

+};

The amount of consolidation to the threads in a core, we will probably take care of it in cpu capacity or a similar parameter, but the above topology level would be required to request the scheduler to try consolidating tasks to cores till the cpu capacity(3/4/5 threads) has exceeded.

Regards Preeti U Murthy

On 6 March 2014 07:17, Dietmar Eggemann dietmar.eggemann@arm.com wrote:

On 05/03/14 07:18, Vincent Guittot wrote:

...

This patchset was previously part of the larger tasks packing patchset [1]. I have splitted the latter in 3 different patchsets (at least) to make the thing easier. -configuration of sched_domain topology (this patchset) -update and consolidation of cpu_power -tasks packing algorithm

Based on Peter Z's proposal [2][3], this patchset modifies the way to configure the sched_domain level in order to let architectures to add specific level like the current BOOK level or the proposed power gating level for ARM architecture.

[1] https://lkml.org/lkml/2013/10/18/121 [2] https://lkml.org/lkml/2013/11/5/239 [3] https://lkml.org/lkml/2013/11/5/449

Vincent Guittot (6): sched: remove unused SCHED_INIT_NODE sched: rework of sched_domain topology definition sched: s390: create a dedicated topology table sched: powerpc: create a dedicated topology table sched: add a new SD_SHARE_POWERDOMAIN for sched_domain sched: ARM: create a dedicated scheduler topology table

arch/arm/kernel/topology.c | 26 ++++ arch/ia64/include/asm/topology.h | 24 ---- arch/metag/include/asm/topology.h | 27 ----- arch/powerpc/kernel/smp.c | 35 ++++-- arch/s390/include/asm/topology.h | 13 +- arch/s390/kernel/topology.c | 25 ++++ arch/tile/include/asm/topology.h | 33 ------ include/linux/sched.h | 30 +++++ include/linux/topology.h | 128 ++------------------ kernel/sched/core.c | 235 ++++++++++++++++++------------------- 10 files changed, 237 insertions(+), 339 deletions(-)

Hi Vincent,

I reviewed your patch-set carefully (including test runs on TC2), especially due to the fact that we want to build our sd_energy stuff on top of it.

Thanks

One thing I'm still not convinced of is the fact that specifying additional sd levels in the struct sched_domain_topology_level table has an advantage over a function pointer for sd topology flags similar to the one we're already using for the cpu mask in struct sched_domain_topology_level.

int (*sched_domain_flags_f)(int cpu);

We have to create additional level for some kind of topology as described in my trial https://lkml.org/lkml/2013/12/18/279 which is not possible with function pointer.

Have you got a situation in mind where it will be necessary to use the function pointer with cpu number as an argument ?

In the current example of this patchset, the flags are statically set in the table but nothing prevents an architecture to update the flags value before being given to the scheduler

This function pointer would be simply another member of struct sched_domain_topology_level and would replace int sd_flags. AFAICS, you have to create additional cpu mask functions anyway for the additional sd levels, like cpu_corepower_mask() for the GMC level in the ARM case. There could be a set of standard sd topology flags function for the default sd layer and archs which want to pass in something special define those function locally since they will use them only in their arch specific struct sched_domain_topology_level table anyway. I know that you use the existing sd degenerate functionality for this and that the freeing of the redundant data structures (sched_domain, sched_group and sched_group_power) is there too but it still doesn't seem to me to be the right thing to do.

The problem that we now expose internal data structures (struct sd_data and struct sched_domain_topology_level) could be dealt with later.

-- Dietmar

On 6 March 2014 20:31, Dietmar Eggemann dietmar.eggemann@arm.com wrote:

On 06/03/14 09:04, Vincent Guittot wrote:

...

On 6 March 2014 07:17, Dietmar Eggemann dietmar.eggemann@arm.com wrote:

...

On 05/03/14 07:18, Vincent Guittot wrote:

...

This patchset was previously part of the larger tasks packing patchset [1]. I have splitted the latter in 3 different patchsets (at least) to make the thing easier. -configuration of sched_domain topology (this patchset) -update and consolidation of cpu_power -tasks packing algorithm

Based on Peter Z's proposal [2][3], this patchset modifies the way to configure the sched_domain level in order to let architectures to add specific level like the current BOOK level or the proposed power gating level for ARM architecture.

[1] https://lkml.org/lkml/2013/10/18/121 [2] https://lkml.org/lkml/2013/11/5/239 [3] https://lkml.org/lkml/2013/11/5/449

Vincent Guittot (6): sched: remove unused SCHED_INIT_NODE sched: rework of sched_domain topology definition sched: s390: create a dedicated topology table sched: powerpc: create a dedicated topology table sched: add a new SD_SHARE_POWERDOMAIN for sched_domain sched: ARM: create a dedicated scheduler topology table

arch/arm/kernel/topology.c | 26 ++++ arch/ia64/include/asm/topology.h | 24 ---- arch/metag/include/asm/topology.h | 27 ----- arch/powerpc/kernel/smp.c | 35 ++++-- arch/s390/include/asm/topology.h | 13 +- arch/s390/kernel/topology.c | 25 ++++ arch/tile/include/asm/topology.h | 33 ------ include/linux/sched.h | 30 +++++ include/linux/topology.h | 128 ++------------------ kernel/sched/core.c | 235 ++++++++++++++++++------------------- 10 files changed, 237 insertions(+), 339 deletions(-)

Hi Vincent,

I reviewed your patch-set carefully (including test runs on TC2), especially due to the fact that we want to build our sd_energy stuff on top of it.

Thanks

...

One thing I'm still not convinced of is the fact that specifying additional sd levels in the struct sched_domain_topology_level table has an advantage over a function pointer for sd topology flags similar to the one we're already using for the cpu mask in struct sched_domain_topology_level.

int (*sched_domain_flags_f)(int cpu);

We have to create additional level for some kind of topology as described in my trial https://lkml.org/lkml/2013/12/18/279 which is not possible with function pointer.

This is what I don't understand at the moment. In your example in the link above, (2 cluster of 4 cores with SMT), cpu 0-7 can powergate while cpu 8-15 can't). Why can't we have

The 2nd example is a bit more complicated and needs an additional level to describe the topology

static inline int cpu_coregroup_flags(int cpu) { int flags = SD_SHARE_PKG_RESOURCES;

if (cpu >= 8)
    flags |= SD_SHARE_POWERDOMAIN;

return flags;

}

inside the arch specific topology file and use it in the MC level as the int (*sched_domain_flags_f)(int cpu) member of struct sched_domain_topology_level?

...

Have you got a situation in mind where it will be necessary to use the function pointer with cpu number as an argument ?

The one above. Fundamentally all situations where you want to set sd topology flags differently for different cpus in the same sd level. big.LITTLE is another example but it's the same as powergated/!powergated in this perspective.

You see the flag of a level as something that can be changed per cpu whereas the proposal is to define a number of level with interesting properties for the scheduler and to associate a mask of cpus to this properties.

I don't have a strong opinion about using or not a cpu argument for setting the flags of a level (it was part of the initial proposal before we start to completely rework the build of sched_domain) Nevertheless, I see one potential concern that you can have completely different flags configuration of the same sd level of 2 cpus.

Peter,

Was the use of the cpu as a parameter in the initialization of sched_domain's flag a reason for asking for reworking the initialization of sched_domain ?

...

In the current example of this patchset, the flags are statically set in the table but nothing prevents an architecture to update the flags value before being given to the scheduler

What will be the infrastructure if the arch wants to do so?

I'm not sure to catch your point. The arch is now able to defines its own table and fill it before giving it to the scheduler so nothing prevents to set/update the flags field according the system configuration during boot sequence.

Thanks, Vincent

Thanks,

-- Dietmar

...

...

This function pointer would be simply another member of struct sched_domain_topology_level and would replace int sd_flags. AFAICS, you have to create additional cpu mask functions anyway for the additional sd levels, like cpu_corepower_mask() for the GMC level in the ARM case. There could be a set of standard sd topology flags function for the default sd layer and archs which want to pass in something special define those function locally since they will use them only in their arch specific struct sched_domain_topology_level table anyway. I know that you use the existing sd degenerate functionality for this and that the freeing of the redundant data structures (sched_domain, sched_group and sched_group_power) is there too but it still doesn't seem to me to be the right thing to do.

The problem that we now expose internal data structures (struct sd_data and struct sched_domain_topology_level) could be dealt with later.

-- Dietmar

On 8 March 2014 13:40, Dietmar Eggemann dietmar.eggemann@arm.com wrote:

On 07/03/14 02:47, Vincent Guittot wrote:

...

On 6 March 2014 20:31, Dietmar Eggemann dietmar.eggemann@arm.com wrote:

...

On 06/03/14 09:04, Vincent Guittot wrote:

...

On 6 March 2014 07:17, Dietmar Eggemann dietmar.eggemann@arm.com wrote:

...

On 05/03/14 07:18, Vincent Guittot wrote:

...

This patchset was previously part of the larger tasks packing patchset [1]. I have splitted the latter in 3 different patchsets (at least) to make the thing easier. -configuration of sched_domain topology (this patchset) -update and consolidation of cpu_power -tasks packing algorithm

Based on Peter Z's proposal [2][3], this patchset modifies the way to configure the sched_domain level in order to let architectures to add specific level like the current BOOK level or the proposed power gating level for ARM architecture.

[1] https://lkml.org/lkml/2013/10/18/121 [2] https://lkml.org/lkml/2013/11/5/239 [3] https://lkml.org/lkml/2013/11/5/449

Vincent Guittot (6): sched: remove unused SCHED_INIT_NODE sched: rework of sched_domain topology definition sched: s390: create a dedicated topology table sched: powerpc: create a dedicated topology table sched: add a new SD_SHARE_POWERDOMAIN for sched_domain sched: ARM: create a dedicated scheduler topology table

arch/arm/kernel/topology.c        |   26 ++++
arch/ia64/include/asm/topology.h  |   24 ----
arch/metag/include/asm/topology.h |   27 -----
arch/powerpc/kernel/smp.c         |   35 ++++--
arch/s390/include/asm/topology.h  |   13 +-
arch/s390/kernel/topology.c       |   25 ++++
arch/tile/include/asm/topology.h  |   33 ------
include/linux/sched.h             |   30 +++++
include/linux/topology.h          |  128 ++------------------
kernel/sched/core.c               |  235

++++++++++++++++++------------------- 10 files changed, 237 insertions(+), 339 deletions(-)

Hi Vincent,

I reviewed your patch-set carefully (including test runs on TC2), especially due to the fact that we want to build our sd_energy stuff on top of it.

Thanks

...

One thing I'm still not convinced of is the fact that specifying additional sd levels in the struct sched_domain_topology_level table has an advantage over a function pointer for sd topology flags similar to the one we're already using for the cpu mask in struct sched_domain_topology_level.

int (*sched_domain_flags_f)(int cpu);

We have to create additional level for some kind of topology as described in my trial https://lkml.org/lkml/2013/12/18/279 which is not possible with function pointer.

This is what I don't understand at the moment. In your example in the link above, (2 cluster of 4 cores with SMT), cpu 0-7 can powergate while cpu 8-15 can't). Why can't we have

The 2nd example is a bit more complicated and needs an additional level to describe the topology

I see. In the 2. example you want to have an additional MC level for cpu 2-7 because you want to do load balance between those cpus more often than for cpu 0-7 for dst cpus from the set (2-7). Not sure if such topologies (only a subset of cpus inside a cluster can't be powergated) exists today in the real world though?

Be sure that such topology is studied and considered by HW guys

from https://lkml.org/lkml/2013/12/18/279:

CPU2: domain 0: span 2-3 level: SMT flags: SD_SHARE_CPUPOWER | SD_SHARE_PKG_RESOURCES | SD_SHARE_POWERDOMAIN groups: 0 1 <-- Doesn't this have to be 'groups: 2 3' domain 1: span 2-7 level: MC flags: SD_SHARE_PKG_RESOURCES | SD_SHARE_POWERDOMAIN groups: 2-7 4-5 6-7 domain 2: span 0-7 level: MC flags: SD_SHARE_PKG_RESOURCES groups: 2-7 0-1 domain 3: span 0-15 level: CPU flags: groups: 0-7 8-15

...

...

static inline int cpu_coregroup_flags(int cpu) { int flags = SD_SHARE_PKG_RESOURCES;

 if (cpu >= 8)
     flags |= SD_SHARE_POWERDOMAIN;

 return flags;

}

inside the arch specific topology file and use it in the MC level as the int (*sched_domain_flags_f)(int cpu) member of struct sched_domain_topology_level?

...

Have you got a situation in mind where it will be necessary to use the function pointer with cpu number as an argument ?

The one above. Fundamentally all situations where you want to set sd topology flags differently for different cpus in the same sd level. big.LITTLE is another example but it's the same as powergated/!powergated in this perspective.

You see the flag of a level as something that can be changed per cpu whereas the proposal is to define a number of level with interesting properties for the scheduler and to associate a mask of cpus to this properties.

That's right.

...

I don't have a strong opinion about using or not a cpu argument for setting the flags of a level (it was part of the initial proposal before we start to completely rework the build of sched_domain) Nevertheless, I see one potential concern that you can have completely different flags configuration of the same sd level of 2 cpus.

Could you elaborate a little bit further regarding the last sentence? Do you think that those completely different flags configuration would make it impossible, that the load-balance code could work at all at this sd?

With such function, an arch could set differently some topology flags like SD_SHARE_PKG_RESOURCES and SD_SHARE_CPUPOWER in the same sd level which make the notion of sd level meaningless.

...

Peter,

Was the use of the cpu as a parameter in the initialization of sched_domain's flag a reason for asking for reworking the initialization of sched_domain ?

...

...

In the current example of this patchset, the flags are statically set in the table but nothing prevents an architecture to update the flags value before being given to the scheduler

What will be the infrastructure if the arch wants to do so?

I'm not sure to catch your point. The arch is now able to defines its own table and fill it before giving it to the scheduler so nothing prevents to set/update the flags field according the system configuration during boot sequence.

Sorry, misunderstanding from my side in the first place. You just said that the arch is able to change those flags (due to the arch specific topology table) and I related it to the possibility for the arch to set the topology flags per cpu.

OK, i was not sure that your point was only about the cpu argument for the SD's flags configuration

Vincent

...

Thanks, Vincent

...

Thanks,

-- Dietmar

...

...

This function pointer would be simply another member of struct sched_domain_topology_level and would replace int sd_flags. AFAICS, you have to create additional cpu mask functions anyway for the additional sd levels, like cpu_corepower_mask() for the GMC level in the ARM case. There could be a set of standard sd topology flags function for the default sd layer and archs which want to pass in something special define those function locally since they will use them only in their arch specific struct sched_domain_topology_level table anyway. I know that you use the existing sd degenerate functionality for this and that the freeing of the redundant data structures (sched_domain, sched_group and sched_group_power) is there too but it still doesn't seem to me to be the right thing to do.

The problem that we now expose internal data structures (struct sd_data and struct sched_domain_topology_level) could be dealt with later.

-- Dietmar

On 11/03/14 13:17, Peter Zijlstra wrote:

On Sat, Mar 08, 2014 at 12:40:58PM +0000, Dietmar Eggemann wrote:

...

...

I don't have a strong opinion about using or not a cpu argument for setting the flags of a level (it was part of the initial proposal before we start to completely rework the build of sched_domain) Nevertheless, I see one potential concern that you can have completely different flags configuration of the same sd level of 2 cpus.

Could you elaborate a little bit further regarding the last sentence? Do you think that those completely different flags configuration would make it impossible, that the load-balance code could work at all at this sd?

So a problem with such an interfaces is that is makes it far too easy to generate completely broken domains.

I see the point. What I'm still struggling with is to understand why this interface is worse then the one where we set-up additional, adjacent sd levels with new cpu_foo_mask functions plus different static sd-flags configurations and rely on the sd degenerate functionality in the core scheduler to fold these levels together to achieve different per cpu sd flags configurations.

IMHO, exposing struct sched_domain_topology_level bar_topology[] to the arch is the reason why the core scheduler has to check if the arch provides a sane sd setup in both cases.

You can, for two cpus in the same domain provide, different flags; such a configuration doesn't make any sense at all.

Now I see why people would like to have this; but unless we can make it robust I'd be very hesitant to go this route.

By making it robust, I guess you mean that the core scheduler has to check that the provided set-ups are sane, something like the following code snippet in sd_init()

if (WARN_ONCE(tl->sd_flags & ~TOPOLOGY_SD_FLAGS, "wrong sd_flags in topology description\n")) tl->sd_flags &= ~TOPOLOGY_SD_FLAGS;

but for per cpu set-up's. Obviously, this check has to be in sync with the usage of these flags in the core scheduler algorithms. This comprises probably that a subset of these topology sd flags has to be set for all cpus in a sd level whereas other can be set only for some cpus.

On 12/03/14 13:47, Vincent Guittot wrote:

On 12 March 2014 14:28, Dietmar Eggemann dietmar.eggemann@arm.com wrote:

...

On 11/03/14 13:17, Peter Zijlstra wrote:

...

On Sat, Mar 08, 2014 at 12:40:58PM +0000, Dietmar Eggemann wrote:

...

...

I don't have a strong opinion about using or not a cpu argument for setting the flags of a level (it was part of the initial proposal before we start to completely rework the build of sched_domain) Nevertheless, I see one potential concern that you can have completely different flags configuration of the same sd level of 2 cpus.

Could you elaborate a little bit further regarding the last sentence? Do you think that those completely different flags configuration would make it impossible, that the load-balance code could work at all at this sd?

So a problem with such an interfaces is that is makes it far too easy to generate completely broken domains.

I see the point. What I'm still struggling with is to understand why this interface is worse then the one where we set-up additional, adjacent sd levels with new cpu_foo_mask functions plus different static sd-flags configurations and rely on the sd degenerate functionality in the core scheduler to fold these levels together to achieve different per cpu sd flags configurations.

The main difference is that all CPUs has got the same levels at the initial state and then the degenerate sequence can decide that it's worth removing a level and if it will not create unsuable domains.

Agreed. But what I'm trying to say is that using the approach of multiple adjacent sd levels with different cpu_mask(int cpu) functions and static sd topology flags will not prevent us from coding the enforcement of sane sd topology flags set-ups somewhere inside the core scheduler.

It is possible to easily introduce erroneous set-ups from the standpoint of sd topology flags with this approach too.

For the sake of an example on ARM TC2 platform, I changed cpu_corepower_mask(int cpu) [arch/arm/kernel/topology.c] to simulate that in socket 1 (3 Cortex-A7) cores can powergate individually whereas in socket 0 (2 Cortex A15) they can't:

const struct cpumask *cpu_corepower_mask(int cpu) { - return &cpu_topology[cpu].thread_sibling; + return cpu_topology[cpu].socket_id ? &cpu_topology[cpu].thread_sibling : + &cpu_topology[cpu].core_sibling; }

With this I get the following cpu mask configuration:

dmesg snippet (w/ additional debug in cpu_coregroup_mask(), cpu_corepower_mask()):

... CPU0: cpu_corepower_mask=0-1 CPU0: cpu_coregroup_mask=0-1 CPU1: cpu_corepower_mask=0-1 CPU1: cpu_coregroup_mask=0-1 CPU2: cpu_corepower_mask=2 CPU2: cpu_coregroup_mask=2-4 CPU3: cpu_corepower_mask=3 CPU3: cpu_coregroup_mask=2-4 CPU4: cpu_corepower_mask=4 CPU4: cpu_coregroup_mask=2-4 ...

And I deliberately introduced the following error into the arm_topology[] table:

static struct sched_domain_topology_level arm_topology[] = { #ifdef CONFIG_SCHED_MC - { cpu_corepower_mask, SD_SHARE_PKG_RESOURCES | SD_SHARE_POWERDOMAIN, SD_INIT_NAME(GMC) }, + { cpu_corepower_mask, SD_SHARE_POWERDOMAIN, SD_INIT_NAME(GMC) }, { cpu_coregroup_mask, SD_SHARE_PKG_RESOURCES, SD_INIT_NAME(MC) },

With this set-up, I get GMC & DIE level for CPU0,1 and MC & DIE level for CPU2,3,4, i.e. the SD_SHARE_PKG_RESOURCES flag is only set for CPU2,3,4 and MC level.

dmesg snippet (w/ adapted sched_domain_debug_one(), only CPU0 and CPU2 shown here):

... CPU0 attaching sched-domain: domain 0: span 0-1 level GMC SD_LOAD_BALANCE SD_BALANCE_NEWIDLE SD_BALANCE_EXEC SD_BALANCE_FORK SD_WAKE_AFFINE SD_SHARE_POWERDOMAIN SD_PREFER_SIBLING groups: 0 1 ... domain 1: span 0-4 level DIE SD_LOAD_BALANCE SD_BALANCE_NEWIDLE SD_BALANCE_EXEC SD_BALANCE_FORK SD_WAKE_AFFINE SD_PREFER_SIBLING groups: 0-1 (cpu_power = 2048) 2-4 (cpu_power = 3072) ... CPU2 attaching sched-domain: domain 0: span 2-4 level MC SD_LOAD_BALANCE SD_BALANCE_NEWIDLE SD_BALANCE_EXEC SD_BALANCE_FORK SD_WAKE_AFFINE SD_SHARE_PKG_RESOURCES groups: 2 3 4 ... domain 1: span 0-4 level DIE SD_LOAD_BALANCE SD_BALANCE_NEWIDLE SD_BALANCE_EXEC SD_BALANCE_FORK SD_WAKE_AFFINE SD_PREFER_SIBLING groups: 2-4 (cpu_power = 3072) 0-1 (cpu_power = 2048) ...

What I wanted to say is IMHO, it doesn't matter which approach we take (multiple adjacent sd levels or per-cpu topo sd flag function), we have to enforce sane sd topology flags set-up inside the core scheduler anyway.

-- Dietmar

...

IMHO, exposing struct sched_domain_topology_level bar_topology[] to the arch is the reason why the core scheduler has to check if the arch provides a sane sd setup in both cases.

...

You can, for two cpus in the same domain provide, different flags; such a configuration doesn't make any sense at all.

Now I see why people would like to have this; but unless we can make it robust I'd be very hesitant to go this route.

By making it robust, I guess you mean that the core scheduler has to check that the provided set-ups are sane, something like the following code snippet in sd_init()

if (WARN_ONCE(tl->sd_flags & ~TOPOLOGY_SD_FLAGS, "wrong sd_flags in topology description\n")) tl->sd_flags &= ~TOPOLOGY_SD_FLAGS;

but for per cpu set-up's. Obviously, this check has to be in sync with the usage of these flags in the core scheduler algorithms. This comprises probably that a subset of these topology sd flags has to be set for all cpus in a sd level whereas other can be set only for some cpus.

[...]

On Fri, Mar 07, 2014 at 10:47:49AM +0800, Vincent Guittot wrote:

Peter,

Was the use of the cpu as a parameter in the initialization of sched_domain's flag a reason for asking for reworking the initialization of sched_domain ?

/me tries very hard to remember.. and fails.

Reading back the linked thread also doesn't seem to jog memory.