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

Groups > linux.kernel > #1423659 > unrolled thread

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

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH v3] sched: fix first task of a task group is attached  twice Yuyang Du <yuyang.du@intel.com> - 2016-06-16 05:20 +0200
    Re: [PATCH v3] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-16 09:20 +0200
      Re: [PATCH v3] sched: fix first task of a task group is attached  twice Yuyang Du <yuyang.du@intel.com> - 2016-06-16 09:30 +0200
        Re: [PATCH v3] sched: fix first task of a task group is attached twice Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-16 11:50 +0200

#1423659 — 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.
> +	 */
> +	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

Then, when enqueued, both cfs_rq and task will be decayed to 0, due to
a large gap between 1 and now, no?

[toc] | [next] | [standalone]

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

On 15 June 2016 at 21:19, 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.
>> +      */
>> +     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
>
> Then, when enqueued, both cfs_rq and task will be decayed to 0, due to
> a large gap between 1 and now, no?

yes, like it is done currently (but 1ns later) .

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

#1423744

On Thu, Jun 16, 2016 at 09:12:58AM +0200, Vincent Guittot wrote:
> > Then, when enqueued, both cfs_rq and task will be decayed to 0, due to
> > a large gap between 1 and now, no?
> 
> yes, like it is done currently (but 1ns later) .

Well, currently, cfs_rq will be decayed to 0, but will then add the task.
So it turns out the current result is right. Attached twice, but result
is right. Correct?

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

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

On 16 June 2016 at 01:24, Yuyang Du <yuyang.du@intel.com> wrote:
> On Thu, Jun 16, 2016 at 09:12:58AM +0200, Vincent Guittot wrote:
>> > Then, when enqueued, both cfs_rq and task will be decayed to 0, due to
>> > a large gap between 1 and now, no?
>>
>> yes, like it is done currently (but 1ns later) .
>
> Well, currently, cfs_rq will be decayed to 0, but will then add the task.
> So it turns out the current result is right. Attached twice, but result
> is right. Correct?

So the load looks accidentally correct but not the utilization which
will be overestimated up to twice the max

With this change,the behavior of the 1st task becomes the same as what
happen to the other tasks that will be attached to the task group that
has been idle for a while and  that has an old last_update_time.
Then, i have other pending patch to fix this behavior, has mentioned
in a previous email

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web