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

Groups > linux.kernel > #1220028 > unrolled thread

[PATCH] sched: fix lose fair sleeper bonus in switch_to_fair()

Back to article view | Back to linux.kernel

Contents

  [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Wanpeng Li <wanpeng.li@hotmail.com> - 2015-09-07 11:50 +0200
    Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Peter Zijlstra <peterz@infradead.org> - 2015-09-07 16:10 +0200
      Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Wanpeng Li <wanpeng.li@hotmail.com> - 2015-09-08 05:50 +0200
        Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Byungchul Park <byungchul.park@lge.com> - 2015-09-08 07:30 +0200
          Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Wanpeng Li <wanpeng.li@hotmail.com> - 2015-09-08 07:40 +0200
            Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Byungchul Park <byungchul.park@lge.com> - 2015-09-08 08:20 +0200
              Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Wanpeng Li <wanpeng.li@hotmail.com> - 2015-09-08 08:30 +0200
                Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Byungchul Park <byungchul.park@lge.com> - 2015-09-08 08:50 +0200
              Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Byungchul Park <byungchul.park@lge.com> - 2015-09-08 08:40 +0200
                Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Wanpeng Li <wanpeng.li@hotmail.com> - 2015-09-08 08:50 +0200
                  Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Byungchul Park <byungchul.park@lge.com> - 2015-09-08 09:20 +0200
                    Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Wanpeng Li <wanpeng.li@hotmail.com> - 2015-09-08 09:40 +0200
                      Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Byungchul Park <byungchul.park@lge.com> - 2015-09-08 10:00 +0200
                        Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Wanpeng Li <wanpeng.li@hotmail.com> - 2015-09-08 10:10 +0200
                          Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Byungchul Park <byungchul.park@lge.com> - 2015-09-08 10:30 +0200
                            Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Wanpeng Li <wanpeng.li@hotmail.com> - 2015-09-08 10:40 +0200
                              RE: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() "byungchul.park" <byungchul.park@lge.com> - 2015-09-08 10:50 +0200
                              Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Wanpeng Li <wanpeng.li@hotmail.com> - 2015-09-08 10:50 +0200
                                RE: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() "byungchul.park" <byungchul.park@lge.com> - 2015-09-08 11:00 +0200
                                Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Byungchul Park <byungchul.park@lge.com> - 2015-09-08 11:20 +0200
                                  Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Peter Zijlstra <peterz@infradead.org> - 2015-09-08 13:30 +0200
          Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Byungchul Park <byungchul.park@lge.com> - 2015-09-08 08:50 +0200
      Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Peter Zijlstra <peterz@infradead.org> - 2015-09-08 11:50 +0200
    Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Byungchul Park <byungchul.park@lge.com> - 2015-09-08 04:20 +0200
      Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() Wanpeng Li <wanpeng.li@hotmail.com> - 2015-09-08 09:00 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1220709

On Tue, Sep 08, 2015 at 06:17:49PM +0900, Byungchul Park wrote:
> even in this case, as i already said, place_entity() should be performed
> after restoring a normalized value to the meaningful original value.

> IMHO, it is wrong that se->vruntime is assigned with a bonused value
> unconditionally. when the task was detached, place_entity() made the
> entity have a bonused vruntime if it was eligible. if it was not
> eligible e.g. it had a too big vruntime before going to sleep, then
> we should not assign a bonused vruntime which is originally intended by
> place_entity().

Correct.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1220519

On Tue, Sep 08, 2015 at 02:28:17PM +0900, Byungchul Park wrote:
> On Tue, Sep 08, 2015 at 11:46:01AM +0800, Wanpeng Li wrote:
> > On 9/7/15 10:02 PM, Peter Zijlstra wrote:
> > >Please always Cc at least the person who wrote the lines you modify.
> > >
> > >On Mon, Sep 07, 2015 at 05:45:20PM +0800, Wanpeng Li wrote:
> > >>The sleeper task will be normalized when moved from fair_sched_class, in
> > >>order that vruntime will be adjusted either the task is running or sleeping
> > >>when moved back. The nomalization in switch_to_fair for sleep task will
> > >>result in lose fair sleeper bonus in place_entity() once the vruntime -
> > >>cfs_rq->min_vruntime is big when moved from fair_sched_class.
> > >>
> > >>This patch fix it by adjusting vruntime just during migrating as original
> > >>codes since the vruntime of the task has usually NOT been normalized in
> > >>this case.
> > >Sorry, I cannot follow that at all. Maybe its me being sleep deprived,
> > >but could you try that again?
> > 
> > When changing away from the fair class while sleeping, relative
> > vruntime is calculated to handle the case sleep when moved from
> > fair_sched_class and running when moved to fair_sched_class. The
> 
> i don't think relative vruntime is calculated to handle the special case
> you mentioned. i think the calculation is necessary for all cases detaching
> a task from a cfs_rq.
> 
> > absolute vruntime will be calculated in enqueue_entity() either the
> > task is running or sleeping when moved back. The fair sleeper bonus
> 
> i think absolute vruntime is calculated in enqueue_entuty() only when the

i mean enqueue_entity(.flags=0), just when moved back.

> task is on rq. therefore in the case that the task is not on rq,
> switched_to_fair() has to calculate the absolute vruntime instread.
> 
> > should be gained in place_entity() if the task is still sleeping.
> > However, after recent commit ( 23ec30ddd7c1306: 'sched: add two
> > functions for att(det)aching a task to(from) a cfs_rq'), the
> > absolute vruntime will be calculated in switched_to_fair(), so the
> > max_vruntime() which is called in place_entity() will select the
> > absolute vruntime which is calculated in switched_to_fair() as the
> > se->vruntime and lose the fair sleeper bonus.
> 
> please refer my another reply, and let me know if i missed something.
> 
> thanks,
> byungchul
> 
> > 
> > Regards,
> > Wanpeng Li
> > 
> > >
> > >>Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> > >>---
> > >>  kernel/sched/fair.c |   11 +++++++----
> > >>  1 files changed, 7 insertions(+), 4 deletions(-)
> > >>
> > >>diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > >>index d26d3b7..eb9aa35 100644
> > >>--- a/kernel/sched/fair.c
> > >>+++ b/kernel/sched/fair.c
> > >>@@ -8005,9 +8005,6 @@ static void attach_task_cfs_rq(struct task_struct *p)
> > >>  	/* Synchronize task with its cfs_rq */
> > >>  	attach_entity_load_avg(cfs_rq, se);
> > >>-
> > >>-	if (!vruntime_normalized(p))
> > >>-		se->vruntime += cfs_rq->min_vruntime;
> > >>  }
> > >>  static void switched_from_fair(struct rq *rq, struct task_struct *p)
> > >>@@ -8066,14 +8063,20 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
> > >>  #ifdef CONFIG_FAIR_GROUP_SCHED
> > >>  static void task_move_group_fair(struct task_struct *p)
> > >>  {
> > >>+	struct sched_entity *se = &p->se;
> > >>+	struct cfs_rq *cfs_rq = cfs_rq_of(se);
> > >>+
> > >>  	detach_task_cfs_rq(p);
> > >>  	set_task_rq(p, task_cpu(p));
> > >>  #ifdef CONFIG_SMP
> > >>  	/* Tell se's cfs_rq has been changed -- migrated */
> > >>-	p->se.avg.last_update_time = 0;
> > >>+	se->avg.last_update_time = 0;
> > >>  #endif
> > >>  	attach_task_cfs_rq(p);
> > >>+
> > >>+	if (!vruntime_normalized(p))
> > >>+		se->vruntime += cfs_rq->min_vruntime;
> > >>  }
> > >>  void free_fair_sched_group(struct task_group *tg)
> > >>-- 
> > >>1.7.1
> > >>
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1220656

On Tue, Sep 08, 2015 at 06:48:43AM +0800, Wanpeng Li wrote:
> On 9/7/15 10:02 PM, Peter Zijlstra wrote:
> >Please always Cc at least the person who wrote the lines you modify.
> >
> >On Mon, Sep 07, 2015 at 05:45:20PM +0800, Wanpeng Li wrote:
> >>The sleeper task will be normalized when moved from fair_sched_class, in
> >>order that vruntime will be adjusted either the task is running or sleeping
> >>when moved back. The nomalization in switch_to_fair for sleep task will
> >>result in lose fair sleeper bonus in place_entity() once the vruntime -
> >>cfs_rq->min_vruntime is big when moved from fair_sched_class.
> >>
> >>This patch fix it by adjusting vruntime just during migrating as original
> >>codes since the vruntime of the task has usually NOT been normalized in
> >>this case.

> >Sorry, I cannot follow that at all. Maybe its me being sleep deprived,
> >but could you try that again?
> 
> When changing away from the fair class while sleeping, relative vruntime is
> calculated to handle the case sleep when moved from fair_sched_class and
> running when moved to fair_sched_class.

That, or the task being migrated to a different cgroup / cpu while being
outside of the fair class.

Also, the 'relative vruntime' as you call it, is an approximation for
lag. Because we do not compute the 0-lag point (too expensive) we use
min_vruntime as a conservative approximation.

Lag is something you can transfer between runqueues, so that is the
natural state for something that is not associated with a rq.

> The absolute vruntime will be
> calculated in enqueue_entity() either the task is running or sleeping when
> moved back.

Incorrect, enqueue_entity() will only do that conditionally, in the
other cases it will assume se->vruntime is already absolute as you call
it.

attach_task_cfs_rq() must deal with the other cases.

> The fair sleeper bonus should be gained in place_entity() if the
> task is still sleeping. 

And this is still true. place_entity() assumes 'absolute vruntime', no
matter how it got there. If we went to relative/lag, someone needs to go
back to 'absolute'.

> However, after recent commit ( 23ec30ddd7c1306:
> 'sched: add two functions for att(det)aching a task to(from) a cfs_rq'), the
> absolute vruntime will be calculated in switched_to_fair(),

Also, conditionally, to complement the other places.

> so the
> max_vruntime() which is called in place_entity() will select the absolute
> vruntime which is calculated in switched_to_fair() as the se->vruntime and
> lose the fair sleeper bonus.

You cannot loose your sleeper bonus by going to and from relative/lag
(or rather you can, but that's due to min_vruntime being a poor
substitute for the 0-lag point). But since we do this for all rq
transfers we should not make exemptions.


Afaict, the only possibly place for a bug to be here is
vruntime_normalized(), if that somehow gets the conditions wrong we
could fail-to/incorrectly subtract/add min_vruntime, creating a mess.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1220458

hello wanpeng,

On Mon, Sep 07, 2015 at 05:45:20PM +0800, Wanpeng Li wrote:
> The sleeper task will be normalized when moved from fair_sched_class, in 
> order that vruntime will be adjusted either the task is running or sleeping 
> when moved back. The nomalization in switch_to_fair for sleep task will 
> result in lose fair sleeper bonus in place_entity() once the vruntime - 
> cfs_rq->min_vruntime is big when moved from fair_sched_class.

it is nothing to do with normalization.

if vruntime - cfs_rq->min_vruntime is big even though place_entity() was
called when moved from fair class, then we actually expect that it still has
a big vruntime when moved back to fair class.

if we don't expect that it still has a big vruntime when moved back to fair
class, we need to consider other approaches e.g. to move a position calling 
place_entity() or to add place_entity() call properly ..

however we should not touch normalization logic. in other words, if we
normalized the vruntime when leaved, then we should necessarily restore the
vruntime to a non-normalized value when moved back.

> 
> This patch fix it by adjusting vruntime just during migrating as original 
> codes since the vruntime of the task has usually NOT been normalized in 
> this case.

could you explain this in detail? when is a vruntime not normalized?

thanks,
byungchul

> 
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  kernel/sched/fair.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d26d3b7..eb9aa35 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8005,9 +8005,6 @@ static void attach_task_cfs_rq(struct task_struct *p)
>  
>  	/* Synchronize task with its cfs_rq */
>  	attach_entity_load_avg(cfs_rq, se);
> -
> -	if (!vruntime_normalized(p))
> -		se->vruntime += cfs_rq->min_vruntime;
>  }
>  
>  static void switched_from_fair(struct rq *rq, struct task_struct *p)
> @@ -8066,14 +8063,20 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
>  #ifdef CONFIG_FAIR_GROUP_SCHED
>  static void task_move_group_fair(struct task_struct *p)
>  {
> +	struct sched_entity *se = &p->se;
> +	struct cfs_rq *cfs_rq = cfs_rq_of(se);
> +
>  	detach_task_cfs_rq(p);
>  	set_task_rq(p, task_cpu(p));
>  
>  #ifdef CONFIG_SMP
>  	/* Tell se's cfs_rq has been changed -- migrated */
> -	p->se.avg.last_update_time = 0;
> +	se->avg.last_update_time = 0;
>  #endif
>  	attach_task_cfs_rq(p);
> +
> +	if (!vruntime_normalized(p))
> +		se->vruntime += cfs_rq->min_vruntime;
>  }
>  
>  void free_fair_sched_group(struct task_group *tg)
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1220525

On 9/8/15 10:10 AM, Byungchul Park wrote:
> hello wanpeng,
>
> On Mon, Sep 07, 2015 at 05:45:20PM +0800, Wanpeng Li wrote:
>> The sleeper task will be normalized when moved from fair_sched_class, in
>> order that vruntime will be adjusted either the task is running or sleeping
>> when moved back. The nomalization in switch_to_fair for sleep task will
>> result in lose fair sleeper bonus in place_entity() once the vruntime -
>> cfs_rq->min_vruntime is big when moved from fair_sched_class.
> it is nothing to do with normalization.
>
> if vruntime - cfs_rq->min_vruntime is big even though place_entity() was
> called when moved from fair class, then we actually expect that it still has
> a big vruntime when moved back to fair class.
>
> if we don't expect that it still has a big vruntime when moved back to fair
> class, we need to consider other approaches e.g. to move a position calling
> place_entity() or to add place_entity() call properly ..
>
> however we should not touch normalization logic. in other words, if we
> normalized the vruntime when leaved, then we should necessarily restore the
> vruntime to a non-normalized value when moved back.

Not about vruntime - cfs_rq->min_vruntime is big, I think my patch 
description above is confusing, and what's wrong I found is explained in 
the mail which reply to Peterz.

>
>> This patch fix it by adjusting vruntime just during migrating as original
>> codes since the vruntime of the task has usually NOT been normalized in
>> this case.
> could you explain this in detail? when is a vruntime not normalized?
>

The comments in task_move_group_fair() which you removed in your commit:

|  * When !queued, vruntime of the task has usually NOT been normalized

Regards,
Wanpeng Li
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web