[PATCH] sched: topology: add input parameter for sched_domain_flags_f()

[PATCH] sched: topology: add input parameter for sched_domain_flags_f()

[Qing Wang]

From: Wang Qing <wangqing@vivo.com>

sched_domain_flags_f() are statically set now, but actually, we can get a
lot of necessary information based on the cpu_map. e.g. we can know whether
its cache is shared.

Allows custom extension without affecting current.

Signed-off-by: Wang Qing <wangqing@vivo.com>
---
 arch/powerpc/kernel/smp.c      |  4 ++--
 arch/x86/kernel/smpboot.c      |  8 ++++----
 include/linux/sched/topology.h | 10 +++++-----
 kernel/sched/topology.c        |  2 +-
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index de0f6f0..e503d23
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -1000,7 +1000,7 @@ static bool shared_caches;
 
 #ifdef CONFIG_SCHED_SMT
 /* cpumask of CPUs with asymmetric SMT dependency */
-static int powerpc_smt_flags(void)
+static int powerpc_smt_flags(const struct cpumask *cpu_map)
 {
 	int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
 
@@ -1018,7 +1018,7 @@ static int powerpc_smt_flags(void)
  * since the migrated task remains cache hot. We want to take advantage of this
  * at the scheduler level so an extra topology level is required.
  */
-static int powerpc_shared_cache_flags(void)
+static int powerpc_shared_cache_flags(const struct cpumask *cpu_map)
 {
 	return SD_SHARE_PKG_RESOURCES;
 }
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 2ef1477..c005a8e
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -535,25 +535,25 @@ static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
 
 
 #if defined(CONFIG_SCHED_SMT) || defined(CONFIG_SCHED_CLUSTER) || defined(CONFIG_SCHED_MC)
-static inline int x86_sched_itmt_flags(void)
+static inline int x86_sched_itmt_flags(const struct cpumask *cpu_map)
 {
 	return sysctl_sched_itmt_enabled ? SD_ASYM_PACKING : 0;
 }
 
 #ifdef CONFIG_SCHED_MC
-static int x86_core_flags(void)
+static int x86_core_flags(const struct cpumask *cpu_map)
 {
 	return cpu_core_flags() | x86_sched_itmt_flags();
 }
 #endif
 #ifdef CONFIG_SCHED_SMT
-static int x86_smt_flags(void)
+static int x86_smt_flags(const struct cpumask *cpu_map)
 {
 	return cpu_smt_flags() | x86_sched_itmt_flags();
 }
 #endif
 #ifdef CONFIG_SCHED_CLUSTER
-static int x86_cluster_flags(void)
+static int x86_cluster_flags(const struct cpumask *cpu_map)
 {
 	return cpu_cluster_flags() | x86_sched_itmt_flags();
 }
diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
index 56cffe4..6aa985a
--- a/include/linux/sched/topology.h
+++ b/include/linux/sched/topology.h
@@ -36,28 +36,28 @@ extern const struct sd_flag_debug sd_flag_debug[];
 #endif
 
 #ifdef CONFIG_SCHED_SMT
-static inline int cpu_smt_flags(void)
+static inline int cpu_smt_flags(const struct cpumask *cpu_map)
 {
 	return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
 }
 #endif
 
 #ifdef CONFIG_SCHED_CLUSTER
-static inline int cpu_cluster_flags(void)
+static inline int cpu_cluster_flags(const struct cpumask *cpu_map)
 {
 	return SD_SHARE_PKG_RESOURCES;
 }
 #endif
 
 #ifdef CONFIG_SCHED_MC
-static inline int cpu_core_flags(void)
+static inline int cpu_core_flags(const struct cpumask *cpu_map)
 {
 	return SD_SHARE_PKG_RESOURCES;
 }
 #endif
 
 #ifdef CONFIG_NUMA
-static inline int cpu_numa_flags(void)
+static inline int cpu_numa_flags(const struct cpumask *cpu_map)
 {
 	return SD_NUMA;
 }
@@ -180,7 +180,7 @@ void free_sched_domains(cpumask_var_t doms[], unsigned int ndoms);
 bool cpus_share_cache(int this_cpu, int that_cpu);
 
 typedef const struct cpumask *(*sched_domain_mask_f)(int cpu);
-typedef int (*sched_domain_flags_f)(void);
+typedef int (*sched_domain_flags_f)(const struct cpumask *cpu_map);
 
 #define SDTL_OVERLAP	0x01
 
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 05b6c2a..34dfec4
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1556,7 +1556,7 @@ sd_init(struct sched_domain_topology_level *tl,
 	sd_weight = cpumask_weight(tl->mask(cpu));
 
 	if (tl->sd_flags)
-		sd_flags = (*tl->sd_flags)();
+		sd_flags = (*tl->sd_flags)(tl->mask(cpu));
 	if (WARN_ONCE(sd_flags & ~TOPOLOGY_SD_FLAGS,
 			"wrong sd_flags in topology description\n"))
 		sd_flags &= TOPOLOGY_SD_FLAGS;
-- 
2.7.4

Re: [PATCH] sched: topology: add input parameter for sched_domain_flags_f()

[Srikar Dronamraju]

* Qing Wang <wangqing@vivo.com> [2022-03-28 02:53:37]:

> From: Wang Qing <wangqing@vivo.com>
> 
> sched_domain_flags_f() are statically set now, but actually, we can get a
> lot of necessary information based on the cpu_map. e.g. we can know whether
> its cache is shared.
> 
> Allows custom extension without affecting current.
> 

can you elaborate on how passing a cpumask pointer will help us to know if
cache is shared or not? With this patch, we are just passing the cpumask,
but dont seem to be using it in any of the cases. 

or did I miss anything?

> Signed-off-by: Wang Qing <wangqing@vivo.com>
> ---
>  arch/powerpc/kernel/smp.c      |  4 ++--
>  arch/x86/kernel/smpboot.c      |  8 ++++----
>  include/linux/sched/topology.h | 10 +++++-----
>  kernel/sched/topology.c        |  2 +-
>  4 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index de0f6f0..e503d23
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1000,7 +1000,7 @@ static bool shared_caches;
>  
>  #ifdef CONFIG_SCHED_SMT
>  /* cpumask of CPUs with asymmetric SMT dependency */
> -static int powerpc_smt_flags(void)
> +static int powerpc_smt_flags(const struct cpumask *cpu_map)
>  {
>  	int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
>  
> @@ -1018,7 +1018,7 @@ static int powerpc_smt_flags(void)
>   * since the migrated task remains cache hot. We want to take advantage of this
>   * at the scheduler level so an extra topology level is required.
>   */
> -static int powerpc_shared_cache_flags(void)
> +static int powerpc_shared_cache_flags(const struct cpumask *cpu_map)
>  {
>  	return SD_SHARE_PKG_RESOURCES;
>  }
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 2ef1477..c005a8e
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -535,25 +535,25 @@ static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
>  
>  
>  #if defined(CONFIG_SCHED_SMT) || defined(CONFIG_SCHED_CLUSTER) || defined(CONFIG_SCHED_MC)
> -static inline int x86_sched_itmt_flags(void)
> +static inline int x86_sched_itmt_flags(const struct cpumask *cpu_map)
>  {
>  	return sysctl_sched_itmt_enabled ? SD_ASYM_PACKING : 0;
>  }
>  
>  #ifdef CONFIG_SCHED_MC
> -static int x86_core_flags(void)
> +static int x86_core_flags(const struct cpumask *cpu_map)
>  {
>  	return cpu_core_flags() | x86_sched_itmt_flags();
>  }
>  #endif
>  #ifdef CONFIG_SCHED_SMT
> -static int x86_smt_flags(void)
> +static int x86_smt_flags(const struct cpumask *cpu_map)
>  {
>  	return cpu_smt_flags() | x86_sched_itmt_flags();
>  }
>  #endif
>  #ifdef CONFIG_SCHED_CLUSTER
> -static int x86_cluster_flags(void)
> +static int x86_cluster_flags(const struct cpumask *cpu_map)
>  {
>  	return cpu_cluster_flags() | x86_sched_itmt_flags();
>  }
> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
> index 56cffe4..6aa985a
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -36,28 +36,28 @@ extern const struct sd_flag_debug sd_flag_debug[];
>  #endif
>  
>  #ifdef CONFIG_SCHED_SMT
> -static inline int cpu_smt_flags(void)
> +static inline int cpu_smt_flags(const struct cpumask *cpu_map)
>  {
>  	return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
>  }
>  #endif
>  
>  #ifdef CONFIG_SCHED_CLUSTER
> -static inline int cpu_cluster_flags(void)
> +static inline int cpu_cluster_flags(const struct cpumask *cpu_map)
>  {
>  	return SD_SHARE_PKG_RESOURCES;
>  }
>  #endif
>  
>  #ifdef CONFIG_SCHED_MC
> -static inline int cpu_core_flags(void)
> +static inline int cpu_core_flags(const struct cpumask *cpu_map)
>  {
>  	return SD_SHARE_PKG_RESOURCES;
>  }
>  #endif
>  
>  #ifdef CONFIG_NUMA
> -static inline int cpu_numa_flags(void)
> +static inline int cpu_numa_flags(const struct cpumask *cpu_map)
>  {
>  	return SD_NUMA;
>  }
> @@ -180,7 +180,7 @@ void free_sched_domains(cpumask_var_t doms[], unsigned int ndoms);
>  bool cpus_share_cache(int this_cpu, int that_cpu);
>  
>  typedef const struct cpumask *(*sched_domain_mask_f)(int cpu);
> -typedef int (*sched_domain_flags_f)(void);
> +typedef int (*sched_domain_flags_f)(const struct cpumask *cpu_map);
>  
>  #define SDTL_OVERLAP	0x01
>  
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 05b6c2a..34dfec4
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1556,7 +1556,7 @@ sd_init(struct sched_domain_topology_level *tl,
>  	sd_weight = cpumask_weight(tl->mask(cpu));
>  
>  	if (tl->sd_flags)
> -		sd_flags = (*tl->sd_flags)();
> +		sd_flags = (*tl->sd_flags)(tl->mask(cpu));
>  	if (WARN_ONCE(sd_flags & ~TOPOLOGY_SD_FLAGS,
>  			"wrong sd_flags in topology description\n"))
>  		sd_flags &= TOPOLOGY_SD_FLAGS;
> -- 
> 2.7.4
> 
> 

Re: [PATCH] sched: topology: add input parameter for sched_domain_flags_f()

[Srikar Dronamraju]

* Qing Wang <wangqing@vivo.com> [2022-03-28 02:53:37]:

> From: Wang Qing <wangqing@vivo.com>
> 
> sched_domain_flags_f() are statically set now, but actually, we can get a
> lot of necessary information based on the cpu_map. e.g. we can know whether
> its cache is shared.
> 
> Allows custom extension without affecting current.
> 

can you elaborate on how passing a cpumask pointer will help us to know if
cache is shared or not? With this patch, we are just passing the cpumask,
but dont seem to be using it in any of the cases. 

or did I miss anything?

> Signed-off-by: Wang Qing <wangqing@vivo.com>
> ---
>  arch/powerpc/kernel/smp.c      |  4 ++--
>  arch/x86/kernel/smpboot.c      |  8 ++++----
>  include/linux/sched/topology.h | 10 +++++-----
>  kernel/sched/topology.c        |  2 +-
>  4 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index de0f6f0..e503d23
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1000,7 +1000,7 @@ static bool shared_caches;
>  
>  #ifdef CONFIG_SCHED_SMT
>  /* cpumask of CPUs with asymmetric SMT dependency */
> -static int powerpc_smt_flags(void)
> +static int powerpc_smt_flags(const struct cpumask *cpu_map)
>  {
>  	int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
>  
> @@ -1018,7 +1018,7 @@ static int powerpc_smt_flags(void)
>   * since the migrated task remains cache hot. We want to take advantage of this
>   * at the scheduler level so an extra topology level is required.
>   */
> -static int powerpc_shared_cache_flags(void)
> +static int powerpc_shared_cache_flags(const struct cpumask *cpu_map)
>  {
>  	return SD_SHARE_PKG_RESOURCES;
>  }
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 2ef1477..c005a8e
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -535,25 +535,25 @@ static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
>  
>  
>  #if defined(CONFIG_SCHED_SMT) || defined(CONFIG_SCHED_CLUSTER) || defined(CONFIG_SCHED_MC)
> -static inline int x86_sched_itmt_flags(void)
> +static inline int x86_sched_itmt_flags(const struct cpumask *cpu_map)
>  {
>  	return sysctl_sched_itmt_enabled ? SD_ASYM_PACKING : 0;
>  }
>  
>  #ifdef CONFIG_SCHED_MC
> -static int x86_core_flags(void)
> +static int x86_core_flags(const struct cpumask *cpu_map)
>  {
>  	return cpu_core_flags() | x86_sched_itmt_flags();
>  }
>  #endif
>  #ifdef CONFIG_SCHED_SMT
> -static int x86_smt_flags(void)
> +static int x86_smt_flags(const struct cpumask *cpu_map)
>  {
>  	return cpu_smt_flags() | x86_sched_itmt_flags();
>  }
>  #endif
>  #ifdef CONFIG_SCHED_CLUSTER
> -static int x86_cluster_flags(void)
> +static int x86_cluster_flags(const struct cpumask *cpu_map)
>  {
>  	return cpu_cluster_flags() | x86_sched_itmt_flags();
>  }
> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
> index 56cffe4..6aa985a
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -36,28 +36,28 @@ extern const struct sd_flag_debug sd_flag_debug[];
>  #endif
>  
>  #ifdef CONFIG_SCHED_SMT
> -static inline int cpu_smt_flags(void)
> +static inline int cpu_smt_flags(const struct cpumask *cpu_map)
>  {
>  	return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
>  }
>  #endif
>  
>  #ifdef CONFIG_SCHED_CLUSTER
> -static inline int cpu_cluster_flags(void)
> +static inline int cpu_cluster_flags(const struct cpumask *cpu_map)
>  {
>  	return SD_SHARE_PKG_RESOURCES;
>  }
>  #endif
>  
>  #ifdef CONFIG_SCHED_MC
> -static inline int cpu_core_flags(void)
> +static inline int cpu_core_flags(const struct cpumask *cpu_map)
>  {
>  	return SD_SHARE_PKG_RESOURCES;
>  }
>  #endif
>  
>  #ifdef CONFIG_NUMA
> -static inline int cpu_numa_flags(void)
> +static inline int cpu_numa_flags(const struct cpumask *cpu_map)
>  {
>  	return SD_NUMA;
>  }
> @@ -180,7 +180,7 @@ void free_sched_domains(cpumask_var_t doms[], unsigned int ndoms);
>  bool cpus_share_cache(int this_cpu, int that_cpu);
>  
>  typedef const struct cpumask *(*sched_domain_mask_f)(int cpu);
> -typedef int (*sched_domain_flags_f)(void);
> +typedef int (*sched_domain_flags_f)(const struct cpumask *cpu_map);
>  
>  #define SDTL_OVERLAP	0x01
>  
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 05b6c2a..34dfec4
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1556,7 +1556,7 @@ sd_init(struct sched_domain_topology_level *tl,
>  	sd_weight = cpumask_weight(tl->mask(cpu));
>  
>  	if (tl->sd_flags)
> -		sd_flags = (*tl->sd_flags)();
> +		sd_flags = (*tl->sd_flags)(tl->mask(cpu));
>  	if (WARN_ONCE(sd_flags & ~TOPOLOGY_SD_FLAGS,
>  			"wrong sd_flags in topology description\n"))
>  		sd_flags &= TOPOLOGY_SD_FLAGS;
> -- 
> 2.7.4
> 
> 

Re: [PATCH] sched: topology: add input parameter for sched_domain_flags_f()

[Peter Zijlstra]

On Mon, Mar 28, 2022 at 02:53:37AM -0700, Qing Wang wrote:
> From: Wang Qing <wangqing@vivo.com>
> 
> sched_domain_flags_f() are statically set now, but actually, we can get a
> lot of necessary information based on the cpu_map. e.g. we can know whether
> its cache is shared.
> 
> Allows custom extension without affecting current.

This all still makes absolutely no sense. The architecture builds these
masks, the architecture is in charge of which flags function is called
on which mask.

Passing the mask back in means it lost the plot somewhere and doens't
know wth it's doing anymore.

NAK

Re: [PATCH] sched: topology: add input parameter for sched_domain_flags_f()

[Peter Zijlstra]

On Mon, Mar 28, 2022 at 02:53:37AM -0700, Qing Wang wrote:
> From: Wang Qing <wangqing@vivo.com>
> 
> sched_domain_flags_f() are statically set now, but actually, we can get a
> lot of necessary information based on the cpu_map. e.g. we can know whether
> its cache is shared.
> 
> Allows custom extension without affecting current.

This all still makes absolutely no sense. The architecture builds these
masks, the architecture is in charge of which flags function is called
on which mask.

Passing the mask back in means it lost the plot somewhere and doens't
know wth it's doing anymore.

NAK

[PATCH] sched: topology: add input parameter for sched_domain_flags_f()

[王擎]

>> From: Wang Qing <wangqing@vivo.com>
>> 
>> sched_domain_flags_f() are statically set now, but actually, we can get a
>> lot of necessary information based on the cpu_map. e.g. we can know whether
>> its cache is shared.
>> 
>> Allows custom extension without affecting current.
>
>This all still makes absolutely no sense. The architecture builds these
>masks, the architecture is in charge of which flags function is called
>on which mask.
>
>Passing the mask back in means it lost the plot somewhere and doens't
>know wth it's doing anymore.

It is not passing the mask back, sched_domain_flags_f() doesn't use cpumask
at all, it always return fixed values. sd_topology_level select different 
sd_flag() to config, it's too primitive.

If an architecture can describe its cache topology clearly in every level,
the sd only need sched_domain_flags_f(cpumask) to get its ShPR flag.

Thanks,
Wang

>
>NAK

[PATCH] sched: topology: add input parameter for sched_domain_flags_f()

[王擎]

>> From: Wang Qing <wangqing@vivo.com>
>> 
>> sched_domain_flags_f() are statically set now, but actually, we can get a
>> lot of necessary information based on the cpu_map. e.g. we can know whether
>> its cache is shared.
>> 
>> Allows custom extension without affecting current.
>
>This all still makes absolutely no sense. The architecture builds these
>masks, the architecture is in charge of which flags function is called
>on which mask.
>
>Passing the mask back in means it lost the plot somewhere and doens't
>know wth it's doing anymore.

It is not passing the mask back, sched_domain_flags_f() doesn't use cpumask
at all, it always return fixed values. sd_topology_level select different 
sd_flag() to config, it's too primitive.

If an architecture can describe its cache topology clearly in every level,
the sd only need sched_domain_flags_f(cpumask) to get its ShPR flag.

Thanks,
Wang

>
>NAK