linux-kernel - Re: [PATCH] sched/fair: Optimize select_idle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6242deaa-e570-3384-0737-e49abb0599dd@arm.com>
Date:   Fri, 6 Dec 2019 12:00:00 +0000
From:   Valentin Schneider <valentin.schneider@....com>
To:     Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
        Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Rik van Riel <riel@...riel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH] sched/fair: Optimize select_idle_core

On 05/12/2019 17:23, Srikar Dronamraju wrote:
> Currently we loop through all threads of a core to evaluate if the core
> is idle or not. This is unnecessary. If a thread of a core is not
> idle, skip evaluating other threads of a core.
> 
> Signed-off-by: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
> ---
>  kernel/sched/fair.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 69a81a5709ff..b9d628128cfc 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5872,10 +5872,12 @@ static int select_idle_core(struct task_struct *p, struct sched_domain *sd, int
>  		bool idle = true;
>  
>  		for_each_cpu(cpu, cpu_smt_mask(core)) {
> -			__cpumask_clear_cpu(cpu, cpus);
> -			if (!available_idle_cpu(cpu))
> +			if (!available_idle_cpu(cpu)) {
>  				idle = false;
> +				break;
> +			}
>  		}
> +		cpumask_andnot(cpus, cpus, cpu_smt_mask(core));
>  

That looks sensible enough to me. I do have one random thought, however.


Say you have a 4-core SMT2 system with the usual numbering scheme:

{0, 4}  {1, 5}  {2, 6}  {3, 7}
CORE0   CORE1   CORE2   CORE3


Say 'target' is the prev_cpu, in that case let's pick 5. Because we do a
for_each_cpu_wrap(), our iteration for 'core' would start with 

  5, 6, 7, ...

So say CORE2 is entirely idle and CORE1 isn't, we would go through the
inner loop on CORE1 (with 'core' == 5), then go through CORE2 (with
'core' == 6) and return 'core'. I find it a bit unusual that we wouldn't
return the first CPU in the SMT mask, usually we try to fill sched_groups
in cpumask order.


If we could have 'cpus' start with only primary CPUs, that would simplify
things methinks:

  for_each_cpu_wrap(core, cpus, target) {
	  bool idle = true;

	  for_each_cpu(cpu, cpu_smt_mask(core)) {
		  if (!available_idle_cpu(cpu)) {
			  idle = false;
			  break;
		  }

	  __cpumask_clear_cpu(core, cpus);

	  if (idle)
		  return core;


Food for thought; your change itself looks fine as it is.

Reviewed-by: Valentin Schneider <valentin.schneider@....com>


>  		if (idle)
>  			return core;
>