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

From: Valentin Schneider
Date: Mon Apr 20 2020 - 18:55:31 EST



(There's a v2 at cover.1587393807.git.yu.c.chen@xxxxxxxxx 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);