Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH 2/2] sched: Extract the task putting code from pick_next_task() | Date | Mon, 20 Apr 2020 23:55:21 +0100 |
| |
(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);
| |