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

Groups > linux.kernel > #1406136 > unrolled thread

[PATCH] sched: fix first task of a task group is attached twice

Back to article view | Back to linux.kernel

Contents

  [PATCH] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-05-24 15:10 +0200
    [PATCH v2] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-05-25 17:10 +0200
      Re: [PATCH v2] sched: fix first task of a task group is attached  twice Yuyang Du <yuyang.du@intel.com> - 2016-05-26 08:40 +0200
        Re: [PATCH v2] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-05-26 10:30 +0200
          Re: [PATCH v2] sched: fix first task of a task group is attached  twice Yuyang Du <yuyang.du@intel.com> - 2016-05-26 10:40 +0200
            Re: [PATCH v2] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-05-26 11:00 +0200
      Re: [PATCH v2] sched: fix first task of a task group is attached  twice Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-05-27 17:50 +0200
        Re: [PATCH v2] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-05-27 19:20 +0200
          Re: [PATCH v2] sched: fix first task of a task group is attached  twice Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-05-27 22:40 +0200
            Re: [PATCH v2] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-05-30 09:10 +0200
              [PATCH v3] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-05-30 18:00 +0200
                Re: [PATCH v3] sched: fix first task of a task group is attached  twice Yuyang Du <yuyang.du@intel.com> - 2016-05-31 05:50 +0200
                  Re: [PATCH v3] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-05-31 09:30 +0200
                    Re: [PATCH v3] sched: fix first task of a task group is attached  twice Yuyang Du <yuyang.du@intel.com> - 2016-05-31 10:50 +0200
            Re: [PATCH v2] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-05-30 18:00 +0200

#1406136 — [PATCH] sched: fix first task of a task group is attached twice

The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
that the 1st sched_entity that will be attached, will keep its
last_update_time set to 0 and will attached once again during the
enqueue.
Initialize cfs_rq->avg.last_update_time to current rq's clock

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/fair.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 218f8e8..a4e2c10 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8586,6 +8586,14 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
 		se->depth = parent->depth + 1;
 	}
 
+	/*
+	 * Set last_update_time to something different from 0 to make
+	 * sure the 1st sched_entity will not be attached twice: once
+	 * when attaching the task to the group and one more time when
+	 * enqueueing the task.
+	 */
+	tg->cfs_rq[cpu]->avg.last_update_time = rq_clock_task(rq_of(cfs_rq));
+
 	se->my_q = cfs_rq;
 	/* guarantee group entities always have weight */
 	update_load_set(&se->load, NICE_0_LOAD);
-- 
1.9.1

[toc] | [next] | [standalone]

#1406991 — [PATCH v2] sched: fix first task of a task group is attached twice

The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
that the 1st sched_entity that will be attached, will keep its
last_update_time set to 0 and will attached once again during the
enqueue.
Initialize cfs_rq->avg.last_update_time to 1 instead.

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

v2:
- rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held

 kernel/sched/fair.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 218f8e8..3724656 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8586,6 +8586,14 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
 		se->depth = parent->depth + 1;
 	}
 
+	/*
+	 * Set last_update_time to something different from 0 to make
+	 * sure the 1st sched_entity will not be attached twice: once
+	 * when attaching the task to the group and one more time when
+	 * enqueueing the task.
+	 */
+	tg->cfs_rq[cpu]->avg.last_update_time = 1;
+
 	se->my_q = cfs_rq;
 	/* guarantee group entities always have weight */
 	update_load_set(&se->load, NICE_0_LOAD);
-- 
1.9.1

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

#1407407 — Re: [PATCH v2] sched: fix first task of a task group is attached twice

On Wed, May 25, 2016 at 05:01:11PM +0200, Vincent Guittot wrote:
> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
> that the 1st sched_entity that will be attached, will keep its
> last_update_time set to 0 and will attached once again during the
> enqueue.
> Initialize cfs_rq->avg.last_update_time to 1 instead.
 
Actually, the first time (attach_task_cfs_rq()) is called even way
before init_entity_runnable_average(), no?

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

#1407449 — Re: [PATCH v2] sched: fix first task of a task group is attached twice

On 26 May 2016 at 00:38, Yuyang Du <yuyang.du@intel.com> wrote:
> On Wed, May 25, 2016 at 05:01:11PM +0200, Vincent Guittot wrote:
>> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
>> that the 1st sched_entity that will be attached, will keep its
>> last_update_time set to 0 and will attached once again during the
>> enqueue.
>> Initialize cfs_rq->avg.last_update_time to 1 instead.
>
> Actually, the first time (attach_task_cfs_rq()) is called even way
> before init_entity_runnable_average(), no?

maybe there is an issue during the fork of a task too
This patch is about the init of the sched_avg of a task group. The
problem happens when the 1st task is moved the group but it's not
about the fork of a task

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

#1407454 — Re: [PATCH v2] sched: fix first task of a task group is attached twice

On Thu, May 26, 2016 at 10:26:54AM +0200, Vincent Guittot wrote:
> On 26 May 2016 at 00:38, Yuyang Du <yuyang.du@intel.com> wrote:
> > On Wed, May 25, 2016 at 05:01:11PM +0200, Vincent Guittot wrote:
> >> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
> >> that the 1st sched_entity that will be attached, will keep its
> >> last_update_time set to 0 and will attached once again during the
> >> enqueue.
> >> Initialize cfs_rq->avg.last_update_time to 1 instead.
> >
> > Actually, the first time (attach_task_cfs_rq()) is called even way
> > before init_entity_runnable_average(), no?
> 
> maybe there is an issue during the fork of a task too
> This patch is about the init of the sched_avg of a task group. The
> problem happens when the 1st task is moved the group but it's not
> about the fork of a task

Right, but the root cause is not addressed, which is when forked, the task
should not be touched on sched avgs before init_entity_runnable_average()
in wake_up_new_task(). You think?

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

#1407464 — Re: [PATCH v2] sched: fix first task of a task group is attached twice

On 26 May 2016 at 02:40, Yuyang Du <yuyang.du@intel.com> wrote:
> On Thu, May 26, 2016 at 10:26:54AM +0200, Vincent Guittot wrote:
>> On 26 May 2016 at 00:38, Yuyang Du <yuyang.du@intel.com> wrote:
>> > On Wed, May 25, 2016 at 05:01:11PM +0200, Vincent Guittot wrote:
>> >> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
>> >> that the 1st sched_entity that will be attached, will keep its
>> >> last_update_time set to 0 and will attached once again during the
>> >> enqueue.
>> >> Initialize cfs_rq->avg.last_update_time to 1 instead.
>> >
>> > Actually, the first time (attach_task_cfs_rq()) is called even way
>> > before init_entity_runnable_average(), no?
>>
>> maybe there is an issue during the fork of a task too
>> This patch is about the init of the sched_avg of a task group. The
>> problem happens when the 1st task is moved the group but it's not
>> about the fork of a task
>
> Right, but the root cause is not addressed, which is when forked, the task
> should not be touched on sched avgs before init_entity_runnable_average()
> in wake_up_new_task(). You think?

so yes, this patch doesn't not address the fact that sched avgs should
not be touch before init_entity_runnable_average(). This patch is only
about inti of sched_avg of task group

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

#1408147 — Re: [PATCH v2] sched: fix first task of a task group is attached twice

On 25/05/16 16:01, Vincent Guittot wrote:
> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
> that the 1st sched_entity that will be attached, will keep its
> last_update_time set to 0 and will attached once again during the
> enqueue.
> Initialize cfs_rq->avg.last_update_time to 1 instead.
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> 
> v2:
> - rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held
> 
>  kernel/sched/fair.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 218f8e8..3724656 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8586,6 +8586,14 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
>  		se->depth = parent->depth + 1;
>  	}
>  
> +	/*
> +	 * Set last_update_time to something different from 0 to make
> +	 * sure the 1st sched_entity will not be attached twice: once
> +	 * when attaching the task to the group and one more time when
> +	 * enqueueing the task.
> +	 */
> +	tg->cfs_rq[cpu]->avg.last_update_time = 1;
> +
>  	se->my_q = cfs_rq;
>  	/* guarantee group entities always have weight */
>  	update_load_set(&se->load, NICE_0_LOAD);

So why not setting the last_update_time value for those cfs_rq's when
we have the lock? E.g. in task_move_group_fair() or attach_task_cfs_rq().

@@ -8490,12 +8493,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)
 {
+#ifdef CONFIG_SMP
+       struct cfs_rq *cfs_rq = NULL;
+#endif
+
        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;
+
+       cfs_rq = cfs_rq_of(&p->se);
+       if (!cfs_rq->avg.last_update_time)
+               cfs_rq->avg.last_update_time = rq_clock_task(rq_of(cfs_rq));
 #endif

or

@@ -8423,6 +8423,9 @@ static void attach_task_cfs_rq(struct task_struct *p)
        se->depth = se->parent ? se->parent->depth + 1 : 0;
 #endif
 
+       if (!cfs_rq->avg.last_update_time)
+               cfs_rq->avg.last_update_time = rq_clock_task(rq_of(cfs_rq));
+
        /* Synchronize task with its cfs_rq */
        attach_entity_load_avg(cfs_rq, se);

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

#1408197 — Re: [PATCH v2] sched: fix first task of a task group is attached twice

On 27 May 2016 at 17:48, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
> On 25/05/16 16:01, Vincent Guittot wrote:
>> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
>> that the 1st sched_entity that will be attached, will keep its
>> last_update_time set to 0 and will attached once again during the
>> enqueue.
>> Initialize cfs_rq->avg.last_update_time to 1 instead.
>>
>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>> ---
>>
>> v2:
>> - rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held
>>
>>  kernel/sched/fair.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 218f8e8..3724656 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -8586,6 +8586,14 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
>>               se->depth = parent->depth + 1;
>>       }
>>
>> +     /*
>> +      * Set last_update_time to something different from 0 to make
>> +      * sure the 1st sched_entity will not be attached twice: once
>> +      * when attaching the task to the group and one more time when
>> +      * enqueueing the task.
>> +      */
>> +     tg->cfs_rq[cpu]->avg.last_update_time = 1;
>> +
>>       se->my_q = cfs_rq;
>>       /* guarantee group entities always have weight */
>>       update_load_set(&se->load, NICE_0_LOAD);
>
> So why not setting the last_update_time value for those cfs_rq's when
> we have the lock? E.g. in task_move_group_fair() or attach_task_cfs_rq().

I'm not sure that it's worth adding this init in functions that are
then used often only for the init of it.
If you are concerned by the update of the load of the 1st task that
will be attached, it can still have elapsed  a long time between the
creation of the group and the 1st enqueue of a task. This was the case
for the test i did when i found this issue.

Beside this point, I have to send a new version to set
load_last_update_time_copy for not 64 bits system. Fengguang points me
the issue

Regards,
Vincent

>
> @@ -8490,12 +8493,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)
>  {
> +#ifdef CONFIG_SMP
> +       struct cfs_rq *cfs_rq = NULL;
> +#endif
> +
>         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;
> +
> +       cfs_rq = cfs_rq_of(&p->se);
> +       if (!cfs_rq->avg.last_update_time)
> +               cfs_rq->avg.last_update_time = rq_clock_task(rq_of(cfs_rq));
>  #endif
>
> or
>
> @@ -8423,6 +8423,9 @@ static void attach_task_cfs_rq(struct task_struct *p)
>         se->depth = se->parent ? se->parent->depth + 1 : 0;
>  #endif
>
> +       if (!cfs_rq->avg.last_update_time)
> +               cfs_rq->avg.last_update_time = rq_clock_task(rq_of(cfs_rq));
> +
>         /* Synchronize task with its cfs_rq */
>         attach_entity_load_avg(cfs_rq, se);

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

#1408295 — Re: [PATCH v2] sched: fix first task of a task group is attached twice

On 27/05/16 18:16, Vincent Guittot wrote:
> On 27 May 2016 at 17:48, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>> On 25/05/16 16:01, Vincent Guittot wrote:
>>> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
>>> that the 1st sched_entity that will be attached, will keep its
>>> last_update_time set to 0 and will attached once again during the
>>> enqueue.
>>> Initialize cfs_rq->avg.last_update_time to 1 instead.
>>>
>>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>>> ---
>>>
>>> v2:
>>> - rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held
>>>
>>>  kernel/sched/fair.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index 218f8e8..3724656 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -8586,6 +8586,14 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
>>>               se->depth = parent->depth + 1;
>>>       }
>>>
>>> +     /*
>>> +      * Set last_update_time to something different from 0 to make
>>> +      * sure the 1st sched_entity will not be attached twice: once
>>> +      * when attaching the task to the group and one more time when
>>> +      * enqueueing the task.
>>> +      */
>>> +     tg->cfs_rq[cpu]->avg.last_update_time = 1;
>>> +

Couldn't you not just set the value in init_cfs_rq():

@@ -8482,6 +8482,7 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
        cfs_rq->min_vruntime_copy = cfs_rq->min_vruntime;
 #endif
 #ifdef CONFIG_SMP
+       cfs_rq->avg.last_update_time = 1;
        atomic_long_set(&cfs_rq->removed_load_avg, 0);
        atomic_long_set(&cfs_rq->removed_util_avg, 0);
 #endif

>>>       se->my_q = cfs_rq;
>>>       /* guarantee group entities always have weight */
>>>       update_load_set(&se->load, NICE_0_LOAD);
>>
>> So why not setting the last_update_time value for those cfs_rq's when
>> we have the lock? E.g. in task_move_group_fair() or attach_task_cfs_rq().
> 
> I'm not sure that it's worth adding this init in functions that are
> then used often only for the init of it.

Yeah, there will be this if condition overhead.

> If you are concerned by the update of the load of the 1st task that
> will be attached, it can still have elapsed  a long time between the
> creation of the group and the 1st enqueue of a task. This was the case
> for the test i did when i found this issue.

Understood, but for me, creation of the task group is
cpu_cgroup_css_alloc ->  sched_create_group() -> ... -> init_cfs_rq(),
init_tg_cfs_entry(), ...

and the functions which are called when the first task is put into the
task group are cpu_cgroup_attach() and cpu_cgroup_fork() and they whould
trigger the initial setup of the cfs_rq->avg.last_update_time.

> 
> Beside this point, I have to send a new version to set
> load_last_update_time_copy for not 64 bits system. Fengguang points me
> the issue

OK.

[...]
>>
>> +       if (!cfs_rq->avg.last_update_time)
>> +               cfs_rq->avg.last_update_time = rq_clock_task(rq_of(cfs_rq));
>> +
>>         /* Synchronize task with its cfs_rq */
>>         attach_entity_load_avg(cfs_rq, se);
> 

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

#1408808 — Re: [PATCH v2] sched: fix first task of a task group is attached twice

On 27 May 2016 at 22:38, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
> On 27/05/16 18:16, Vincent Guittot wrote:
>> On 27 May 2016 at 17:48, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>>> On 25/05/16 16:01, Vincent Guittot wrote:
>>>> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
>>>> that the 1st sched_entity that will be attached, will keep its
>>>> last_update_time set to 0 and will attached once again during the
>>>> enqueue.
>>>> Initialize cfs_rq->avg.last_update_time to 1 instead.
>>>>
>>>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>>>> ---
>>>>
>>>> v2:
>>>> - rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held
>>>>
>>>>  kernel/sched/fair.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>>> index 218f8e8..3724656 100644
>>>> --- a/kernel/sched/fair.c
>>>> +++ b/kernel/sched/fair.c
>>>> @@ -8586,6 +8586,14 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
>>>>               se->depth = parent->depth + 1;
>>>>       }
>>>>
>>>> +     /*
>>>> +      * Set last_update_time to something different from 0 to make
>>>> +      * sure the 1st sched_entity will not be attached twice: once
>>>> +      * when attaching the task to the group and one more time when
>>>> +      * enqueueing the task.
>>>> +      */
>>>> +     tg->cfs_rq[cpu]->avg.last_update_time = 1;
>>>> +
>
> Couldn't you not just set the value in init_cfs_rq():

yes, there is no good reason to use  init_tg_cfs_entry instead of  init_cfs_rq

>
> @@ -8482,6 +8482,7 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
>         cfs_rq->min_vruntime_copy = cfs_rq->min_vruntime;
>  #endif
>  #ifdef CONFIG_SMP
> +       cfs_rq->avg.last_update_time = 1;
>         atomic_long_set(&cfs_rq->removed_load_avg, 0);
>         atomic_long_set(&cfs_rq->removed_util_avg, 0);
>  #endif
>
>>>>       se->my_q = cfs_rq;
>>>>       /* guarantee group entities always have weight */
>>>>       update_load_set(&se->load, NICE_0_LOAD);
>>>
>>> So why not setting the last_update_time value for those cfs_rq's when
>>> we have the lock? E.g. in task_move_group_fair() or attach_task_cfs_rq().
>>
>> I'm not sure that it's worth adding this init in functions that are
>> then used often only for the init of it.
>
> Yeah, there will be this if condition overhead.
>
>> If you are concerned by the update of the load of the 1st task that
>> will be attached, it can still have elapsed  a long time between the
>> creation of the group and the 1st enqueue of a task. This was the case
>> for the test i did when i found this issue.
>
> Understood, but for me, creation of the task group is
> cpu_cgroup_css_alloc ->  sched_create_group() -> ... -> init_cfs_rq(),
> init_tg_cfs_entry(), ...
>
> and the functions which are called when the first task is put into the
> task group are cpu_cgroup_attach() and cpu_cgroup_fork() and they whould
> trigger the initial setup of the cfs_rq->avg.last_update_time.
>
>>
>> Beside this point, I have to send a new version to set
>> load_last_update_time_copy for not 64 bits system. Fengguang points me
>> the issue
>
> OK.
>
> [...]
>>>
>>> +       if (!cfs_rq->avg.last_update_time)
>>> +               cfs_rq->avg.last_update_time = rq_clock_task(rq_of(cfs_rq));
>>> +
>>>         /* Synchronize task with its cfs_rq */
>>>         attach_entity_load_avg(cfs_rq, se);
>>

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

#1409250 — [PATCH v3] sched: fix first task of a task group is attached twice

The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
that the 1st sched_entity that will be attached, will keep its
last_update_time set to 0 and will attached once again during the
enqueue.
Initialize cfs_rq->avg.last_update_time to 1 instead.

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

v3:
- add initialization of load_last_update_time_copy for not 64bits system
- move init into init_cfs_rq

v2:
- rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held

 kernel/sched/fair.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 218f8e8..86be9c1 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8459,6 +8459,16 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
 	cfs_rq->min_vruntime_copy = cfs_rq->min_vruntime;
 #endif
 #ifdef CONFIG_SMP
+	/*
+	 * Set last_update_time to something different from 0 to make
+	 * sure the 1st sched_entity will not be attached twice: once
+	 * when attaching the task to the group and one more time when
+	 * enqueueing the task.
+	 */
+	cfs_rq->avg.last_update_time = 1;
+#ifndef CONFIG_64BIT
+	cfs_rq->load_last_update_time_copy = 1;
+#endif
 	atomic_long_set(&cfs_rq->removed_load_avg, 0);
 	atomic_long_set(&cfs_rq->removed_util_avg, 0);
 #endif
-- 
1.9.1

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

#1409794 — Re: [PATCH v3] sched: fix first task of a task group is attached twice

On Mon, May 30, 2016 at 05:52:20PM +0200, Vincent Guittot wrote:
> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
> that the 1st sched_entity that will be attached, will keep its
> last_update_time set to 0 and will attached once again during the
> enqueue.
> Initialize cfs_rq->avg.last_update_time to 1 instead.
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> 
> v3:
> - add initialization of load_last_update_time_copy for not 64bits system
> - move init into init_cfs_rq
> 
> v2:
> - rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held
> 
>  kernel/sched/fair.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 218f8e8..86be9c1 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8459,6 +8459,16 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
>  	cfs_rq->min_vruntime_copy = cfs_rq->min_vruntime;
>  #endif
>  #ifdef CONFIG_SMP
> +	/*
> +	 * Set last_update_time to something different from 0 to make
> +	 * sure the 1st sched_entity will not be attached twice:once
> +	 * when attaching the task to the group and one more time when
> +	 * enqueueing the task.
> +	 */

The first time: "once when attaching the task to the group".

That attaching is purely wrong, but will not have any effect (at least
load/util wise), because the task will later be inited in
init_entity_runnable_average().

The second time: "one more time when enqueueing the task".

In enqueue_entity_load_avg(), your patch will not have any effect either,
because of the above overwriting the new task's load in
init_entity_runnable_average(), no?

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

#1409929 — Re: [PATCH v3] sched: fix first task of a task group is attached twice

Hi Yuyang,

On 30 May 2016 at 21:48, Yuyang Du <yuyang.du@intel.com> wrote:
> On Mon, May 30, 2016 at 05:52:20PM +0200, Vincent Guittot wrote:
>> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
>> that the 1st sched_entity that will be attached, will keep its
>> last_update_time set to 0 and will attached once again during the
>> enqueue.
>> Initialize cfs_rq->avg.last_update_time to 1 instead.
>>
>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>> ---
>>
>> v3:
>> - add initialization of load_last_update_time_copy for not 64bits system
>> - move init into init_cfs_rq
>>
>> v2:
>> - rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held
>>
>>  kernel/sched/fair.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 218f8e8..86be9c1 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -8459,6 +8459,16 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
>>       cfs_rq->min_vruntime_copy = cfs_rq->min_vruntime;
>>  #endif
>>  #ifdef CONFIG_SMP
>> +     /*
>> +      * Set last_update_time to something different from 0 to make
>> +      * sure the 1st sched_entity will not be attached twice:once
>> +      * when attaching the task to the group and one more time when
>> +      * enqueueing the task.
>> +      */
>
> The first time: "once when attaching the task to the group".
>
> That attaching is purely wrong, but will not have any effect (at least
> load/util wise), because the task will later be inited in
> init_entity_runnable_average().

This patch is not related to the init of a task but related to the
init of the cfs_rq and to what happen with the 1st task that is
enqueued on it.

Lets take a task A that has already been scheduled on other cfs_rq so
its se->avg.last_update_time is different from 0.

Create a new task group TGB
At creation, the cfs_rq->avg.last_update_time of this TGB is set to 0.

Now move task A on TGB.
A is attached to TGB so  se->avg.last_update_time =
cfs_rq->avg.last_update_time which is 0
A is then enqueued on th cfs_rq and because se->avg.last_update_time
== 0, A will be attached one more time on the cfs_rq

This patch set cfs_rq->avg.last_update_time to 1 at creation so the
1st time that A is attached to TGB,  se->avg.last_update_time =
cfs_rq->avg.last_update_time = 1 and A will not bve attached one more
time during the enqueue.

>
> The second time: "one more time when enqueueing the task".
>
> In enqueue_entity_load_avg(), your patch will not have any effect either,
> because of the above overwriting the new task's load in
> init_entity_runnable_average(), no?

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

#1409999 — Re: [PATCH v3] sched: fix first task of a task group is attached twice

On Tue, May 31, 2016 at 09:28:30AM +0200, Vincent Guittot wrote:
> Hi Yuyang,
> 
> On 30 May 2016 at 21:48, Yuyang Du <yuyang.du@intel.com> wrote:
> > On Mon, May 30, 2016 at 05:52:20PM +0200, Vincent Guittot wrote:
> >> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
> >> that the 1st sched_entity that will be attached, will keep its
> >> last_update_time set to 0 and will attached once again during the
> >> enqueue.
> >> Initialize cfs_rq->avg.last_update_time to 1 instead.
> >>
> >> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> >> ---
> >>
> >> v3:
> >> - add initialization of load_last_update_time_copy for not 64bits system
> >> - move init into init_cfs_rq
> >>
> >> v2:
> >> - rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held
> >>
> >>  kernel/sched/fair.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 218f8e8..86be9c1 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -8459,6 +8459,16 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
> >>       cfs_rq->min_vruntime_copy = cfs_rq->min_vruntime;
> >>  #endif
> >>  #ifdef CONFIG_SMP
> >> +     /*
> >> +      * Set last_update_time to something different from 0 to make
> >> +      * sure the 1st sched_entity will not be attached twice:once
> >> +      * when attaching the task to the group and one more time when
> >> +      * enqueueing the task.
> >> +      */
> >
> > The first time: "once when attaching the task to the group".
> >
> > That attaching is purely wrong, but will not have any effect (at least
> > load/util wise), because the task will later be inited in
> > init_entity_runnable_average().
> 
> This patch is not related to the init of a task but related to the
> init of the cfs_rq and to what happen with the 1st task that is
> enqueued on it.
> 
> Lets take a task A that has already been scheduled on other cfs_rq so
> its se->avg.last_update_time is different from 0.

I understand it, finally, :)
 
> Create a new task group TGB
> At creation, the cfs_rq->avg.last_update_time of this TGB is set to 0.
> 
> Now move task A on TGB.
> A is attached to TGB so  se->avg.last_update_time =
> cfs_rq->avg.last_update_time which is 0
> A is then enqueued on th cfs_rq and because se->avg.last_update_time
> == 0, A will be attached one more time on the cfs_rq
> 
> This patch set cfs_rq->avg.last_update_time to 1 at creation so the
> 1st time that A is attached to TGB,  se->avg.last_update_time =
> cfs_rq->avg.last_update_time = 1 and A will not bve attached one more
> time during the enqueue.

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

#1409249 — Re: [PATCH v2] sched: fix first task of a task group is attached twice

On 27 May 2016 at 22:38, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
> On 27/05/16 18:16, Vincent Guittot wrote:
>> On 27 May 2016 at 17:48, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>>> On 25/05/16 16:01, Vincent Guittot wrote:
>>>> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
>>>> that the 1st sched_entity that will be attached, will keep its
>>>> last_update_time set to 0 and will attached once again during the
>>>> enqueue.
>>>> Initialize cfs_rq->avg.last_update_time to 1 instead.
>>>>
>>>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>>>> ---
>>>>
>>>> v2:
>>>> - rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held
>>>>
>>>>  kernel/sched/fair.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>>> index 218f8e8..3724656 100644
>>>> --- a/kernel/sched/fair.c
>>>> +++ b/kernel/sched/fair.c
>>>> @@ -8586,6 +8586,14 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
>>>>               se->depth = parent->depth + 1;
>>>>       }
>>>>
>>>> +     /*
>>>> +      * Set last_update_time to something different from 0 to make
>>>> +      * sure the 1st sched_entity will not be attached twice: once
>>>> +      * when attaching the task to the group and one more time when
>>>> +      * enqueueing the task.
>>>> +      */
>>>> +     tg->cfs_rq[cpu]->avg.last_update_time = 1;
>>>> +
>
> Couldn't you not just set the value in init_cfs_rq():
>
> @@ -8482,6 +8482,7 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
>         cfs_rq->min_vruntime_copy = cfs_rq->min_vruntime;
>  #endif
>  #ifdef CONFIG_SMP
> +       cfs_rq->avg.last_update_time = 1;
>         atomic_long_set(&cfs_rq->removed_load_avg, 0);
>         atomic_long_set(&cfs_rq->removed_util_avg, 0);
>  #endif
>
>>>>       se->my_q = cfs_rq;
>>>>       /* guarantee group entities always have weight */
>>>>       update_load_set(&se->load, NICE_0_LOAD);
>>>
>>> So why not setting the last_update_time value for those cfs_rq's when
>>> we have the lock? E.g. in task_move_group_fair() or attach_task_cfs_rq().
>>
>> I'm not sure that it's worth adding this init in functions that are
>> then used often only for the init of it.
>
> Yeah, there will be this if condition overhead.
>
>> If you are concerned by the update of the load of the 1st task that
>> will be attached, it can still have elapsed  a long time between the
>> creation of the group and the 1st enqueue of a task. This was the case
>> for the test i did when i found this issue.
>
> Understood, but for me, creation of the task group is
> cpu_cgroup_css_alloc ->  sched_create_group() -> ... -> init_cfs_rq(),
> init_tg_cfs_entry(), ...
>
> and the functions which are called when the first task is put into the
> task group are cpu_cgroup_attach() and cpu_cgroup_fork() and they whould
> trigger the initial setup of the cfs_rq->avg.last_update_time.

Adding a test and the init of cfs_rq->avg.last_update_time in
cpu_cgroup_attach() and cpu_cgroup_fork() in order to have an almost
up to date cfs_rq->avg.last_update_time at creation, will only solve a
part of a wider issue that happens when moving a task to a cfs_rq that
has not been updated for a while (since the creation for the 1st time
but also since the last update of a blocked cfs_rq)

I have another pending patch for this kind of issue that i haven't sent yet

>
>>
>> Beside this point, I have to send a new version to set
>> load_last_update_time_copy for not 64 bits system. Fengguang points me
>> the issue
>
> OK.
>
> [...]
>>>
>>> +       if (!cfs_rq->avg.last_update_time)
>>> +               cfs_rq->avg.last_update_time = rq_clock_task(rq_of(cfs_rq));
>>> +
>>>         /* Synchronize task with its cfs_rq */
>>>         attach_entity_load_avg(cfs_rq, se);
>>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web