Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1586802 > unrolled thread

Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too

Started byPavan Kondeti <pkondeti@codeaurora.org>
First post2017-02-23 11:40 +0100
Last post2017-02-27 19:20 +0100
Articles 9 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too Pavan Kondeti <pkondeti@codeaurora.org> - 2017-02-23 11:40 +0100
    Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too Peter Zijlstra <peterz@infradead.org> - 2017-02-23 15:00 +0100
      Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too Pavan Kondeti <pkondeti@codeaurora.org> - 2017-02-23 16:20 +0100
        Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too Peter Zijlstra <peterz@infradead.org> - 2017-02-23 16:50 +0100
          Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too Peter Zijlstra <peterz@infradead.org> - 2017-02-23 17:40 +0100
            Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too Pavan Kondeti <pkondeti@codeaurora.org> - 2017-02-23 18:30 +0100
              Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too Peter Zijlstra <peterz@infradead.org> - 2017-02-23 18:50 +0100
                Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too Peter Zijlstra <peterz@infradead.org> - 2017-02-23 19:00 +0100
                  Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too Steven Rostedt <rostedt@goodmis.org> - 2017-02-27 19:20 +0100

#1586802 — Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too

FromPavan Kondeti <pkondeti@codeaurora.org>
Date2017-02-23 11:40 +0100
SubjectRe: [PATCH] sched: Optimize pick_next_task for idle_sched_class too
Message-ID<tdYYy-8q5-11@gated-at.bofh.it>
Hi Peter,

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 49ce1cb..51ca21e 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3321,15 +3321,14 @@ static inline void schedule_debug(struct task_struct *prev)
>  static inline struct task_struct *
>  pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
>  {
> -       const struct sched_class *class = &fair_sched_class;
> +       const struct sched_class *class;
>         struct task_struct *p;
>
>         /*
>          * Optimization: we know that if all tasks are in
>          * the fair class we can call that function directly:
>          */
> -       if (likely(prev->sched_class == class &&
> -                  rq->nr_running == rq->cfs.h_nr_running)) {
> +       if (likely(rq->nr_running == rq->cfs.h_nr_running)) {
>                 p = fair_sched_class.pick_next_task(rq, prev, rf);
>                 if (unlikely(p == RETRY_TASK))
>                         goto again;

Would this delay pulling RT tasks from other CPUs? Lets say this CPU
has 2 fair tasks and 1 RT task. The RT task is sleeping now. Earlier,
we attempt to pull RT tasks from other CPUs in pick_next_task_rt(),
which is not done anymore.

Thanks,
Pavan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1586904

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-23 15:00 +0100
Message-ID<te265-23e-9@gated-at.bofh.it>
In reply to#1586802
On Thu, Feb 23, 2017 at 04:04:22PM +0530, Pavan Kondeti wrote:
> Hi Peter,
> 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 49ce1cb..51ca21e 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -3321,15 +3321,14 @@ static inline void schedule_debug(struct task_struct *prev)
> >  static inline struct task_struct *
> >  pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> >  {
> > -       const struct sched_class *class = &fair_sched_class;
> > +       const struct sched_class *class;
> >         struct task_struct *p;
> >
> >         /*
> >          * Optimization: we know that if all tasks are in
> >          * the fair class we can call that function directly:
> >          */
> > -       if (likely(prev->sched_class == class &&
> > -                  rq->nr_running == rq->cfs.h_nr_running)) {
> > +       if (likely(rq->nr_running == rq->cfs.h_nr_running)) {
> >                 p = fair_sched_class.pick_next_task(rq, prev, rf);
> >                 if (unlikely(p == RETRY_TASK))
> >                         goto again;
> 
> Would this delay pulling RT tasks from other CPUs? Lets say this CPU
> has 2 fair tasks and 1 RT task. The RT task is sleeping now. Earlier,
> we attempt to pull RT tasks from other CPUs in pick_next_task_rt(),
> which is not done anymore.

It should not; the two places of interrests are when we leave the RT
class to run anything lower (fair,idle), at which point we'll pull,
or when an RT tasks wakes up, at which point it'll push.

[toc] | [prev] | [next] | [standalone]

#1586946

FromPavan Kondeti <pkondeti@codeaurora.org>
Date2017-02-23 16:20 +0100
Message-ID<te3lv-323-3@gated-at.bofh.it>
In reply to#1586904
Hi Peter,

On Thu, Feb 23, 2017 at 7:24 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Feb 23, 2017 at 04:04:22PM +0530, Pavan Kondeti wrote:
>> Hi Peter,
>>
>> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> > index 49ce1cb..51ca21e 100644
>> > --- a/kernel/sched/core.c
>> > +++ b/kernel/sched/core.c
>> > @@ -3321,15 +3321,14 @@ static inline void schedule_debug(struct task_struct *prev)
>> >  static inline struct task_struct *
>> >  pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
>> >  {
>> > -       const struct sched_class *class = &fair_sched_class;
>> > +       const struct sched_class *class;
>> >         struct task_struct *p;
>> >
>> >         /*
>> >          * Optimization: we know that if all tasks are in
>> >          * the fair class we can call that function directly:
>> >          */
>> > -       if (likely(prev->sched_class == class &&
>> > -                  rq->nr_running == rq->cfs.h_nr_running)) {
>> > +       if (likely(rq->nr_running == rq->cfs.h_nr_running)) {
>> >                 p = fair_sched_class.pick_next_task(rq, prev, rf);
>> >                 if (unlikely(p == RETRY_TASK))
>> >                         goto again;
>>
>> Would this delay pulling RT tasks from other CPUs? Lets say this CPU
>> has 2 fair tasks and 1 RT task. The RT task is sleeping now. Earlier,
>> we attempt to pull RT tasks from other CPUs in pick_next_task_rt(),
>> which is not done anymore.
>
> It should not; the two places of interrests are when we leave the RT
> class to run anything lower (fair,idle), at which point we'll pull,
> or when an RT tasks wakes up, at which point it'll push.

Can you kindly show me where we are pulling when a RT task goes to
sleep? Apart from class/prio change, I see pull happening only from
pick_next_task_rt().

Thanks,
Pavan

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1586973

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-23 16:50 +0100
Message-ID<te3Ox-3bZ-23@gated-at.bofh.it>
In reply to#1586946
On Thu, Feb 23, 2017 at 08:45:06PM +0530, Pavan Kondeti wrote:
> Hi Peter,
> 
> On Thu, Feb 23, 2017 at 7:24 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Thu, Feb 23, 2017 at 04:04:22PM +0530, Pavan Kondeti wrote:
> >> Hi Peter,
> >>
> >> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> >> > index 49ce1cb..51ca21e 100644
> >> > --- a/kernel/sched/core.c
> >> > +++ b/kernel/sched/core.c
> >> > @@ -3321,15 +3321,14 @@ static inline void schedule_debug(struct task_struct *prev)
> >> >  static inline struct task_struct *
> >> >  pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> >> >  {
> >> > -       const struct sched_class *class = &fair_sched_class;
> >> > +       const struct sched_class *class;
> >> >         struct task_struct *p;
> >> >
> >> >         /*
> >> >          * Optimization: we know that if all tasks are in
> >> >          * the fair class we can call that function directly:
> >> >          */
> >> > -       if (likely(prev->sched_class == class &&
> >> > -                  rq->nr_running == rq->cfs.h_nr_running)) {
> >> > +       if (likely(rq->nr_running == rq->cfs.h_nr_running)) {
> >> >                 p = fair_sched_class.pick_next_task(rq, prev, rf);
> >> >                 if (unlikely(p == RETRY_TASK))
> >> >                         goto again;
> >>
> >> Would this delay pulling RT tasks from other CPUs? Lets say this CPU
> >> has 2 fair tasks and 1 RT task. The RT task is sleeping now. Earlier,
> >> we attempt to pull RT tasks from other CPUs in pick_next_task_rt(),
> >> which is not done anymore.
> >
> > It should not; the two places of interrests are when we leave the RT
> > class to run anything lower (fair,idle), at which point we'll pull,
> > or when an RT tasks wakes up, at which point it'll push.
> 
> Can you kindly show me where we are pulling when a RT task goes to
> sleep? Apart from class/prio change, I see pull happening only from
> pick_next_task_rt().

Ah, I read your question wrong. Yes I think you're right, we now loose
the pull when the last RT task goes away.

Hmm.. how to fix that nicely..

[toc] | [prev] | [next] | [standalone]

#1587005

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-23 17:40 +0100
Message-ID<te4AW-3Pf-17@gated-at.bofh.it>
In reply to#1586973
On Thu, Feb 23, 2017 at 04:25:33PM +0100, Peter Zijlstra wrote:
> 
> Ah, I read your question wrong. Yes I think you're right, we now loose
> the pull when the last RT task goes away.
> 
> Hmm.. how to fix that nicely..

Something like so perhaps? This would make a pull happen when the last
RT task on this CPU goes away.

Steve?

---
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 9f3e40226dec..283d591078b0 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1336,6 +1336,9 @@ static void dequeue_task_rt(struct rq *rq, struct task_struct *p, int flags)
 	dequeue_rt_entity(rt_se, flags);
 
 	dequeue_pushable_task(rq, p);
+
+	if (!rq->rt.rt_nr_running)
+		queue_pull_task(rq);
 }
 
 /*

[toc] | [prev] | [next] | [standalone]

#1587034

FromPavan Kondeti <pkondeti@codeaurora.org>
Date2017-02-23 18:30 +0100
Message-ID<te5nj-4nQ-9@gated-at.bofh.it>
In reply to#1587005
On Thu, Feb 23, 2017 at 10:07 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Feb 23, 2017 at 04:25:33PM +0100, Peter Zijlstra wrote:
>>
>> Ah, I read your question wrong. Yes I think you're right, we now loose
>> the pull when the last RT task goes away.
>>
>> Hmm.. how to fix that nicely..
>
> Something like so perhaps? This would make a pull happen when the last
> RT task on this CPU goes away.
>
> Steve?
>
> ---
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 9f3e40226dec..283d591078b0 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1336,6 +1336,9 @@ static void dequeue_task_rt(struct rq *rq, struct task_struct *p, int flags)
>         dequeue_rt_entity(rt_se, flags);
>
>         dequeue_pushable_task(rq, p);
> +
> +       if (!rq->rt.rt_nr_running)
> +               queue_pull_task(rq);
>  }
>
>  /*

The next balance_callback() is not called until the context switch is
completed. So we potentially pick a lower class task before the pull
happens. Would it be wrong to call pull_rt_task() directly instead of
queuing the callback.


-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1587048

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-23 18:50 +0100
Message-ID<te5GG-4ww-19@gated-at.bofh.it>
In reply to#1587034
On Thu, Feb 23, 2017 at 10:59:15PM +0530, Pavan Kondeti wrote:
> On Thu, Feb 23, 2017 at 10:07 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Thu, Feb 23, 2017 at 04:25:33PM +0100, Peter Zijlstra wrote:
> >>
> >> Ah, I read your question wrong. Yes I think you're right, we now loose
> >> the pull when the last RT task goes away.
> >>
> >> Hmm.. how to fix that nicely..
> >
> > Something like so perhaps? This would make a pull happen when the last
> > RT task on this CPU goes away.
> >
> > Steve?
> >
> > ---
> > diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> > index 9f3e40226dec..283d591078b0 100644
> > --- a/kernel/sched/rt.c
> > +++ b/kernel/sched/rt.c
> > @@ -1336,6 +1336,9 @@ static void dequeue_task_rt(struct rq *rq, struct task_struct *p, int flags)
> >         dequeue_rt_entity(rt_se, flags);
> >
> >         dequeue_pushable_task(rq, p);
> > +
> > +       if (!rq->rt.rt_nr_running)
> > +               queue_pull_task(rq);
> >  }
> >
> >  /*
> 
> The next balance_callback() is not called until the context switch is
> completed. So we potentially pick a lower class task before the pull
> happens. Would it be wrong to call pull_rt_task() directly instead of
> queuing the callback.

deactivate_task()...->dequeue_task_rt() cannot drop the rq->lock which
would be required to pull.

Hurm.. maybe we should do what Steve initially suggested. The
alternative is link order trickery, and I'm not sure we want to do that.

[toc] | [prev] | [next] | [standalone]

#1587058

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-23 19:00 +0100
Message-ID<te5Qm-4zQ-27@gated-at.bofh.it>
In reply to#1587048
On Thu, Feb 23, 2017 at 06:45:05PM +0100, Peter Zijlstra wrote:
> Hurm.. maybe we should do what Steve initially suggested. The
> alternative is link order trickery, and I'm not sure we want to do that.

That is, given:

kernel/sched/Makefile: obj-y += idle_task.o fair.o rt.o deadline.o stop_task.o

results in:

readelf -s defconfig-build/vmlinux | awk '/sched_class/ {print $2 " " $8}' | sort -n
00000000602c93c0 idle_sched_class
00000000602c9480 fair_sched_class
00000000602c9580 rt_sched_class
00000000602c96c0 dl_sched_class
00000000602c97c0 stop_sched_class

we can do this, but yuck!

---
 kernel/sched/core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 8f972df76eb2..eebe6729ceb7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3285,10 +3285,16 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
 	struct task_struct *p;
 
 	/*
-	 * Optimization: we know that if all tasks are in
-	 * the fair class we can call that function directly:
+	 * Optimization: we know that if all tasks are in the fair class we can
+	 * call that function directly, but only if the @prev task wasn't of a
+	 * higher scheduling class, because otherwise those loose the
+	 * opportinity to pull in more work from other CPUs.
+	 *
+	 * Depends on link order in kernel/sched/Makefile.
 	 */
-	if (likely(rq->nr_running == rq->cfs.h_nr_running)) {
+	if (likely(rq->nr_running == rq->cfs.h_nr_running &&
+		   prev->sched_class <= &fair_sched_class)) {
+
 		p = fair_sched_class.pick_next_task(rq, prev, rf);
 		if (unlikely(p == RETRY_TASK))
 			goto again;

[toc] | [prev] | [next] | [standalone]

#1588851

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-02-27 19:20 +0100
Message-ID<tfy3T-TW-15@gated-at.bofh.it>
In reply to#1587058
Sorry, for the late reply. Just got back from traveling.

On Thu, 23 Feb 2017 18:54:38 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Feb 23, 2017 at 06:45:05PM +0100, Peter Zijlstra wrote:
> > Hurm.. maybe we should do what Steve initially suggested. The
> > alternative is link order trickery, and I'm not sure we want to do that.  
> 
> That is, given:
> 
> kernel/sched/Makefile: obj-y += idle_task.o fair.o rt.o deadline.o stop_task.o
> 
> results in:
> 
> readelf -s defconfig-build/vmlinux | awk '/sched_class/ {print $2 " " $8}' | sort -n
> 00000000602c93c0 idle_sched_class
> 00000000602c9480 fair_sched_class
> 00000000602c9580 rt_sched_class
> 00000000602c96c0 dl_sched_class
> 00000000602c97c0 stop_sched_class
> 
> we can do this, but yuck!
> 
> ---
>  kernel/sched/core.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 8f972df76eb2..eebe6729ceb7 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3285,10 +3285,16 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
>  	struct task_struct *p;
>  
>  	/*
> -	 * Optimization: we know that if all tasks are in
> -	 * the fair class we can call that function directly:
> +	 * Optimization: we know that if all tasks are in the fair class we can
> +	 * call that function directly, but only if the @prev task wasn't of a
> +	 * higher scheduling class, because otherwise those loose the
> +	 * opportinity to pull in more work from other CPUs.
> +	 *
> +	 * Depends on link order in kernel/sched/Makefile.
>  	 */
> -	if (likely(rq->nr_running == rq->cfs.h_nr_running)) {
> +	if (likely(rq->nr_running == rq->cfs.h_nr_running &&
> +		   prev->sched_class <= &fair_sched_class)) {

If we go this route, I would suggest that we hardcode the classes in
vmlinux.lds.h.

-- Steve

> +
>  		p = fair_sched_class.pick_next_task(rq, prev, rf);
>  		if (unlikely(p == RETRY_TASK))
>  			goto again;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web