Re: [PATCH -tip 14/32] sched: migration changes for core scheduling

[Peter Zijlstra]

On Mon, Nov 23, 2020 at 12:36:10PM +0800, Li, Aubrey wrote:
> >> +#ifdef CONFIG_SCHED_CORE
> >> +		/*
> >> +		 * Skip this cpu if source task's cookie does not match
> >> +		 * with CPU's core cookie.
> >> +		 */
> >> +		if (!sched_core_cookie_match(cpu_rq(cpu), env->p))
> >> +			continue;
> >> +#endif
> >> +
> > 
> > Any reason this is under an #ifdef? In sched_core_cookie_match() won't
> > the check for sched_core_enabled() do the right thing even when
> > CONFIG_SCHED_CORE is not enabed?> 
> Yes, sched_core_enabled works properly when CONFIG_SCHED_CORE is not
> enabled. But when CONFIG_SCHED_CORE is not enabled, it does not make
> sense to leave a core scheduler specific function here even at compile
> time. Also, for the cases in hot path, this saves CPU cycles to avoid
> a judgment.

No, that's nonsense. If it works, remove the #ifdef. Less (#ifdef) is
more.

> >> +static inline bool sched_core_cookie_match(struct rq *rq, struct task_struct *p)
> >> +{
> >> +	bool idle_core = true;
> >> +	int cpu;
> >> +
> >> +	/* Ignore cookie match if core scheduler is not enabled on the CPU. */
> >> +	if (!sched_core_enabled(rq))
> >> +		return true;
> >> +
> >> +	for_each_cpu(cpu, cpu_smt_mask(cpu_of(rq))) {
> >> +		if (!available_idle_cpu(cpu)) {
> > 
> > I was looking at this snippet and comparing this to is_core_idle(), the
> > major difference is the check for vcpu_is_preempted(). Do we want to
> > call the core as non idle if any vcpu was preempted on this CPU?
> 
> Yes, if there is a VCPU was preempted on this CPU, better not place task
> on this core as the VCPU may be holding a spinlock and wants to be executed
> again ASAP.

If you're doing core scheduling on vcpus, you deserve all the pain
possible.