Re: [PATCH] sched/fair: Make is_core_idle() check all cpus in a core

Hi,

> The skip is what lets a caller ask this from a CPU that is about to become
> idle, where idle_cpu() cannot be true yet.
> 
> sched_balance_newidle() calls
> 
> 	sched_balance_rq(this_cpu, this_rq, sd, CPU_NEWLY_IDLE, ...)
> 
> so env->dst_cpu is this_cpu, and we are inside __schedule() with rq->curr
> still the outgoing task.  idle_rq() wants rq->curr == rq->idle, so
> idle_cpu(this_cpu) is false.  __CPU_NOT_IDLE is 0, so the env->idle test in
> update_sg_lb_stats() does not filter CPU_NEWLY_IDLE out either.  For every
> newidle balance the patch therefore gives:
> 
>    env->dst_core_idle	false, so the misfit gate in
> 			update_sd_pick_busiest() stops pulling
>    sched_use_asym_prio()	false, so asym packing no longer applies
> 
> s390 cannot show that: SD_ASYM_PACKING is set only by powerpc and x86 ITMT,
> SD_ASYM_CPUCAPACITY only by arm64 big.LITTLE and x86 hybrid.

Thank you for the detailed explanation. I totally overlooked the
CPU_NEWLY_IDLE case.

> The other callers -- numa_idle_core(), select_idle_capacity(),
> asym_fits_cpu(), should_we_balance() -- all establish idle_cpu(cpu) first,
> so there this only adds a redundant idle_cpu() per candidate, two of them
> on the wakeup path.

I thought idle_cpu() is a cheap call to make and an extra one wouldn't
make a difference.

> Since the complaint is really the name, would renaming it do the job
> without touching behaviour?  asym_fits_cpu() already words it as "the core
> has no busy siblings", and sched_use_asym_prio()'s kernel-doc treats @cpu's
> idleness as the caller's precondition.

Right, a rename sounds like a better option to me now. I'll send a new
version.

> dst_core_idle may well want whole-core semantics as its comment says, but
> that reads like a separate patch with numbers from an asymmetric-capacity
> machine.

I suppose after the rename, the code block would represent what is
being done more clearly and cause more people to raise their
eyebrows. Maybe then we get some measurements :)

Thanks!