[v2,6/6,RFT] sched/fair: change sched asym checking condition

  From: Alex Shi <alexs@kernel.org>

Asym only used on SMT sd, or core sd with ITMT and core idled.
!sched_smt_active isn't necessary.

Signed-off-by: Alex Shi <alexs@kernel.org>
To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: linux-kernel@vger.kernel.org
To: Valentin Schneider <vschneid@redhat.com>
To: Daniel Bristot de Oliveira <bristot@redhat.com>
To: Ben Segall <bsegall@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
To: Dietmar Eggemann <dietmar.eggemann@arm.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/fair.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

On Tue, Jan 30, 2024 at 09:17:08PM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> Asym only used on SMT sd, or core sd with ITMT and core idled.
> !sched_smt_active isn't necessary.

sched_smt_active() is implemented as a static key. Thus, if SMT is not
enabled, we can quickly return without having to check the rest of the
conditions, as we should.

> 
> Signed-off-by: Alex Shi <alexs@kernel.org>
> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> To: linux-kernel@vger.kernel.org
> To: Valentin Schneider <vschneid@redhat.com>
> To: Daniel Bristot de Oliveira <bristot@redhat.com>
> To: Ben Segall <bsegall@google.com>
> To: Steven Rostedt <rostedt@goodmis.org>
> To: Dietmar Eggemann <dietmar.eggemann@arm.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/fair.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 6680cb39c787..0b7530b93429 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9744,8 +9744,8 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>  	if (!(sd->flags & SD_ASYM_PACKING))
>  		return false;
>  
> -	return (!sched_smt_active()) ||
> -		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
> +	return (sd->flags & SD_SHARE_CPUCAPACITY) ||
> +		(is_core_idle(cpu) && test_bit(cpu_core_flags(), (void *)&sd->flags));

cpu_core_flags() can contain more than one flag, AFAICS. Which bit should
it check? Moreover, it is implemented differently for each architecture.
Also, as stated, in x86 asym_packing is also used in domains other than MC.
  

On 2/1/24 9:10 AM, Ricardo Neri wrote:
> On Tue, Jan 30, 2024 at 09:17:08PM +0800, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> Asym only used on SMT sd, or core sd with ITMT and core idled.
>> !sched_smt_active isn't necessary.
> 
> sched_smt_active() is implemented as a static key. Thus, if SMT is not
> enabled, we can quickly return without having to check the rest of the
> conditions, as we should.

Hi Ricardo,

Thanks a lot for comments! I will drop this patch in this series.

But forgive my stupidity, asym feature is possible when SMT enabled instead of SMT disable. Why no SMT is a condition for asm feature? For this asym feature, I only see the SMT and MC domain use this, correct me if I'm wrong. 

> 
>>
>> Signed-off-by: Alex Shi <alexs@kernel.org>
>> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>> To: linux-kernel@vger.kernel.org
>> To: Valentin Schneider <vschneid@redhat.com>
>> To: Daniel Bristot de Oliveira <bristot@redhat.com>
>> To: Ben Segall <bsegall@google.com>
>> To: Steven Rostedt <rostedt@goodmis.org>
>> To: Dietmar Eggemann <dietmar.eggemann@arm.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/fair.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 6680cb39c787..0b7530b93429 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -9744,8 +9744,8 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>>  	if (!(sd->flags & SD_ASYM_PACKING))
>>  		return false;
>>  
>> -	return (!sched_smt_active()) ||
>> -		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
>> +	return (sd->flags & SD_SHARE_CPUCAPACITY) ||
>> +		(is_core_idle(cpu) && test_bit(cpu_core_flags(), (void *)&sd->flags));
> 
> cpu_core_flags() can contain more than one flag, AFAICS. Which bit should
> it check? Moreover, it is implemented differently for each architecture.

It seems only x86 using the function. But there is still a error which SMT/CLUSTER domain also has this flags bit.
$ git  grep 'cpu_core_flags('
arch/x86/kernel/smpboot.c:      return cpu_core_flags() | x86_sched_itmt_flags();
include/linux/sched/topology.h:static inline int cpu_core_flags(void)

> Also, as stated, in x86 asym_packing is also used in domains other than MC.

For the feature SD_ASYM_PACKING and SD_ASYM_CPUCAPACITY, for guts of 2 features, is it possible to combine them into one, if we give a little bit more capacity to priority cpus, like 5%?

Thanks
Alex
  

On 2/1/24 5:10 PM, kuiliang Shi wrote:
> 
> 
> On 2/1/24 9:10 AM, Ricardo Neri wrote:
>> On Tue, Jan 30, 2024 at 09:17:08PM +0800, alexs@kernel.org wrote:
>>> From: Alex Shi <alexs@kernel.org>
>>>
>>> Asym only used on SMT sd, or core sd with ITMT and core idled.
>>> !sched_smt_active isn't necessary.
>>
>> sched_smt_active() is implemented as a static key. Thus, if SMT is not
>> enabled, we can quickly return without having to check the rest of the
>> conditions, as we should.
> 
> Hi Ricardo,
> 
> Thanks a lot for comments! I will drop this patch in this series.
> 
> But forgive my stupidity, asym feature is possible when SMT enabled instead of SMT disable. Why no SMT is a condition for asm feature? For this asym feature, I only see the SMT and MC domain use this, correct me if I'm wrong. 
> 

on power7 ASYM_PACKING is used to pack at SMT level. 

On x86, ITMT topology uses ASYM_PACKING to do load balancing instead of using different cpu capacities.

Its possible to have it in PKG(earlier referred as DIE) as well. 
In powerpc recently we did that for shared processor LPAR's. So asym feature is in PKG as well. 

>>
>>>
  

On Thu, Feb 01, 2024 at 08:49:05PM +0530, Shrikanth Hegde wrote:
> 
> 
> On 2/1/24 5:10 PM, kuiliang Shi wrote:
> > 
> > 
> > On 2/1/24 9:10 AM, Ricardo Neri wrote:
> >> On Tue, Jan 30, 2024 at 09:17:08PM +0800, alexs@kernel.org wrote:
> >>> From: Alex Shi <alexs@kernel.org>
> >>>
> >>> Asym only used on SMT sd, or core sd with ITMT and core idled.
> >>> !sched_smt_active isn't necessary.
> >>
> >> sched_smt_active() is implemented as a static key. Thus, if SMT is not
> >> enabled, we can quickly return without having to check the rest of the
> >> conditions, as we should.
> > 
> > Hi Ricardo,
> > 
> > Thanks a lot for comments! I will drop this patch in this series.
> > 
> > But forgive my stupidity, asym feature is possible when SMT enabled instead of SMT disable. Why no SMT is a condition for asm feature? For this asym feature, I only see the SMT and MC domain use this, correct me if I'm wrong. 
> > 
> 
> on power7 ASYM_PACKING is used to pack at SMT level. 

Indeed, this is why the function returns true if it the sched domain has
the SD_SHARE_CPUCAPACITY flag.

When SMT is disabled there is no point in doing any check because we will
always want to use asym_packing.

> 
> On x86, ITMT topology uses ASYM_PACKING to do load balancing instead of using different cpu capacities.

You can look at x86_cluster_flags() and x86_die_flags().

> 
> Its possible to have it in PKG(earlier referred as DIE) as well. 
> In powerpc recently we did that for shared processor LPAR's. So asym feature is in PKG as well. 
> 
> >>
> >>>