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

Groups > linux.kernel > #1424989 > unrolled thread

[PATCH 0/4] sched/fair: Fix PELT wobblies

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] sched/fair: Fix PELT wobblies Peter Zijlstra <peterz@infradead.org> - 2016-06-17 14:10 +0200
    [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-17 14:10 +0200
      Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-17 16:10 +0200
        Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-17 16:30 +0200
          Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-17 18:10 +0200
            Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-17 18:20 +0200
            Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-17 18:20 +0200
              Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Yuyang Du <yuyang.du@intel.com> - 2016-06-20 09:00 +0200
              Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-20 11:30 +0200
                Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-20 12:00 +0200
                  Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-20 12:10 +0200
                Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-21 13:50 +0200
                  Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-21 14:40 +0200
                    Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-21 14:50 +0200
                      Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-21 15:10 +0200
              Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-06-20 14:00 +0200
                Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-20 14:40 +0200
                  Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-06-20 17:00 +0200
                    Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-21 10:50 +0200
                      Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Yuyang Du <yuyang.du@intel.com> - 2016-06-21 14:50 +0200
                        Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-24 15:10 +0200
                      Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-21 15:40 +0200
                        Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-22 13:50 +0200
                      Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-21 16:20 +0200
                      Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-06-23 17:40 +0200
                        Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-23 19:20 +0200
                Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-20 16:50 +0200
      Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra <peterz@infradead.org> - 2016-06-23 13:20 +0200
    [PATCH 2/4] sched/fair: Fix PELT integrity for new groups Peter Zijlstra <peterz@infradead.org> - 2016-06-17 14:10 +0200
      Re: [PATCH 2/4] sched/fair: Fix PELT integrity for new groups Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-17 16:00 +0200
    [PATCH 3/4] sched,cgroup: Fix cpu_cgroup_fork() Peter Zijlstra <peterz@infradead.org> - 2016-06-17 14:10 +0200
      Re: [PATCH 3/4] sched,cgroup: Fix cpu_cgroup_fork() Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-17 16:00 +0200
        Re: [PATCH 3/4] sched,cgroup: Fix cpu_cgroup_fork() Peter Zijlstra <peterz@infradead.org> - 2016-06-17 16:10 +0200

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

#1430648 — Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks

Sorry, I only spotted your reply yesterday.

On Tue, Jun 21, 2016 at 12:51:39PM +0800, Yuyang Du wrote:
> On Tue, Jun 21, 2016 at 10:41:19AM +0200, Peter Zijlstra wrote:

> > The things we ran into with these patches were that:
> > 
> >  1) You need to update the cfs_rq _before_ any entity attach/detach
> >     (and might need to update_tg_load_avg when update_cfs_rq_load_avg()
> >     returns true).
> 
> This is intrinsically an additional update, not a fix to anything. I
> don't think it is a must, but I am fine with it.

> > Esp. 1 is important, because while for mathematically consistency you
> > don't actually need to do this, you only need the entities to be
> > up-to-date with the cfs rq when you attach/detach, but that forgets the
> > temporal aspect of _when_ you do this.
>  
> Yes, temporally at any instant the avgs are outdated. But, I can have it,
> and what if I have it?

So I see your point; but there's a big difference between 'instant' and
10ms (HZ=100). So by aging the cfs_rq to the instant we fix two issues:

 - that it can be up to 10ms stale
 - that is can be 'uninitialized' at all

It also makes code consistent, all other sites also do this.

> >  3) cpu migration is the only exception and uses the last_update_time=0
> >     thing -- because refusal to take second rq->lock.
> 
> Task's last_update_time means this task is detached from fair queue. This
> (re)definition is by all means much better than migrating. No?

I would maybe redefine it as an up-to-date marker for a migration across
a clock discontinuity. Both CPU migration and group movement suffer from
this, albeit for different reasons.

In the CPU migration case we simply cannot tell time by our refusal to
acquire the old rq lock. So we age to the last time we 'know' and then
mark it up-to-date.

For the cgroup move the timelines simply _are_ discontinuous. So we have
to mark it up-to-date after we update it to the instant of detach, such
that when we attach it to the new group we don't try to age it across
the time difference.

> > Which is why I dislike Yuyang's patches, they create more exceptions
> > instead of applying existing rules (albeit undocumented).
> > 
> 
> I am thinking about document this really well, like "An art of load tracking:
> accuracy, overhead, and usefulness", seriously.

Any attempt to document all this would be greatly appreciated, although
I would like it to be in comments in the fair.c file itself if possible.

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

#1427761 — Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks

On 21 June 2016 at 15:17, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jun 21, 2016 at 10:41:19AM +0200, Peter Zijlstra wrote:
>> On Mon, Jun 20, 2016 at 03:49:34PM +0100, Dietmar Eggemann wrote:
>> > On 20/06/16 13:35, Vincent Guittot wrote:
>>
>> > > It will go through wake_up_new_task and post_init_entity_util_avg
>> > > during its fork which is enough to set last_update_time. Then, it will
>> > > use the switched_to_fair if the task becomes a fair one
>> >
>> > Oh I see. We want to make sure that every task (even when forked as
>> > !fair) has a last_update_time value != 0, when becoming fair one day.
>>
>> Right, see 2 below. I need to write a bunch of comments explaining PELT
>> proper, as well as document these things.
>>
>> The things we ran into with these patches were that:
>>
>>  1) You need to update the cfs_rq _before_ any entity attach/detach
>>     (and might need to update_tg_load_avg when update_cfs_rq_load_avg()
>>     returns true).
>>
>>  2) (fair) entities are always attached, switched_from/to deal with !fair.
>>
>>  3) cpu migration is the only exception and uses the last_update_time=0
>>     thing -- because refusal to take second rq->lock.
>>
>> Which is why I dislike Yuyang's patches, they create more exceptions
>> instead of applying existing rules (albeit undocumented).
>>
>> Esp. 1 is important, because while for mathematically consistency you
>> don't actually need to do this, you only need the entities to be
>> up-to-date with the cfs rq when you attach/detach, but that forgets the
>> temporal aspect of _when_ you do this.
>
> I have the below for now, I'll continue poking at this for a bit.
>
>
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -692,6 +692,7 @@ void init_entity_runnable_average(struct
>
>  static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq);
>  static int update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq, bool update_freq);
> +static void update_tg_load_avg(struct cfs_rq *cfs_rq, int force);
>  static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se);
>
>  /*
> @@ -757,7 +758,8 @@ void post_init_entity_util_avg(struct sc
>                 }
>         }
>
> -       update_cfs_rq_load_avg(now, cfs_rq, false);
> +       if (update_cfs_rq_load_avg(now, cfs_rq, false))
> +               update_tg_load_avg(cfs_rq, false);

You should move update_tg_load_avg after attach_entity_load_avg to
take into account the newly attached task

>         attach_entity_load_avg(cfs_rq, se);
>  }
>
> @@ -2919,7 +2921,21 @@ static inline void cfs_rq_util_change(st
>         WRITE_ONCE(*ptr, res);                                  \
>  } while (0)
>
> -/* Group cfs_rq's load_avg is used for task_h_load and update_cfs_share */
> +/**
> + * update_cfs_rq_load_avg - update the cfs_rq's load/util averages
> + * @now: current time, as per cfs_rq_clock_task()
> + * @cfs_rq: cfs_rq to update
> + * @update_freq: should we call cfs_rq_util_change() or will the call do so
> + *
> + * The cfs_rq avg is the direct sum of all its entities (blocked and runnable)
> + * avg. The immediate corollary is that all (fair) tasks must be attached, see
> + * post_init_entity_util_avg().
> + *
> + * cfs_rq->avg is used for task_h_load() and update_cfs_share() for example.
> + *
> + * Returns true if the load decayed or we removed utilization. It is expected
> + * that one calls update_tg_load_avg() on this condition.
> + */
>  static inline int
>  update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq, bool update_freq)
>  {
> @@ -2974,6 +2990,14 @@ static inline void update_load_avg(struc
>                 update_tg_load_avg(cfs_rq, 0);
>  }
>
> +/**
> + * attach_entity_load_avg - attach this entity to its cfs_rq load avg
> + * @cfs_rq: cfs_rq to attach to
> + * @se: sched_entity to attach
> + *
> + * Must call update_cfs_rq_load_avg() before this, since we rely on
> + * cfs_rq->avg.last_update_time being current.
> + */
>  static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  {
>         if (!sched_feat(ATTACH_AGE_LOAD))
> @@ -3005,6 +3029,14 @@ static void attach_entity_load_avg(struc
>         cfs_rq_util_change(cfs_rq);
>  }
>
> +/**
> + * detach_entity_load_avg - detach this entity from its cfs_rq load avg
> + * @cfs_rq: cfs_rq to detach from
> + * @se: sched_entity to detach
> + *
> + * Must call update_cfs_rq_load_avg() before this, since we rely on
> + * cfs_rq->avg.last_update_time being current.
> + */
>  static void detach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  {
>         __update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq_of(cfs_rq)),
> @@ -8392,7 +8424,8 @@ static void detach_task_cfs_rq(struct ta
>         }
>
>         /* Catch up with the cfs_rq and remove our load when we leave */
> -       update_cfs_rq_load_avg(now, cfs_rq, false);
> +       if (update_cfs_rq_load_avg(now, cfs_rq, false))
> +               update_tg_load_avg(cfs_rq, false);

same as post_init_entity_util_avg. You should put it after the
detach_entity_load_avg

>         detach_entity_load_avg(cfs_rq, se);
>  }
>
> @@ -8411,7 +8444,8 @@ static void attach_task_cfs_rq(struct ta
>  #endif
>
>         /* Synchronize task with its cfs_rq */
> -       update_cfs_rq_load_avg(now, cfs_rq, false);
> +       if (update_cfs_rq_load_avg(now, cfs_rq, false))
> +               update_tg_load_avg(cfs_rq, false);

same as post_init_entity_util_avg

>         attach_entity_load_avg(cfs_rq, se);
>
>         if (!vruntime_normalized(p))

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

#1428713 — Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks

On Tue, Jun 21, 2016 at 03:29:49PM +0200, Vincent Guittot wrote:
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -692,6 +692,7 @@ void init_entity_runnable_average(struct
> >
> >  static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq);
> >  static int update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq, bool update_freq);
> > +static void update_tg_load_avg(struct cfs_rq *cfs_rq, int force);
> >  static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se);
> >
> >  /*
> > @@ -757,7 +758,8 @@ void post_init_entity_util_avg(struct sc
> >                 }
> >         }
> >
> > -       update_cfs_rq_load_avg(now, cfs_rq, false);
> > +       if (update_cfs_rq_load_avg(now, cfs_rq, false))
> > +               update_tg_load_avg(cfs_rq, false);
> 
> You should move update_tg_load_avg after attach_entity_load_avg to
> take into account the newly attached task

Right you are, I've also updated the comment to reflect this.

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

#1427804 — Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks

On Tue, Jun 21, 2016 at 10:41:19AM +0200, Peter Zijlstra wrote:
> On Mon, Jun 20, 2016 at 03:49:34PM +0100, Dietmar Eggemann wrote:
> > On 20/06/16 13:35, Vincent Guittot wrote:
> 
> > > It will go through wake_up_new_task and post_init_entity_util_avg
> > > during its fork which is enough to set last_update_time. Then, it will
> > > use the switched_to_fair if the task becomes a fair one
> > 
> > Oh I see. We want to make sure that every task (even when forked as
> > !fair) has a last_update_time value != 0, when becoming fair one day.
> 
> Right, see 2 below. I need to write a bunch of comments explaining PELT
> proper, as well as document these things.
> 
> The things we ran into with these patches were that:
> 
>  1) You need to update the cfs_rq _before_ any entity attach/detach
>     (and might need to update_tg_load_avg when update_cfs_rq_load_avg()
>     returns true).
> 
>  2) (fair) entities are always attached, switched_from/to deal with !fair.
> 
>  3) cpu migration is the only exception and uses the last_update_time=0
>     thing -- because refusal to take second rq->lock.
> 
> Which is why I dislike Yuyang's patches, they create more exceptions
> instead of applying existing rules (albeit undocumented).
> 
> Esp. 1 is important, because while for mathematically consistency you
> don't actually need to do this, you only need the entities to be
> up-to-date with the cfs rq when you attach/detach, but that forgets the
> temporal aspect of _when_ you do this.

I have the below for now, I'll continue poking at this for a bit.


--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -692,6 +692,7 @@ void init_entity_runnable_average(struct
 
 static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq);
 static int update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq, bool update_freq);
+static void update_tg_load_avg(struct cfs_rq *cfs_rq, int force);
 static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se);
 
 /*
@@ -757,7 +758,8 @@ void post_init_entity_util_avg(struct sc
 		}
 	}
 
-	update_cfs_rq_load_avg(now, cfs_rq, false);
+	if (update_cfs_rq_load_avg(now, cfs_rq, false))
+		update_tg_load_avg(cfs_rq, false);
 	attach_entity_load_avg(cfs_rq, se);
 }
 
@@ -2919,7 +2921,21 @@ static inline void cfs_rq_util_change(st
 	WRITE_ONCE(*ptr, res);					\
 } while (0)
 
-/* Group cfs_rq's load_avg is used for task_h_load and update_cfs_share */
+/**
+ * update_cfs_rq_load_avg - update the cfs_rq's load/util averages
+ * @now: current time, as per cfs_rq_clock_task()
+ * @cfs_rq: cfs_rq to update
+ * @update_freq: should we call cfs_rq_util_change() or will the call do so
+ *
+ * The cfs_rq avg is the direct sum of all its entities (blocked and runnable)
+ * avg. The immediate corollary is that all (fair) tasks must be attached, see
+ * post_init_entity_util_avg().
+ *
+ * cfs_rq->avg is used for task_h_load() and update_cfs_share() for example.
+ *
+ * Returns true if the load decayed or we removed utilization. It is expected
+ * that one calls update_tg_load_avg() on this condition.
+ */
 static inline int
 update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq, bool update_freq)
 {
@@ -2974,6 +2990,14 @@ static inline void update_load_avg(struc
 		update_tg_load_avg(cfs_rq, 0);
 }
 
+/**
+ * attach_entity_load_avg - attach this entity to its cfs_rq load avg
+ * @cfs_rq: cfs_rq to attach to
+ * @se: sched_entity to attach
+ *
+ * Must call update_cfs_rq_load_avg() before this, since we rely on
+ * cfs_rq->avg.last_update_time being current.
+ */
 static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
 	if (!sched_feat(ATTACH_AGE_LOAD))
@@ -3005,6 +3029,14 @@ static void attach_entity_load_avg(struc
 	cfs_rq_util_change(cfs_rq);
 }
 
+/**
+ * detach_entity_load_avg - detach this entity from its cfs_rq load avg
+ * @cfs_rq: cfs_rq to detach from
+ * @se: sched_entity to detach
+ *
+ * Must call update_cfs_rq_load_avg() before this, since we rely on
+ * cfs_rq->avg.last_update_time being current.
+ */
 static void detach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
 	__update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq_of(cfs_rq)),
@@ -8392,7 +8424,8 @@ static void detach_task_cfs_rq(struct ta
 	}
 
 	/* Catch up with the cfs_rq and remove our load when we leave */
-	update_cfs_rq_load_avg(now, cfs_rq, false);
+	if (update_cfs_rq_load_avg(now, cfs_rq, false))
+		update_tg_load_avg(cfs_rq, false);
 	detach_entity_load_avg(cfs_rq, se);
 }
 
@@ -8411,7 +8444,8 @@ static void attach_task_cfs_rq(struct ta
 #endif
 
 	/* Synchronize task with its cfs_rq */
-	update_cfs_rq_load_avg(now, cfs_rq, false);
+	if (update_cfs_rq_load_avg(now, cfs_rq, false))
+		update_tg_load_avg(cfs_rq, false);
 	attach_entity_load_avg(cfs_rq, se);
 
 	if (!vruntime_normalized(p))

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

#1429927 — Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks

On 21/06/16 09:41, Peter Zijlstra wrote:
> On Mon, Jun 20, 2016 at 03:49:34PM +0100, Dietmar Eggemann wrote:
>> On 20/06/16 13:35, Vincent Guittot wrote:
> 
>>> It will go through wake_up_new_task and post_init_entity_util_avg
>>> during its fork which is enough to set last_update_time. Then, it will
>>> use the switched_to_fair if the task becomes a fair one
>>
>> Oh I see. We want to make sure that every task (even when forked as
>> !fair) has a last_update_time value != 0, when becoming fair one day.
> 
> Right, see 2 below. I need to write a bunch of comments explaining PELT
> proper, as well as document these things.
> 
> The things we ran into with these patches were that:
> 
>  1) You need to update the cfs_rq _before_ any entity attach/detach
>     (and might need to update_tg_load_avg when update_cfs_rq_load_avg()
>     returns true).
> 
>  2) (fair) entities are always attached, switched_from/to deal with !fair.
> 
>  3) cpu migration is the only exception and uses the last_update_time=0
>     thing -- because refusal to take second rq->lock.

2) is about changing sched classes, 3) is about changing cpus but what
about 4) changing task groups?

There is still this last_update_time = 0 between
detach_task_cfs_rq()/set_task_rq() and attach_task_cfs_rq() in
task_move_group_fair() preventing the call __update_load_avg(...
p->se->avg, ...) in attach_task_cfs_rq() -> attach_entity_load_avg().

Shouldn't be necessary any more since cfs_rq 'next' is up-to-date now.

Assuming here that the exception in 3) relates to the fact that the
rq->lock is not taken.

Or is 4) a second exception in the sense that the se has been aged in
remove_entity_load_avg() (3)) resp. detach_entity_load_avg() (4))?

> Which is why I dislike Yuyang's patches, they create more exceptions
> instead of applying existing rules (albeit undocumented).
> 
> Esp. 1 is important, because while for mathematically consistency you
> don't actually need to do this, you only need the entities to be
> up-to-date with the cfs rq when you attach/detach, but that forgets the
> temporal aspect of _when_ you do this.

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

#1430001 — Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks

On Thu, Jun 23, 2016 at 04:35:54PM +0100, Dietmar Eggemann wrote:
> > The things we ran into with these patches were that:
> > 
> >  1) You need to update the cfs_rq _before_ any entity attach/detach
> >     (and might need to update_tg_load_avg when update_cfs_rq_load_avg()
> >     returns true).
> > 
> >  2) (fair) entities are always attached, switched_from/to deal with !fair.
> > 
> >  3) cpu migration is the only exception and uses the last_update_time=0
> >     thing -- because refusal to take second rq->lock.
> 
> 2) is about changing sched classes, 3) is about changing cpus but what
> about 4) changing task groups?
> 
> There is still this last_update_time = 0 between
> detach_task_cfs_rq()/set_task_rq() and attach_task_cfs_rq() in
> task_move_group_fair() preventing the call __update_load_avg(...
> p->se->avg, ...) in attach_task_cfs_rq() -> attach_entity_load_avg().
> 
> Shouldn't be necessary any more since cfs_rq 'next' is up-to-date now.
> 
> Assuming here that the exception in 3) relates to the fact that the
> rq->lock is not taken.
> 
> Or is 4) a second exception in the sense that the se has been aged in
> remove_entity_load_avg() (3)) resp. detach_entity_load_avg() (4))?

Ah, good point, so move between groups is also special in that the time
between cgroups doesn't need to match.

And since we've aged the se to 'now' on detach, we can assume its
up-to-date (and hence last_update_time=0) for attach, which then only
updates its cfs_rq to 'now' before attaching the se.

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

#1426662 — Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks

On Mon, Jun 20, 2016 at 12:35:59PM +0100, Dietmar Eggemann wrote:
> 
> 
> On 17/06/16 17:18, Peter Zijlstra wrote:
> > On Fri, Jun 17, 2016 at 06:02:39PM +0200, Peter Zijlstra wrote:
> >> So yes, ho-humm, how to go about doing that bestest. Lemme have a play.
> > 
> > This is what I came up with, not entirely pretty, but I suppose it'll
> > have to do.
> > 
> > ---
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -724,6 +724,7 @@ void post_init_entity_util_avg(struct sc
> >  	struct cfs_rq *cfs_rq = cfs_rq_of(se);
> >  	struct sched_avg *sa = &se->avg;
> >  	long cap = (long)(SCHED_CAPACITY_SCALE - cfs_rq->avg.util_avg) / 2;
> > +	u64 now = cfs_rq_clock_task(cfs_rq);
> >  
> >  	if (cap > 0) {
> >  		if (cfs_rq->avg.util_avg != 0) {
> > @@ -738,7 +739,20 @@ void post_init_entity_util_avg(struct sc
> >  		sa->util_sum = sa->util_avg * LOAD_AVG_MAX;
> >  	}
> >  
> > -	update_cfs_rq_load_avg(cfs_rq_clock_task(cfs_rq), cfs_rq, false);
> > +	if (entity_is_task(se)) {
> > +		struct task_struct *p = task_of(se);
> > +		if (p->sched_class != &fair_sched_class) {
> > +			/*
> > +			 * For !fair tasks do attach_entity_load_avg()
> > +			 * followed by detach_entity_load_avg() as per
> > +			 * switched_from_fair().
> > +			 */
> > +			se->avg.last_update_time = now;
> > +			return;
> > +		}
> > +	}
> > +
> > +	update_cfs_rq_load_avg(now, cfs_rq, false);
> >  	attach_entity_load_avg(cfs_rq, se);
> >  }
> >  
> > 
> 
> Doesn't a sleeping !fair_sched_class task which switches to fair uses
> try_to_wake_up() rather than wake_up_new_task() so it won't go through
> post_init_entity_util_avg()?

Its switched_to_fair() that'll fix that up.

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

#1429683 — Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks

On Fri, Jun 17, 2016 at 02:01:40PM +0200, Peter Zijlstra wrote:
> @@ -8219,6 +8254,19 @@ static int cpu_cgroup_can_attach(struct
>  		if (task->sched_class != &fair_sched_class)
>  			return -EINVAL;
>  #endif
> +		/*
> +		 * Serialize against wake_up_new_task() such
> +		 * that if its running, we're sure to observe
> +		 * its full state.
> +		 */
> +		raw_spin_unlock_wait(&task->pi_lock);
> +		/*
> +		 * Avoid calling sched_move_task() before wake_up_new_task()
> +		 * has happened. This would lead to problems with PELT. See
> +		 * XXX.
> +		 */
> +		if (task->state == TASK_NEW)
> +			return -EINVAL;
>  	}
>  	return 0;
>  }

So I think that's backwards; we want:

		if (task->state == TASK_NEW)
			return -EINVAL;
		raw_spin_unlock_wait(&task->pi_lock);

Because failing the attach is 'safe', but if we do not observe NEW we
must be absolutely sure to observe the full wakeup_new.

But since its not critical I'll change it to more obvious code:

		raw_spin_lock_irq(&task->pi_lock);
		if (task->state == TASK_NEW)
			ret = -EINVAL;
		raw_spin_unlock_irq(&task->pi_lock);

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

#1424995 — [PATCH 2/4] sched/fair: Fix PELT integrity for new groups

Vincent reported that when a new task is moved into a new cgroup it
gets attached twice to the load tracking.

  sched_move_task()
    task_move_group_fair()
      detach_task_cfs_rq()
      set_task_rq()
      attach_task_cfs_rq()
        attach_entity_load_avg()
          se->avg.last_load_update = cfs_rq->avg.last_load_update // == 0

  enqueue_entity()
    enqueue_entity_load_avg()
      update_cfs_rq_load_avg()
        now = clock()
        __update_load_avg(&cfs_rq->avg)
          cfs_rq->avg.last_load_update = now
          // ages load/util for: now - 0, load/util -> 0
      if (migrated)
        attach_entity_load_avg()
          se->avg.last_load_update = cfs_rq->avg.last_load_update; // now != 0

The problem is that we don't update cfs_rq load_avg before all
entity attach/detach operations. Only enqueue and migrate_task do
this.

By fixing this, the above will not happen, because the
sched_move_task() attach will have updated cfs_rq's last_load_update
time before attach, and in turn the attach will have set the entity's
last_load_update stamp.

Note that there is a further problem with sched_move_task() calling
detach on a task that hasn't yet been attached; this will be taken
care of in a subsequent patch.

Cc: Yuyang Du <yuyang.du@intel.com>
Reported-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/fair.c |    4 ++++
 1 file changed, 4 insertions(+)

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8366,6 +8366,7 @@ static void detach_task_cfs_rq(struct ta
 {
 	struct sched_entity *se = &p->se;
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
+	u64 now = cfs_rq_clock_task(cfs_rq);
 
 	if (!vruntime_normalized(p)) {
 		/*
@@ -8377,6 +8378,7 @@ static void detach_task_cfs_rq(struct ta
 	}
 
 	/* Catch up with the cfs_rq and remove our load when we leave */
+	update_cfs_rq_load_avg(now, cfs_rq, false);
 	detach_entity_load_avg(cfs_rq, se);
 }
 
@@ -8384,6 +8386,7 @@ static void attach_task_cfs_rq(struct ta
 {
 	struct sched_entity *se = &p->se;
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
+	u64 now = cfs_rq_clock_task(cfs_rq);
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	/*
@@ -8394,6 +8397,7 @@ static void attach_task_cfs_rq(struct ta
 #endif
 
 	/* Synchronize task with its cfs_rq */
+	update_cfs_rq_load_avg(now, cfs_rq, false);
 	attach_entity_load_avg(cfs_rq, se);
 
 	if (!vruntime_normalized(p))

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

#1425114 — Re: [PATCH 2/4] sched/fair: Fix PELT integrity for new groups

On 17 June 2016 at 14:01, Peter Zijlstra <peterz@infradead.org> wrote:
> Vincent reported that when a new task is moved into a new cgroup it

task doesn't have to be new only the cgroup

> gets attached twice to the load tracking.
>
>   sched_move_task()
>     task_move_group_fair()
>       detach_task_cfs_rq()
>       set_task_rq()
>       attach_task_cfs_rq()
>         attach_entity_load_avg()
>           se->avg.last_load_update = cfs_rq->avg.last_load_update // == 0
>
>   enqueue_entity()
>     enqueue_entity_load_avg()
>       update_cfs_rq_load_avg()
>         now = clock()
>         __update_load_avg(&cfs_rq->avg)
>           cfs_rq->avg.last_load_update = now
>           // ages load/util for: now - 0, load/util -> 0
>       if (migrated)
>         attach_entity_load_avg()
>           se->avg.last_load_update = cfs_rq->avg.last_load_update; // now != 0
>
> The problem is that we don't update cfs_rq load_avg before all
> entity attach/detach operations. Only enqueue and migrate_task do
> this.
>
> By fixing this, the above will not happen, because the
> sched_move_task() attach will have updated cfs_rq's last_load_update
> time before attach, and in turn the attach will have set the entity's
> last_load_update stamp.
>
> Note that there is a further problem with sched_move_task() calling
> detach on a task that hasn't yet been attached; this will be taken
> care of in a subsequent patch.

This patch fixes the double attach
Tested-by:  Vincent Guittot <vincent.guittot@linaro.org>

>
> Cc: Yuyang Du <yuyang.du@intel.com>
> Reported-by: Vincent Guittot <vincent.guittot@linaro.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/sched/fair.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8366,6 +8366,7 @@ static void detach_task_cfs_rq(struct ta
>  {
>         struct sched_entity *se = &p->se;
>         struct cfs_rq *cfs_rq = cfs_rq_of(se);
> +       u64 now = cfs_rq_clock_task(cfs_rq);
>
>         if (!vruntime_normalized(p)) {
>                 /*
> @@ -8377,6 +8378,7 @@ static void detach_task_cfs_rq(struct ta
>         }
>
>         /* Catch up with the cfs_rq and remove our load when we leave */
> +       update_cfs_rq_load_avg(now, cfs_rq, false);
>         detach_entity_load_avg(cfs_rq, se);
>  }
>
> @@ -8384,6 +8386,7 @@ static void attach_task_cfs_rq(struct ta
>  {
>         struct sched_entity *se = &p->se;
>         struct cfs_rq *cfs_rq = cfs_rq_of(se);
> +       u64 now = cfs_rq_clock_task(cfs_rq);
>
>  #ifdef CONFIG_FAIR_GROUP_SCHED
>         /*
> @@ -8394,6 +8397,7 @@ static void attach_task_cfs_rq(struct ta
>  #endif
>
>         /* Synchronize task with its cfs_rq */
> +       update_cfs_rq_load_avg(now, cfs_rq, false);
>         attach_entity_load_avg(cfs_rq, se);
>
>         if (!vruntime_normalized(p))
>
>

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

#1424998 — [PATCH 3/4] sched,cgroup: Fix cpu_cgroup_fork()

From: Vincent Guittot <vincent.guittot@linaro.org>

A new fair task is detached and attached from/to task_group with:

  cgroup_post_fork()
    ss->fork(child) := cpu_cgroup_fork()
      sched_move_task()
        task_move_group_fair()

Which is wrong, because at this point in fork() the task isn't fully
initialized and it cannot 'move' to another group, because its not
attached to any group as yet.

In fact, cpu_cgroup_fork needs a small part of sched_move_task so we
can just call this small part directly instead sched_move_task. And
the task doesn't really migrate because it is not yet attached so we
need the sequence:

  do_fork()
    sched_fork()
      __set_task_cpu()

    cgroup_post_fork()
      set_task_rq() # set task group and runqueue

    wake_up_new_task()
      select_task_rq() can select a new cpu
      __set_task_cpu
      post_init_entity_util_avg
        attach_task_cfs_rq()
      activate_task
        enqueue_task

This patch makes that happen.

Maybe-Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/core.c |   67 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 20 deletions(-)

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7743,27 +7743,17 @@ void sched_offline_group(struct task_gro
 	spin_unlock_irqrestore(&task_group_lock, flags);
 }
 
-/* change task's runqueue when it moves between groups.
- *	The caller of this function should have put the task in its new group
- *	by now. This function just updates tsk->se.cfs_rq and tsk->se.parent to
- *	reflect its new group.
+/*
+ * Set task's runqueue and group.
+ *
+ * In case of a move between group, we update src and dst group thanks to
+ * sched_class->task_move_group. Otherwise, we just need to set runqueue and
+ * group pointers. The task will be attached to the runqueue during its wake
+ * up.
  */
-void sched_move_task(struct task_struct *tsk)
+static void sched_set_group(struct task_struct *tsk, bool move)
 {
 	struct task_group *tg;
-	int queued, running;
-	struct rq_flags rf;
-	struct rq *rq;
-
-	rq = task_rq_lock(tsk, &rf);
-
-	running = task_current(rq, tsk);
-	queued = task_on_rq_queued(tsk);
-
-	if (queued)
-		dequeue_task(rq, tsk, DEQUEUE_SAVE | DEQUEUE_MOVE);
-	if (unlikely(running))
-		put_prev_task(rq, tsk);
 
 	/*
 	 * All callers are synchronized by task_rq_lock(); we do not use RCU
@@ -7776,11 +7766,37 @@ void sched_move_task(struct task_struct
 	tsk->sched_task_group = tg;
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
-	if (tsk->sched_class->task_move_group)
+	if (move && tsk->sched_class->task_move_group)
 		tsk->sched_class->task_move_group(tsk);
 	else
 #endif
 		set_task_rq(tsk, task_cpu(tsk));
+}
+
+/*
+ * Change task's runqueue when it moves between groups.
+ *
+ * The caller of this function should have put the task in its new group by
+ * now. This function just updates tsk->se.cfs_rq and tsk->se.parent to reflect
+ * its new group.
+ */
+void sched_move_task(struct task_struct *tsk)
+{
+	int queued, running;
+	struct rq_flags rf;
+	struct rq *rq;
+
+	rq = task_rq_lock(tsk, &rf);
+
+	running = task_current(rq, tsk);
+	queued = task_on_rq_queued(tsk);
+
+	if (queued)
+		dequeue_task(rq, tsk, DEQUEUE_SAVE | DEQUEUE_MOVE);
+	if (unlikely(running))
+		put_prev_task(rq, tsk);
+
+	sched_set_group(tsk, true);
 
 	if (unlikely(running))
 		tsk->sched_class->set_curr_task(rq);
@@ -8208,9 +8224,20 @@ static void cpu_cgroup_css_free(struct c
 	sched_free_group(tg);
 }
 
+/*
+ * This is called before wake_up_new_task(), therefore we really only
+ * have to set its group bits, all the other stuff does not apply.
+ */
 static void cpu_cgroup_fork(struct task_struct *task)
 {
-	sched_move_task(task);
+	struct rq_flags rf;
+	struct rq *rq;
+
+	rq = task_rq_lock(task, &rf);
+
+	sched_set_group(task, false);
+
+	task_rq_unlock(rq, task, &rf);
 }
 
 static int cpu_cgroup_can_attach(struct cgroup_taskset *tset)

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

#1425118 — Re: [PATCH 3/4] sched,cgroup: Fix cpu_cgroup_fork()

On 17 June 2016 at 14:01, Peter Zijlstra <peterz@infradead.org> wrote:
> From: Vincent Guittot <vincent.guittot@linaro.org>
>
> A new fair task is detached and attached from/to task_group with:
>
>   cgroup_post_fork()
>     ss->fork(child) := cpu_cgroup_fork()
>       sched_move_task()
>         task_move_group_fair()
>
> Which is wrong, because at this point in fork() the task isn't fully
> initialized and it cannot 'move' to another group, because its not
> attached to any group as yet.
>
> In fact, cpu_cgroup_fork needs a small part of sched_move_task so we
> can just call this small part directly instead sched_move_task. And
> the task doesn't really migrate because it is not yet attached so we
> need the sequence:
>
>   do_fork()
>     sched_fork()
>       __set_task_cpu()
>
>     cgroup_post_fork()
>       set_task_rq() # set task group and runqueue
>
>     wake_up_new_task()
>       select_task_rq() can select a new cpu
>       __set_task_cpu
>       post_init_entity_util_avg
>         attach_task_cfs_rq()
>       activate_task
>         enqueue_task
>
> This patch makes that happen.
>

With this patch and patch 1, the fork sequence looks correct in my test

> Maybe-Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

You can remove the Maybe if you want

> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/sched/core.c |   67 ++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 47 insertions(+), 20 deletions(-)
>
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7743,27 +7743,17 @@ void sched_offline_group(struct task_gro
>         spin_unlock_irqrestore(&task_group_lock, flags);
>  }
>
> -/* change task's runqueue when it moves between groups.
> - *     The caller of this function should have put the task in its new group
> - *     by now. This function just updates tsk->se.cfs_rq and tsk->se.parent to
> - *     reflect its new group.
> +/*
> + * Set task's runqueue and group.
> + *
> + * In case of a move between group, we update src and dst group thanks to
> + * sched_class->task_move_group. Otherwise, we just need to set runqueue and
> + * group pointers. The task will be attached to the runqueue during its wake
> + * up.
>   */
> -void sched_move_task(struct task_struct *tsk)
> +static void sched_set_group(struct task_struct *tsk, bool move)
>  {
>         struct task_group *tg;
> -       int queued, running;
> -       struct rq_flags rf;
> -       struct rq *rq;
> -
> -       rq = task_rq_lock(tsk, &rf);
> -
> -       running = task_current(rq, tsk);
> -       queued = task_on_rq_queued(tsk);
> -
> -       if (queued)
> -               dequeue_task(rq, tsk, DEQUEUE_SAVE | DEQUEUE_MOVE);
> -       if (unlikely(running))
> -               put_prev_task(rq, tsk);
>
>         /*
>          * All callers are synchronized by task_rq_lock(); we do not use RCU
> @@ -7776,11 +7766,37 @@ void sched_move_task(struct task_struct
>         tsk->sched_task_group = tg;
>
>  #ifdef CONFIG_FAIR_GROUP_SCHED
> -       if (tsk->sched_class->task_move_group)
> +       if (move && tsk->sched_class->task_move_group)
>                 tsk->sched_class->task_move_group(tsk);
>         else
>  #endif
>                 set_task_rq(tsk, task_cpu(tsk));
> +}
> +
> +/*
> + * Change task's runqueue when it moves between groups.
> + *
> + * The caller of this function should have put the task in its new group by
> + * now. This function just updates tsk->se.cfs_rq and tsk->se.parent to reflect
> + * its new group.
> + */
> +void sched_move_task(struct task_struct *tsk)
> +{
> +       int queued, running;
> +       struct rq_flags rf;
> +       struct rq *rq;
> +
> +       rq = task_rq_lock(tsk, &rf);
> +
> +       running = task_current(rq, tsk);
> +       queued = task_on_rq_queued(tsk);
> +
> +       if (queued)
> +               dequeue_task(rq, tsk, DEQUEUE_SAVE | DEQUEUE_MOVE);
> +       if (unlikely(running))
> +               put_prev_task(rq, tsk);
> +
> +       sched_set_group(tsk, true);
>
>         if (unlikely(running))
>                 tsk->sched_class->set_curr_task(rq);
> @@ -8208,9 +8224,20 @@ static void cpu_cgroup_css_free(struct c
>         sched_free_group(tg);
>  }
>
> +/*
> + * This is called before wake_up_new_task(), therefore we really only
> + * have to set its group bits, all the other stuff does not apply.
> + */
>  static void cpu_cgroup_fork(struct task_struct *task)
>  {
> -       sched_move_task(task);
> +       struct rq_flags rf;
> +       struct rq *rq;
> +
> +       rq = task_rq_lock(task, &rf);
> +
> +       sched_set_group(task, false);
> +
> +       task_rq_unlock(rq, task, &rf);
>  }
>
>  static int cpu_cgroup_can_attach(struct cgroup_taskset *tset)
>
>

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

#1425122 — Re: [PATCH 3/4] sched,cgroup: Fix cpu_cgroup_fork()

On Fri, Jun 17, 2016 at 03:58:37PM +0200, Vincent Guittot wrote:
> On 17 June 2016 at 14:01, Peter Zijlstra <peterz@infradead.org> wrote:
> > From: Vincent Guittot <vincent.guittot@linaro.org>
> >
> > A new fair task is detached and attached from/to task_group with:
> >
> >   cgroup_post_fork()
> >     ss->fork(child) := cpu_cgroup_fork()
> >       sched_move_task()
> >         task_move_group_fair()
> >
> > Which is wrong, because at this point in fork() the task isn't fully
> > initialized and it cannot 'move' to another group, because its not
> > attached to any group as yet.
> >
> > In fact, cpu_cgroup_fork needs a small part of sched_move_task so we
> > can just call this small part directly instead sched_move_task. And
> > the task doesn't really migrate because it is not yet attached so we
> > need the sequence:
> >
> >   do_fork()
> >     sched_fork()
> >       __set_task_cpu()
> >
> >     cgroup_post_fork()
> >       set_task_rq() # set task group and runqueue
> >
> >     wake_up_new_task()
> >       select_task_rq() can select a new cpu
> >       __set_task_cpu
> >       post_init_entity_util_avg
> >         attach_task_cfs_rq()
> >       activate_task
> >         enqueue_task
> >
> > This patch makes that happen.
> >
> 
> With this patch and patch 1, the fork sequence looks correct in my test
> 
> > Maybe-Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> 
> You can remove the Maybe if you want

Thanks!

[toc] | [prev] | [standalone]

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

Back to top | Article view | linux.kernel

csiph-web