Re: [PATCH v3 05/13] sched: Introduce SD_ASYM_CPUCAPACITY sched_domain topology flag

From: Morten Rasmussen
Date: Mon Aug 15 2016 - 07:43:11 EST


On Mon, Aug 15, 2016 at 12:54:59PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 25, 2016 at 02:34:22PM +0100, Morten Rasmussen wrote:
> > @@ -6336,14 +6338,16 @@ static int sched_domains_curr_level;
> > * SD_NUMA - describes NUMA topologies
> > * SD_SHARE_POWERDOMAIN - describes shared power domain
> > *
> > - * Odd one out:
> > + * Odd ones out:
> > * SD_ASYM_PACKING - describes SMT quirks
> > + * SD_ASYM_CPUCAPACITY - describes mixed capacity topologies
> > */
>
> So I'm not sure the new CPUCAPACITY is 'odd'.
>
> That said, the comment is very terse and doesn't explain why PACKING is
> odd.
>
> IIRC the distinction is that the 'normal' ones only describe topology,
> while the ASYM_PACKING one also prescribes behaviour. It is odd in the
> way that it doesn't only describe things.
>
> This ASYM_CPUCAPACITY otoh is purely descriptive, it doesn't prescribe
> how to deal with it.

I think I initially put it in as an 'odd' flag due to the somewhat
strange semantics in the previous versions, but now that it is fixed I
agree that it belongs together with purely descriptive flags.

>
> Does something like so clarify things?
>
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6355,13 +6355,19 @@ static int sched_domains_curr_level;
> /*
> * SD_flags allowed in topology descriptions.
> *
> - * SD_SHARE_CPUCAPACITY - describes SMT topologies
> - * SD_SHARE_PKG_RESOURCES - describes shared caches
> - * SD_NUMA - describes NUMA topologies
> - * SD_SHARE_POWERDOMAIN - describes shared power domain
> + * These flags are purely descriptive of the topology and do not prescribe
> + * behaviour. Behaviour is artificial and mapped in the below sd_init()
> + * function:
> *
> - * Odd one out:
> - * SD_ASYM_PACKING - describes SMT quirks
> + * SD_SHARE_CPUCAPACITY - describes SMT topologies
> + * SD_SHARE_PKG_RESOURCES - describes shared caches
> + * SD_NUMA - describes NUMA topologies
> + * SD_SHARE_POWERDOMAIN - describes shared power domain
> + *
> + * Odd one out, which beside describing the topology has a quirk also
> + * prescribes the desired behaviour that goes along with it:
> + *
> + * SD_ASYM_PACKING - describes SMT quirks
> */
> #define TOPOLOGY_SD_FLAGS \
> (SD_SHARE_CPUCAPACITY | \

I like it :)

Morten