Re: [PATCH 2/2] sched: Extract the task putting code from pick_next_task()

[Valentin Schneider <valentin.schneider@arm.com>]

(There's a v2 at cover.1587393807.git.yu.c.chen@intel.com but I think this
still applies)

On 20/04/20 23:32, Steven Rostedt wrote:
>> @@ -3904,6 +3904,28 @@ static inline void schedule_debug(struct task_struct *prev, bool preempt)
>>      schedstat_inc(this_rq()->sched_count);
>>  }
>>
>> +static void finish_prev_task(struct rq *rq, struct task_struct *prev,
>> +			     struct rq_flags *rf)
>> +{
>> +	const struct sched_class *class;
>> +#ifdef CONFIG_SMP
>> +	/*
>> +	 * We must do the balancing pass before put_next_task(), such
>
> I know this is just a cut and paste move, but I'm thinking that this
> comment is wrong. Shouldn't this be "put_prev_task()" as we have no
> "put_next_task()" function.
>

Oh, I think you're right.

>
>> +	 * that when we release the rq->lock the task is in the same
>> +	 * state as before we took rq->lock.
>> +	 *
>> +	 * We can terminate the balance pass as soon as we know there is
>> +	 * a runnable task of @class priority or higher.
>> +	 */
>> +	for_class_range(class, prev->sched_class, &idle_sched_class) {
>> +		if (class->balance(rq, prev, rf))
>> +			break;
>> +	}
>> +#endif
>> +
>> +	put_prev_task(rq, prev);
>> +}
>> +
>>  /*
>>   * Pick up the highest-prio task:
>>   */
>> @@ -3937,22 +3959,7 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
>>      }
>>
>>  restart:
>> -#ifdef CONFIG_SMP
>> -	/*
>> -	 * We must do the balancing pass before put_next_task(), such
>> -	 * that when we release the rq->lock the task is in the same
>> -	 * state as before we took rq->lock.
>> -	 *
>> -	 * We can terminate the balance pass as soon as we know there is
>> -	 * a runnable task of @class priority or higher.
>> -	 */
>> -	for_class_range(class, prev->sched_class, &idle_sched_class) {
>> -		if (class->balance(rq, prev, rf))
>> -			break;
>> -	}
>> -#endif
>> -
>> -	put_prev_task(rq, prev);
>> +	finish_prev_task(rq, prev, rf);
>
> I'm not sure I like the name of this function. Perhaps
> "balance_and_put_prev_task()"? Something more in kind to what the function
> does.
>

The 'finish' thing isn't too far from the truth; it's the last thing we
need to do with the prev task (in terms of sched bookkeeping, I mean) -
and in Chen's defence ISTR Peter suggested that name.

Seeing as it's a "supercharged" put_prev_task(), I could live with the
marginally shorter "put_prev_task_balance()".

> -- Steve
>
>>
>>      for_each_class(class) {
>>              p = class->pick_next_task(rq);