Introduce a new function finish_prev_task() to do the balance
when necessary, and then put previous task back to the run queue.
This function is extracted from pick_next_task() to prepare for
future usage by other type of task picking logic.
No functional change.
Suggested-by: Peter Zijlstra <[email protected]>
Signed-off-by: Chen Yu <[email protected]>
---
kernel/sched/core.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3a61a3b8eaa9..bf59a5cf030c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -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
+ * 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);
for_each_class(class) {
p = class->pick_next_task(rq);
--
2.20.1
On Mon, 20 Apr 2020 00:31:52 +0800
Chen Yu <[email protected]> wrote:
> Introduce a new function finish_prev_task() to do the balance
> when necessary, and then put previous task back to the run queue.
> This function is extracted from pick_next_task() to prepare for
> future usage by other type of task picking logic.
>
> No functional change.
>
> Suggested-by: Peter Zijlstra <[email protected]>
> Signed-off-by: Chen Yu <[email protected]>
> ---
> kernel/sched/core.c | 39 +++++++++++++++++++++++----------------
> 1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 3a61a3b8eaa9..bf59a5cf030c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -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.
> + * 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.
-- Steve
>
> for_each_class(class) {
> p = class->pick_next_task(rq);
(There's a v2 at [email protected] 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);
On Mon, Apr 20, 2020 at 11:55:21PM +0100, Valentin Schneider wrote:
> >> + 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()".
What Valentin said; it's the last put we do before picking a new task.
Also, I don't like long names. That said, I'm open to short and
appropriate suggestions.
On Tue, 21 Apr 2020 01:13:55 +0200
Peter Zijlstra <[email protected]> wrote:
> > 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()".
>
> What Valentin said; it's the last put we do before picking a new task.
> Also, I don't like long names. That said, I'm open to short and
> appropriate suggestions.
I wont bikeshed this too much.
Is the "finish" more appropriate with the other use cases that are
coming. I do like that "put_prev_task_balance()" too.
-- Steve
On Tue, 21 Apr 2020 at 04:23, Steven Rostedt <[email protected]> wrote:
>
> On Tue, 21 Apr 2020 01:13:55 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > > 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()".
> >
> > What Valentin said; it's the last put we do before picking a new task.
> > Also, I don't like long names. That said, I'm open to short and
> > appropriate suggestions.
>
> I wont bikeshed this too much.
>
> Is the "finish" more appropriate with the other use cases that are
> coming. I do like that "put_prev_task_balance()" too.
This name looks reasonnable
>
> -- Steve
On Mon, Apr 20, 2020 at 06:32:32PM -0400, Steven Rostedt wrote:
> On Mon, 20 Apr 2020 00:31:52 +0800
> Chen Yu <[email protected]> wrote:
>
> > Introduce a new function finish_prev_task() to do the balance
> > when necessary, and then put previous task back to the run queue.
> > This function is extracted from pick_next_task() to prepare for
> > future usage by other type of task picking logic.
> >
> > No functional change.
> >
> > Suggested-by: Peter Zijlstra <[email protected]>
> > Signed-off-by: Chen Yu <[email protected]>
> > ---
> > kernel/sched/core.c | 39 +++++++++++++++++++++++----------------
> > 1 file changed, 23 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 3a61a3b8eaa9..bf59a5cf030c 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -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.
>
>
Okay, I'll fix it in v3.
> > + 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.
>
Per the discussion, I think put_prev_task_balance() might be an
appropriate one.
Thanks,
Chenyu
> -- Steve
>
> >
> > for_each_class(class) {
> > p = class->pick_next_task(rq);
>
On Tue, Apr 21, 2020 at 09:42:26AM +0200, Vincent Guittot wrote:
> On Tue, 21 Apr 2020 at 04:23, Steven Rostedt <[email protected]> wrote:
> >
> > On Tue, 21 Apr 2020 01:13:55 +0200
> > Peter Zijlstra <[email protected]> wrote:
> >
> > > > 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()".
> > >
> > > What Valentin said; it's the last put we do before picking a new task.
> > > Also, I don't like long names. That said, I'm open to short and
> > > appropriate suggestions.
> >
> > I wont bikeshed this too much.
> >
> > Is the "finish" more appropriate with the other use cases that are
> > coming. I do like that "put_prev_task_balance()" too.
>
> This name looks reasonnable
>
Okay, I'll change it to this name.
Thanks,
Chenyu
> >
> > -- Steve