[v3,1/4] sched/fair: add SD_CLUSTER in comments

  From: Alex Shi <alexs@kernel.org>

The description of SD_CLUSTER is missing. Add it.

Signed-off-by: Alex Shi <alexs@kernel.org>
To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: Valentin Schneider <vschneid@redhat.com>
To: Vincent Guittot <vincent.guittot@linaro.org>
To: Juri Lelli <juri.lelli@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
To: Ingo Molnar <mingo@redhat.com>
---
 kernel/sched/topology.c | 1 +
 1 file changed, 1 insertion(+)
  

Subject nit: the prefix should be sched/topology

On 01/02/24 19:54, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
>
> The description of SD_CLUSTER is missing. Add it.
>
> Signed-off-by: Alex Shi <alexs@kernel.org>
> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> To: Valentin Schneider <vschneid@redhat.com>
> To: Vincent Guittot <vincent.guittot@linaro.org>
> To: Juri Lelli <juri.lelli@redhat.com>
> To: Peter Zijlstra <peterz@infradead.org>
> To: Ingo Molnar <mingo@redhat.com>
> ---
>  kernel/sched/topology.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 10d1391e7416..8b45f16a1890 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
>   * function:
>   *
>   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
> + *   SD_CLUSTER             - describes CPU Cluster topologies

So I know this is the naming we've gone for the "Cluster" naming, but this
comment isn't really explaining anything.

include/linux/sched/sd_flags.h has a bit more info already:
 * Domain members share CPU cluster (LLC tags or L2 cache)

I had to go through a bit of git history to remember what the CLUSTER thing
was about, how about this:

* SD_CLUSTER             - describes shared shared caches, cache tags or busses
* SD_SHARE_PKG_RESOURCES - describes shared LLC cache

And looking at this it would make sense to:
  rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
  rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
but that's another topic...

>   *   SD_SHARE_PKG_RESOURCES - describes shared caches
>   *   SD_NUMA                - describes NUMA topologies
>   *
> --
> 2.43.0
  

On 2/2/24 10:27 PM, Valentin Schneider wrote:
> 
> Subject nit: the prefix should be sched/topology
> 
> On 01/02/24 19:54, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> The description of SD_CLUSTER is missing. Add it.
>>
>> Signed-off-by: Alex Shi <alexs@kernel.org>
>> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>> To: Valentin Schneider <vschneid@redhat.com>
>> To: Vincent Guittot <vincent.guittot@linaro.org>
>> To: Juri Lelli <juri.lelli@redhat.com>
>> To: Peter Zijlstra <peterz@infradead.org>
>> To: Ingo Molnar <mingo@redhat.com>
>> ---
>>  kernel/sched/topology.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>> index 10d1391e7416..8b45f16a1890 100644
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
>>   * function:
>>   *
>>   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
>> + *   SD_CLUSTER             - describes CPU Cluster topologies
> 
> So I know this is the naming we've gone for the "Cluster" naming, but this
> comment isn't really explaining anything.
> 
> include/linux/sched/sd_flags.h has a bit more info already:
>  * Domain members share CPU cluster (LLC tags or L2 cache)
> 
> I had to go through a bit of git history to remember what the CLUSTER thing
> was about, how about this:
> 
> * SD_CLUSTER             - describes shared shared caches, cache tags or busses

Double "shared", so could we use:
* SD_CLUSTER             - describes shared caches, cache tags or busses?


> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
> 
> And looking at this it would make sense to:
>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
>   rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
> but that's another topic...

Uh, naming is a hard things. :)

Thanks
Alex
> 
>>   *   SD_SHARE_PKG_RESOURCES - describes shared caches
>>   *   SD_NUMA                - describes NUMA topologies
>>   *
>> --
>> 2.43.0
>
  

On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
> 
> Subject nit: the prefix should be sched/topology
> 
> On 01/02/24 19:54, alexs@kernel.org wrote:
> > From: Alex Shi <alexs@kernel.org>
> >
> > The description of SD_CLUSTER is missing. Add it.
> >
> > Signed-off-by: Alex Shi <alexs@kernel.org>
> > To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > To: Valentin Schneider <vschneid@redhat.com>
> > To: Vincent Guittot <vincent.guittot@linaro.org>
> > To: Juri Lelli <juri.lelli@redhat.com>
> > To: Peter Zijlstra <peterz@infradead.org>
> > To: Ingo Molnar <mingo@redhat.com>
> > ---
> >  kernel/sched/topology.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> > index 10d1391e7416..8b45f16a1890 100644
> > --- a/kernel/sched/topology.c
> > +++ b/kernel/sched/topology.c
> > @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
> >   * function:
> >   *
> >   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
> > + *   SD_CLUSTER             - describes CPU Cluster topologies
> 
> So I know this is the naming we've gone for the "Cluster" naming, but this
> comment isn't really explaining anything.
> 
> include/linux/sched/sd_flags.h has a bit more info already:
>  * Domain members share CPU cluster (LLC tags or L2 cache)

I also thought of this, but I didn't want to suggest to repeat in topolog.c
what is described in sd_flags.h.

Maybe it is better to remove the descriptions of all flags here and instead
direct the reader to sd_flags.h?

> 
> I had to go through a bit of git history to remember what the CLUSTER thing
> was about, how about this:
> 
> * SD_CLUSTER             - describes shared shared caches, cache tags or busses

AFAIK, this describes a subset of CPUs in the package that share a
resource, likely L2 cache.

> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
> 
> And looking at this it would make sense to:
>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES

but not all CPUs in the package share the resource

>   rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
> but that's another topic...
>
  

On 2024/2/2 22:27, Valentin Schneider wrote:
> 
> Subject nit: the prefix should be sched/topology
> 
> On 01/02/24 19:54, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> The description of SD_CLUSTER is missing. Add it.
>>
>> Signed-off-by: Alex Shi <alexs@kernel.org>
>> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>> To: Valentin Schneider <vschneid@redhat.com>
>> To: Vincent Guittot <vincent.guittot@linaro.org>
>> To: Juri Lelli <juri.lelli@redhat.com>
>> To: Peter Zijlstra <peterz@infradead.org>
>> To: Ingo Molnar <mingo@redhat.com>
>> ---
>>  kernel/sched/topology.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>> index 10d1391e7416..8b45f16a1890 100644
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
>>   * function:
>>   *
>>   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
>> + *   SD_CLUSTER             - describes CPU Cluster topologies
> 
> So I know this is the naming we've gone for the "Cluster" naming, but this
> comment isn't really explaining anything.
> 
> include/linux/sched/sd_flags.h has a bit more info already:
>  * Domain members share CPU cluster (LLC tags or L2 cache)
> 

Cluster topology in scheduler should mean CPUs beyond the SMT which are sharing
some cache resources (currently L2 on some Intel platforms or L3 Tag on our platforms)
but not the LLC.

A drawing in c5e22feffdd7 ("topology: Represent clusters of CPUs within a die") has
a good illustration and comment of cpus_share_resources() also illustrate this a bit:

/*
 * Whether CPUs are share cache resources, which means LLC on non-cluster
 * machines and LLC tag or L2 on machines with clusters.
 */
bool cpus_share_resources(int this_cpu, int that_cpu)

> I had to go through a bit of git history to remember what the CLUSTER thing
> was about, how about this:
> 
> * SD_CLUSTER             - describes shared shared caches, cache tags or busses
> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
> 
> And looking at this it would make sense to:
>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
>   rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
> but that's another topic...
> 
>>   *   SD_SHARE_PKG_RESOURCES - describes shared caches
>>   *   SD_NUMA                - describes NUMA topologies
>>   *
>> --
>> 2.43.0
> 
> 
> .
>
  

On 2/6/24 10:46 AM, Ricardo Neri wrote:
> On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
>>
>> Subject nit: the prefix should be sched/topology
>>
>> On 01/02/24 19:54, alexs@kernel.org wrote:
>>> From: Alex Shi <alexs@kernel.org>
>>>
>>> The description of SD_CLUSTER is missing. Add it.
>>>
>>> Signed-off-by: Alex Shi <alexs@kernel.org>
>>> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>>> To: Valentin Schneider <vschneid@redhat.com>
>>> To: Vincent Guittot <vincent.guittot@linaro.org>
>>> To: Juri Lelli <juri.lelli@redhat.com>
>>> To: Peter Zijlstra <peterz@infradead.org>
>>> To: Ingo Molnar <mingo@redhat.com>
>>> ---
>>>  kernel/sched/topology.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>>> index 10d1391e7416..8b45f16a1890 100644
>>> --- a/kernel/sched/topology.c
>>> +++ b/kernel/sched/topology.c
>>> @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
>>>   * function:
>>>   *
>>>   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
>>> + *   SD_CLUSTER             - describes CPU Cluster topologies
>>
>> So I know this is the naming we've gone for the "Cluster" naming, but this
>> comment isn't really explaining anything.
>>
>> include/linux/sched/sd_flags.h has a bit more info already:
>>  * Domain members share CPU cluster (LLC tags or L2 cache)
> 
> I also thought of this, but I didn't want to suggest to repeat in topolog.c
> what is described in sd_flags.h.
> 
> Maybe it is better to remove the descriptions of all flags here and instead
> direct the reader to sd_flags.h?

yes, good idea for getting their meaning directly from the header file. 
So is it fine for next sending?

    sched/fair: remove duplicate comments for SD_ flags
    
    Originally, it missed SD_CLUSTER flag description, but comparing to copy
    the flags meaning from sd_flags.h, reader is better to get info from
    there.
    
    Keep SD_ASYM_PACKING unchange for a point from another way.
    
    Signed-off-by: Alex Shi <alexs@kernel.org>
    To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
    To: Valentin Schneider <vschneid@redhat.com>
    To: Vincent Guittot <vincent.guittot@linaro.org>
    To: Juri Lelli <juri.lelli@redhat.com>
    To: Peter Zijlstra <peterz@infradead.org>
    To: Ingo Molnar <mingo@redhat.com>

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 10d1391e7416..96a24bf954ad 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1551,11 +1551,7 @@ static struct cpumask            ***sched_domains_numa_masks;
  *
  * These flags are purely descriptive of the topology and do not prescribe
  * behaviour. Behaviour is artificial and mapped in the below sd_init()
- * function:
- *
- *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
- *   SD_SHARE_PKG_RESOURCES - describes shared caches
- *   SD_NUMA                - describes NUMA topologies
+ * function, for details in include/linux/sched/sd_flags.h:
  *
  * Odd one out, which beside describing the topology has a quirk also
  * prescribes the desired behaviour that goes along with it:
> 
>>
>> I had to go through a bit of git history to remember what the CLUSTER thing
>> was about, how about this:
>>
>> * SD_CLUSTER             - describes shared shared caches, cache tags or busses
> 
> AFAIK, this describes a subset of CPUs in the package that share a
> resource, likely L2 cache.
> 
>> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
>>
>> And looking at this it would make sense to:
>>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
> 
> but not all CPUs in the package share the resource
> 
>>   rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
>> but that's another topic...
>>
  

On 05/02/24 18:46, Ricardo Neri wrote:
> On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
>>
>> Subject nit: the prefix should be sched/topology
>>
>> On 01/02/24 19:54, alexs@kernel.org wrote:
>> > From: Alex Shi <alexs@kernel.org>
>> >
>> > The description of SD_CLUSTER is missing. Add it.
>> >
>> > Signed-off-by: Alex Shi <alexs@kernel.org>
>> > To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>> > To: Valentin Schneider <vschneid@redhat.com>
>> > To: Vincent Guittot <vincent.guittot@linaro.org>
>> > To: Juri Lelli <juri.lelli@redhat.com>
>> > To: Peter Zijlstra <peterz@infradead.org>
>> > To: Ingo Molnar <mingo@redhat.com>
>> > ---
>> >  kernel/sched/topology.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>> > index 10d1391e7416..8b45f16a1890 100644
>> > --- a/kernel/sched/topology.c
>> > +++ b/kernel/sched/topology.c
>> > @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
>> >   * function:
>> >   *
>> >   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
>> > + *   SD_CLUSTER             - describes CPU Cluster topologies
>>
>> So I know this is the naming we've gone for the "Cluster" naming, but this
>> comment isn't really explaining anything.
>>
>> include/linux/sched/sd_flags.h has a bit more info already:
>>  * Domain members share CPU cluster (LLC tags or L2 cache)
>
> I also thought of this, but I didn't want to suggest to repeat in topolog.c
> what is described in sd_flags.h.
>
> Maybe it is better to remove the descriptions of all flags here and instead
> direct the reader to sd_flags.h?
>

Yeah I agree on less duplication.

>>
>> I had to go through a bit of git history to remember what the CLUSTER thing
>> was about, how about this:
>>
>> * SD_CLUSTER             - describes shared shared caches, cache tags or busses
>
> AFAIK, this describes a subset of CPUs in the package that share a
> resource, likely L2 cache.
>
>> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
>>
>> And looking at this it would make sense to:
>>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
>
> but not all CPUs in the package share the resource

But SD_CLUSTER never expands beyond the package, right?

Regardless, my main point is that having both SD_CLUSTER and
SD_SHARE_PKG_RESOURCES is a source of confusion (at the very least for
myself), and given SD_SHARE_PKG_RESOURCES is really used to mean "shares
LLC" (see update_top_cache_domain()), we could make that flag more explicit
and lift some ambiguity with SD_CLUSTER.

>
>>   rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
>> but that's another topic...
>>
  

On Tue, Feb 06, 2024 at 04:56:02PM +0800, kuiliang Shi wrote:
> 
> 
> On 2/6/24 10:46 AM, Ricardo Neri wrote:
> > On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
> >>
> >> Subject nit: the prefix should be sched/topology
> >>
> >> On 01/02/24 19:54, alexs@kernel.org wrote:
> >>> From: Alex Shi <alexs@kernel.org>
> >>>
> >>> The description of SD_CLUSTER is missing. Add it.
> >>>
> >>> Signed-off-by: Alex Shi <alexs@kernel.org>
> >>> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> >>> To: Valentin Schneider <vschneid@redhat.com>
> >>> To: Vincent Guittot <vincent.guittot@linaro.org>
> >>> To: Juri Lelli <juri.lelli@redhat.com>
> >>> To: Peter Zijlstra <peterz@infradead.org>
> >>> To: Ingo Molnar <mingo@redhat.com>
> >>> ---
> >>>  kernel/sched/topology.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> >>> index 10d1391e7416..8b45f16a1890 100644
> >>> --- a/kernel/sched/topology.c
> >>> +++ b/kernel/sched/topology.c
> >>> @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
> >>>   * function:
> >>>   *
> >>>   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
> >>> + *   SD_CLUSTER             - describes CPU Cluster topologies
> >>
> >> So I know this is the naming we've gone for the "Cluster" naming, but this
> >> comment isn't really explaining anything.
> >>
> >> include/linux/sched/sd_flags.h has a bit more info already:
> >>  * Domain members share CPU cluster (LLC tags or L2 cache)
> > 
> > I also thought of this, but I didn't want to suggest to repeat in topolog.c
> > what is described in sd_flags.h.
> > 
> > Maybe it is better to remove the descriptions of all flags here and instead
> > direct the reader to sd_flags.h?
> 
> yes, good idea for getting their meaning directly from the header file. 
> So is it fine for next sending?

Some tweaks below.

> 
>     sched/fair: remove duplicate comments for SD_ flags

      sched/topology: Remove duplicate descriptions from TOPOLOGY_SD_FLAGS
>     
>     Originally, it missed SD_CLUSTER flag description, but comparing to copy
>     the flags meaning from sd_flags.h, reader is better to get info from
>     there.

      These flags are already documented in include/linux/sched/sd_flags.h.

      Keep the comment on SD_ASYM_PACKING as it is a special case.
>     
>     Keep SD_ASYM_PACKING unchange for a point from another way.
>     
>     Signed-off-by: Alex Shi <alexs@kernel.org>
>     To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>     To: Valentin Schneider <vschneid@redhat.com>
>     To: Vincent Guittot <vincent.guittot@linaro.org>
>     To: Juri Lelli <juri.lelli@redhat.com>
>     To: Peter Zijlstra <peterz@infradead.org>
>     To: Ingo Molnar <mingo@redhat.com>
> 
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 10d1391e7416..96a24bf954ad 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1551,11 +1551,7 @@ static struct cpumask            ***sched_domains_numa_masks;
>   *
>   * These flags are purely descriptive of the topology and do not prescribe
>   * behaviour. Behaviour is artificial and mapped in the below sd_init()
> - * function:
> - *
> - *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
> - *   SD_SHARE_PKG_RESOURCES - describes shared caches
> - *   SD_NUMA                - describes NUMA topologies

Maybe only remove the descriptions but keep the enumeration?

> + * function, for details in include/linux/sched/sd_flags.h:

      function. For details, see include/linux/sched/sd_flags.h.
  

On Tue, Feb 06, 2024 at 02:16:06PM +0100, Valentin Schneider wrote:
> On 05/02/24 18:46, Ricardo Neri wrote:
> > On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
> >>
> >> Subject nit: the prefix should be sched/topology
> >>
> >> On 01/02/24 19:54, alexs@kernel.org wrote:
> >> > From: Alex Shi <alexs@kernel.org>
> >> >
> >> > The description of SD_CLUSTER is missing. Add it.
> >> >
> >> > Signed-off-by: Alex Shi <alexs@kernel.org>
> >> > To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> >> > To: Valentin Schneider <vschneid@redhat.com>
> >> > To: Vincent Guittot <vincent.guittot@linaro.org>
> >> > To: Juri Lelli <juri.lelli@redhat.com>
> >> > To: Peter Zijlstra <peterz@infradead.org>
> >> > To: Ingo Molnar <mingo@redhat.com>
> >> > ---
> >> >  kernel/sched/topology.c | 1 +
> >> >  1 file changed, 1 insertion(+)
> >> >
> >> > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> >> > index 10d1391e7416..8b45f16a1890 100644
> >> > --- a/kernel/sched/topology.c
> >> > +++ b/kernel/sched/topology.c
> >> > @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
> >> >   * function:
> >> >   *
> >> >   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
> >> > + *   SD_CLUSTER             - describes CPU Cluster topologies
> >>
> >> So I know this is the naming we've gone for the "Cluster" naming, but this
> >> comment isn't really explaining anything.
> >>
> >> include/linux/sched/sd_flags.h has a bit more info already:
> >>  * Domain members share CPU cluster (LLC tags or L2 cache)
> >
> > I also thought of this, but I didn't want to suggest to repeat in topolog.c
> > what is described in sd_flags.h.
> >
> > Maybe it is better to remove the descriptions of all flags here and instead
> > direct the reader to sd_flags.h?
> >
> 
> Yeah I agree on less duplication.
> 
> >>
> >> I had to go through a bit of git history to remember what the CLUSTER thing
> >> was about, how about this:
> >>
> >> * SD_CLUSTER             - describes shared shared caches, cache tags or busses
> >
> > AFAIK, this describes a subset of CPUs in the package that share a
> > resource, likely L2 cache.
> >
> >> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
> >>
> >> And looking at this it would make sense to:
> >>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
> >
> > but not all CPUs in the package share the resource
> 
> But SD_CLUSTER never expands beyond the package, right?

Correct.

> 
> Regardless, my main point is that having both SD_CLUSTER and
> SD_SHARE_PKG_RESOURCES is a source of confusion (at the very least for
> myself),

Agreed!

> and given SD_SHARE_PKG_RESOURCES is really used to mean "shares
> LLC" (see update_top_cache_domain()), we could make that flag more explicit
> and lift some ambiguity with SD_CLUSTER.

As Yicong stated, cluster topology should mean CPUs beyond SMT that share
some resource but not LLC.

It makes sense to me to keep SD_CLUSTER name as it is today and rename
SD_SHARE_PKG_RESOURCES as SD_SHARE_LLC.
  

On 2/7/24 5:57 AM, Ricardo Neri wrote:
> On Tue, Feb 06, 2024 at 02:16:06PM +0100, Valentin Schneider wrote:
>> On 05/02/24 18:46, Ricardo Neri wrote:
>>> On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
>>>>
>>>> Subject nit: the prefix should be sched/topology
>>>>
>>>> On 01/02/24 19:54, alexs@kernel.org wrote:
>>>>> From: Alex Shi <alexs@kernel.org>
>>>>>
>>>>> The description of SD_CLUSTER is missing. Add it.
>>>>>
>>>>> Signed-off-by: Alex Shi <alexs@kernel.org>
>>>>> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>>>>> To: Valentin Schneider <vschneid@redhat.com>
>>>>> To: Vincent Guittot <vincent.guittot@linaro.org>
>>>>> To: Juri Lelli <juri.lelli@redhat.com>
>>>>> To: Peter Zijlstra <peterz@infradead.org>
>>>>> To: Ingo Molnar <mingo@redhat.com>
>>>>> ---
>>>>>  kernel/sched/topology.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>>>>> index 10d1391e7416..8b45f16a1890 100644
>>>>> --- a/kernel/sched/topology.c
>>>>> +++ b/kernel/sched/topology.c
>>>>> @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
>>>>>   * function:
>>>>>   *
>>>>>   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
>>>>> + *   SD_CLUSTER             - describes CPU Cluster topologies
>>>>
>>>> So I know this is the naming we've gone for the "Cluster" naming, but this
>>>> comment isn't really explaining anything.
>>>>
>>>> include/linux/sched/sd_flags.h has a bit more info already:
>>>>  * Domain members share CPU cluster (LLC tags or L2 cache)
>>>
>>> I also thought of this, but I didn't want to suggest to repeat in topolog.c
>>> what is described in sd_flags.h.
>>>
>>> Maybe it is better to remove the descriptions of all flags here and instead
>>> direct the reader to sd_flags.h?
>>>
>>
>> Yeah I agree on less duplication.
>>
>>>>
>>>> I had to go through a bit of git history to remember what the CLUSTER thing
>>>> was about, how about this:
>>>>
>>>> * SD_CLUSTER             - describes shared shared caches, cache tags or busses
>>>
>>> AFAIK, this describes a subset of CPUs in the package that share a
>>> resource, likely L2 cache.
>>>
>>>> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
>>>>
>>>> And looking at this it would make sense to:
>>>>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
>>>
>>> but not all CPUs in the package share the resource
>>
>> But SD_CLUSTER never expands beyond the package, right?
> 
> Correct.
> 
>>
>> Regardless, my main point is that having both SD_CLUSTER and
>> SD_SHARE_PKG_RESOURCES is a source of confusion (at the very least for
>> myself),
> 
> Agreed!
> 
>> and given SD_SHARE_PKG_RESOURCES is really used to mean "shares
>> LLC" (see update_top_cache_domain()), we could make that flag more explicit
>> and lift some ambiguity with SD_CLUSTER.
> 
> As Yicong stated, cluster topology should mean CPUs beyond SMT that share
> some resource but not LLC.
> 
> It makes sense to me to keep SD_CLUSTER name as it is today and rename
> SD_SHARE_PKG_RESOURCES as SD_SHARE_LLC.

yes, agree with his.
  

On 2/7/24 5:24 AM, Ricardo Neri wrote:
> On Tue, Feb 06, 2024 at 04:56:02PM +0800, kuiliang Shi wrote:
>>
>>
>> On 2/6/24 10:46 AM, Ricardo Neri wrote:
>>> On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
>>>>
>>>> Subject nit: the prefix should be sched/topology
>>>>
>>>> On 01/02/24 19:54, alexs@kernel.org wrote:
>>>>> From: Alex Shi <alexs@kernel.org>
>>>>>
>>>>> The description of SD_CLUSTER is missing. Add it.
>>>>>
>>>>> Signed-off-by: Alex Shi <alexs@kernel.org>
>>>>> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>>>>> To: Valentin Schneider <vschneid@redhat.com>
>>>>> To: Vincent Guittot <vincent.guittot@linaro.org>
>>>>> To: Juri Lelli <juri.lelli@redhat.com>
>>>>> To: Peter Zijlstra <peterz@infradead.org>
>>>>> To: Ingo Molnar <mingo@redhat.com>
>>>>> ---
>>>>>  kernel/sched/topology.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>>>>> index 10d1391e7416..8b45f16a1890 100644
>>>>> --- a/kernel/sched/topology.c
>>>>> +++ b/kernel/sched/topology.c
>>>>> @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
>>>>>   * function:
>>>>>   *
>>>>>   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
>>>>> + *   SD_CLUSTER             - describes CPU Cluster topologies
>>>>
>>>> So I know this is the naming we've gone for the "Cluster" naming, but this
>>>> comment isn't really explaining anything.
>>>>
>>>> include/linux/sched/sd_flags.h has a bit more info already:
>>>>  * Domain members share CPU cluster (LLC tags or L2 cache)
>>>
>>> I also thought of this, but I didn't want to suggest to repeat in topolog.c
>>> what is described in sd_flags.h.
>>>
>>> Maybe it is better to remove the descriptions of all flags here and instead
>>> direct the reader to sd_flags.h?
>>
>> yes, good idea for getting their meaning directly from the header file. 
>> So is it fine for next sending?
> 
> Some tweaks below.
> 
>>
>>     sched/fair: remove duplicate comments for SD_ flags
> 
>       sched/topology: Remove duplicate descriptions from TOPOLOGY_SD_FLAGS
>>     
>>     Originally, it missed SD_CLUSTER flag description, but comparing to copy
>>     the flags meaning from sd_flags.h, reader is better to get info from
>>     there.
> 
>       These flags are already documented in include/linux/sched/sd_flags.h.
> 
>       Keep the comment on SD_ASYM_PACKING as it is a special case.
>>     
>>     Keep SD_ASYM_PACKING unchange for a point from another way.
>>     
>>     Signed-off-by: Alex Shi <alexs@kernel.org>
>>     To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>>     To: Valentin Schneider <vschneid@redhat.com>
>>     To: Vincent Guittot <vincent.guittot@linaro.org>
>>     To: Juri Lelli <juri.lelli@redhat.com>
>>     To: Peter Zijlstra <peterz@infradead.org>
>>     To: Ingo Molnar <mingo@redhat.com>
>>
>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>> index 10d1391e7416..96a24bf954ad 100644
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -1551,11 +1551,7 @@ static struct cpumask            ***sched_domains_numa_masks;
>>   *
>>   * These flags are purely descriptive of the topology and do not prescribe
>>   * behaviour. Behaviour is artificial and mapped in the below sd_init()
>> - * function:
>> - *
>> - *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
>> - *   SD_SHARE_PKG_RESOURCES - describes shared caches
>> - *   SD_NUMA                - describes NUMA topologies
> 
> Maybe only remove the descriptions but keep the enumeration?
> 
>> + * function, for details in include/linux/sched/sd_flags.h:
> 
>       function. For details, see include/linux/sched/sd_flags.h.
> 

Thanks for comments. will update in next version.
>