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

[Morten Rasmussen <morten.rasmussen@arm.com>]

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