Re: [PATCH 1/2] sched/fair: Honor asymmetric SMT priority in idle selection

[Kayra Cizmeci]

Hi Andrea,

Hope I ain't got anything wrong, I'm a bit sick.

> POWER7 uses SD_ASYM_PACKING at the shared-capacity SMT level to order
> hardware threads, and NVIDIA Olympus benefits from the same policy. Idle
> CPU selection does not consult that order, so a task can wake on an
> arbitrary sibling and remain there until load balancing corrects the
> placement. On these systems, that initial choice can prevent the core
> from entering its preferred lower-thread resource mode and cause a large
> and persistent performance loss.

> When idle selection finds an available CPU in an SMT core, choose the
> highest-priority available sibling. On SMT2 Olympus this only changes
> selection on fully idle cores. A partially idle core has only one
> available CPU. On wider SMT systems such as POWER7, it also fills
> available siblings in priority order while the core is partially busy.

> Apply the preference to idle-core and idle-CPU scans,
> asymmetric-capacity scans, target, previous, recently-used CPU fast
> paths and the slow path. Inspect the lowest scheduling domain directly,
> but require both CPUs to share its span because isolcpus can split
> hardware siblings across scheduling domains.

> Keep physical-core capacity selection independent from SMT sibling
> ordering. SD_ASYM_CPUCAPACITY first selects among cores with different
> maximum capacities, then SD_ASYM_PACKING selects the preferred available
> sibling inside the chosen core, whose siblings continue to share equal
> capacity.


> +/*
> + * Redirect a CPU to a higher-priority available sibling in its SMT domain,
> + * subject to task affinity.
> + */
> +static inline int select_idle_smt_cpu(struct task_struct *p, int cpu)
> +{
> +	struct sched_domain *sd;
> +	int best = cpu;
> +	int sibling;
> +
> +	if (!sched_smt_active())
> +		return cpu;
> +
> +	sd = rcu_dereference_all(cpu_rq(cpu)->sd);
> +	if (!sd || !(sd->flags & SD_SHARE_CPUCAPACITY) ||
> +	    !(sd->flags & SD_ASYM_PACKING))
> +		return cpu;
> +
> +	for_each_cpu_and(sibling, sched_domain_span(sd), p->cpus_ptr) {
> +		if (sibling == best || !choose_idle_cpu(sibling, p))
> +			continue;
> +
> +		if (sched_asym_prefer(sibling, best))
> +			best = sibling;
> +	}
> +
> +	return best;
> +}

> @@ -9061,14 +9099,18 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>  
>  		if (!has_idle_core && cpus_share_cache(prev, target)) {
>  			i = select_idle_smt(p, sd, prev);
> -			if ((unsigned int)i < nr_cpumask_bits)
> -				return i;
> +			if ((unsigned int)i < nr_cpumask_bits) {
> +				target = i;
> +				goto select_smt_priority;
> +			}
>  		}
>  	}

The sched_smt_active() check on select_idle_smt_cpu seems to be redundant on here.

We could remove this
By moving the check on select_idle_smt_cpu() to goto block and
adding one check before the select_idle_smt_cpu() on select_task_rq_fair(). And just calling
select_idle_smt_cpu() on here.

I also have a question, do we need select_idle_smt() call on here? If yes then why? I was really confused while reading.

Thanks,
Kayra :_: