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

[Yicong Yang]

On 2024/2/2 22:27, Valentin Schneider wrote:
> 
> Subject nit: the prefix should be sched/topology
> 
> On 01/02/24 19:54, alexs@xxxxxxxxxx wrote:
>> From: Alex Shi <alexs@xxxxxxxxxx>
>>
>> The description of SD_CLUSTER is missing. Add it.
>>
>> Signed-off-by: Alex Shi <alexs@xxxxxxxxxx>
>> To: Ricardo Neri <ricardo.neri-calderon@xxxxxxxxxxxxxxx>
>> To: Valentin Schneider <vschneid@xxxxxxxxxx>
>> To: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
>> To: Juri Lelli <juri.lelli@xxxxxxxxxx>
>> To: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> To: Ingo Molnar <mingo@xxxxxxxxxx>
>> ---
>>  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
> 
> 
> .
>