Re: [PATCH -tip 12/32] sched: Simplify the core pick loop for optimized case

[Joel Fernandes <joel@joelfernandes.org>]

Hi Peter,

On Tue, Nov 24, 2020 at 01:04:38PM +0100, Peter Zijlstra wrote:
> On Tue, Nov 17, 2020 at 06:19:42PM -0500, Joel Fernandes (Google) wrote:
> > +	/*
> > +	 * Optimize for common case where this CPU has no cookies
> > +	 * and there are no cookied tasks running on siblings.
> > +	 */
> > +	if (!need_sync) {
> > +		for_each_class(class) {
> > +			next = class->pick_task(rq);
> > +			if (next)
> > +				break;
> > +		}
> > +
> > +		if (!next->core_cookie) {
> > +			rq->core_pick = NULL;
> > +			goto done;
> > +		}
> >  		need_sync = true;
> >  	}
> 
> This isn't what I send you here:
> 
>   https://lkml.kernel.org/r/20201026093131.GF2628@hirez.programming.kicks-ass.net

I had replied to it here with concerns about the effects of newly idle
balancing not being reverseable, it was only a theoretical concern:
http://lore.kernel.org/r/20201105185019.GA2771003@google.com

Also I was trying to keep the logic the same as v8 for unconstrained pick
(calling pick_task), considering that has been tested quite a bit.

> Specifically, you've lost the whole cfs-cgroup optimization.

Are you referring to this optimization in pick_next_task_fair() ?

/*
 * Since we haven't yet done put_prev_entity and if the
 * selected task
 * is a different task than we started out with, try
 * and touch the
 * least amount of cfs_rqs.
 */

You are right, we wouldn't get that with just calling pick_task_fair(). We
did not have this in v8 series either though.

Also, if the task is a cookied task, then I think you are doing more work
with your patch due to the extra put_prev_task().

> What was wrong/not working with the below?

Other than the new idle balancing, IIRC it was also causing instability.
Maybe we can considering this optimization in the future if that's Ok with
you?

thanks,

 - Joel

> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5225,8 +5227,6 @@ pick_next_task(struct rq *rq, struct tas
>  		return next;
>  	}
>  
> -	put_prev_task_balance(rq, prev, rf);
> -
>  	smt_mask = cpu_smt_mask(cpu);
>  	need_sync = !!rq->core->core_cookie;
>  
> @@ -5255,17 +5255,14 @@ pick_next_task(struct rq *rq, struct tas
>  	 * and there are no cookied tasks running on siblings.
>  	 */
>  	if (!need_sync) {
> -		for_each_class(class) {
> -			next = class->pick_task(rq);
> -			if (next)
> -				break;
> -		}
> -
> +		next = __pick_next_task(rq, prev, rf);
>  		if (!next->core_cookie) {
>  			rq->core_pick = NULL;
> -			goto done;
> +			return next;
>  		}
> -		need_sync = true;
> +		put_prev_task(next);
> +	} else {
> +		put_prev_task_balance(rq, prev, rf);
>  	}
>  
>  	for_each_cpu(i, smt_mask) {