On 12/03/14 07:44, Vincent Guittot wrote: > On 12 March 2014 05:42, Preeti U Murthy <pre...@linux.vnet.ibm.com> wrote: >> On 03/11/2014 06:48 PM, Vincent Guittot wrote: >>> On 11 March 2014 11:08, Preeti U Murthy <pre...@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 >> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/