Re: [PATCH] sched/fair: Optimize select_idle_core

[Srikar Dronamraju <srikar@linux.vnet.ibm.com>]

Hi Valentin,

> 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:
> 

Its probably something to think over. I probably don't have an answer on why
we are not choosing the starting cpu to be primary CPU.  Would we have to
think of the case where the Primary CPUs are online / offline etc? I mean
with target cpu, we know the CPU is online for sure.

>   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@arm.com>
> 

Thanks for the review.

-- 
Thanks and Regards
Srikar Dronamraju