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

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

[Wanpeng Li]

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.

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

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

[Peter Zijlstra]

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?

> 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
> 

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

[Wanpeng Li]

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 absolute vruntime will 
be calculated in enqueue_entity() either the task is running or sleeping 
when moved back. The fair sleeper bonus 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.

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
>>

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

[Byungchul Park]

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
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/

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

[Wanpeng Li]

On 9/8/15 1:28 PM, 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

Please refer why the relative vruntime caculation is introduced to 
switched_from_fair(): https://lkml.org/lkml/2011/1/17/129

> 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
> 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.

Absolute vruntime is caculated in place_entity() which is called by 
enqueue_entity() for DEQUEUE_SLEEP task.

Regards,
Wanpeng Li

>
>> 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/

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

[Byungchul Park]

On Tue, Sep 08, 2015 at 01:38:08PM +0800, Wanpeng Li wrote:
> On 9/8/15 1:28 PM, 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
> 
> Please refer why the relative vruntime caculation is introduced to
> switched_from_fair(): https://lkml.org/lkml/2011/1/17/129

hello,

it is just a bug caused by not calculating a relative vruntime when
detached a task from cfs_rq, which is necessary though.

> 
> >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
> >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.
> 
> Absolute vruntime is caculated in place_entity() which is called by
> enqueue_entity() for DEQUEUE_SLEEP task.

as you may know, place_entity() is not for calculating an absolute
vruntime though.. anyway the important thing here is that, when a
sleeping task is moved back to fair class, enqueue_entity() for
DEQUEUE_SLEEP task won't be called.

thanks,
byungchul

> 
> Regards,
> Wanpeng Li
> 
> >
> >>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/

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

[Wanpeng Li]

On 9/8/15 2:14 PM, Byungchul Park wrote:
> On Tue, Sep 08, 2015 at 01:38:08PM +0800, Wanpeng Li wrote:
>> On 9/8/15 1:28 PM, 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
>> Please refer why the relative vruntime caculation is introduced to
>> switched_from_fair(): https://lkml.org/lkml/2011/1/17/129
> hello,
>
> it is just a bug caused by not calculating a relative vruntime when
> detached a task from cfs_rq, which is necessary though.

Refer to Peterz's comments:

| There is also a case where it was moved from fair_sched_class when it
| was in a blocked state and moved back while it is running.

>
>>> 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
>>> 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.
>> Absolute vruntime is caculated in place_entity() which is called by
>> enqueue_entity() for DEQUEUE_SLEEP task.
> as you may know, place_entity() is not for calculating an absolute
> vruntime though.. anyway the important thing here is that, when a
> sleeping task is moved back to fair class, enqueue_entity() for
> DEQUEUE_SLEEP task won't be called.

The enqueue_entity() for DEQUEUE_SLEEP task will be called when the task 
is wake up.

Regards,
Wanpeng Li

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

[Byungchul Park]

On Tue, Sep 08, 2015 at 02:23:56PM +0800, Wanpeng Li wrote:
> On 9/8/15 2:14 PM, Byungchul Park wrote:
> >On Tue, Sep 08, 2015 at 01:38:08PM +0800, Wanpeng Li wrote:
> >>On 9/8/15 1:28 PM, 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
> >>Please refer why the relative vruntime caculation is introduced to
> >>switched_from_fair(): https://lkml.org/lkml/2011/1/17/129
> >hello,
> >
> >it is just a bug caused by not calculating a relative vruntime when
> >detached a task from cfs_rq, which is necessary though.
> 
> Refer to Peterz's comments:
> 
> | There is also a case where it was moved from fair_sched_class when it
> | was in a blocked state and moved back while it is running.
> 
> >
> >>>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
> >>>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.
> >>Absolute vruntime is caculated in place_entity() which is called by
> >>enqueue_entity() for DEQUEUE_SLEEP task.
> >as you may know, place_entity() is not for calculating an absolute
> >vruntime though.. anyway the important thing here is that, when a
> >sleeping task is moved back to fair class, enqueue_entity() for
> >DEQUEUE_SLEEP task won't be called.
> 
> The enqueue_entity() for DEQUEUE_SLEEP task will be called when the
> task is wake up.

now, i see what it means "enqueue_entity() for DEQUEUE_SLEEP task".

> 
> 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/

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

[Byungchul Park]

On Tue, Sep 08, 2015 at 03:14:26PM +0900, Byungchul Park wrote:
> On Tue, Sep 08, 2015 at 01:38:08PM +0800, Wanpeng Li wrote:
> > On 9/8/15 1:28 PM, 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
> > 
> > Please refer why the relative vruntime caculation is introduced to
> > switched_from_fair(): https://lkml.org/lkml/2011/1/17/129
> 
> hello,
> 
> it is just a bug caused by not calculating a relative vruntime when
> detached a task from cfs_rq, which is necessary though.
> 
> > 
> > >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
> > >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.
> > 
> > Absolute vruntime is caculated in place_entity() which is called by
> > enqueue_entity() for DEQUEUE_SLEEP task.
> 
> as you may know, place_entity() is not for calculating an absolute
> vruntime though.. anyway the important thing here is that, when a
> sleeping task is moved back to fair class, enqueue_entity() for
> DEQUEUE_SLEEP task won't be called.

you may talk about calling enqueue_entity() when the task is woken up,
not just when it is moved back. right? 

even if yes, i think place_entity() should not be used directly for
calculating an absolute vruntime. it should be called after non/normalizing
operations.

> 
> thanks,
> byungchul
> 
> > 
> > Regards,
> > Wanpeng Li
> > 
> > >
> > >>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/

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

[Wanpeng Li]

On 9/8/15 2:32 PM, Byungchul Park wrote:
> On Tue, Sep 08, 2015 at 03:14:26PM +0900, Byungchul Park wrote:
>> On Tue, Sep 08, 2015 at 01:38:08PM +0800, Wanpeng Li wrote:
>>> On 9/8/15 1:28 PM, 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
>>> Please refer why the relative vruntime caculation is introduced to
>>> switched_from_fair(): https://lkml.org/lkml/2011/1/17/129
>> hello,
>>
>> it is just a bug caused by not calculating a relative vruntime when
>> detached a task from cfs_rq, which is necessary though.
>>
>>>> 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
>>>> 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.
>>> Absolute vruntime is caculated in place_entity() which is called by
>>> enqueue_entity() for DEQUEUE_SLEEP task.
>> as you may know, place_entity() is not for calculating an absolute
>> vruntime though.. anyway the important thing here is that, when a
>> sleeping task is moved back to fair class, enqueue_entity() for
>> DEQUEUE_SLEEP task won't be called.
> you may talk about calling enqueue_entity() when the task is woken up,
> not just when it is moved back. right?

Exactly.

>
> even if yes, i think place_entity() should not be used directly for
> calculating an absolute vruntime. it should be called after non/normalizing
> operations.

The se->vruntime += cfs_rq->min_vruntime(in your switched_to_fair()) 
which means that se->vruntime is bigger than cfs_rq->min_vruntime, 
however, fair sleeper bonus is min_vuntime - sysctl_sched_latency/2, 
which means that max_vruntime() will select the absolute vruntime which 
is caculated in your switched_to_fair() as the se->vruntime, then the 
fair sleeper bonus is lost in this case.

Regards,
Wanpeng Li

>
>> thanks,
>> byungchul
>>
>>> Regards,
>>> Wanpeng Li
>>>
>>>>> 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/

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

[Byungchul Park]

On Tue, Sep 08, 2015 at 02:42:52PM +0800, Wanpeng Li wrote:
> On 9/8/15 2:32 PM, Byungchul Park wrote:
> >On Tue, Sep 08, 2015 at 03:14:26PM +0900, Byungchul Park wrote:
> >>On Tue, Sep 08, 2015 at 01:38:08PM +0800, Wanpeng Li wrote:
> >>>On 9/8/15 1:28 PM, 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
> >>>Please refer why the relative vruntime caculation is introduced to
> >>>switched_from_fair(): https://lkml.org/lkml/2011/1/17/129
> >>hello,
> >>
> >>it is just a bug caused by not calculating a relative vruntime when
> >>detached a task from cfs_rq, which is necessary though.
> >>
> >>>>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
> >>>>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.
> >>>Absolute vruntime is caculated in place_entity() which is called by
> >>>enqueue_entity() for DEQUEUE_SLEEP task.
> >>as you may know, place_entity() is not for calculating an absolute
> >>vruntime though.. anyway the important thing here is that, when a
> >>sleeping task is moved back to fair class, enqueue_entity() for
> >>DEQUEUE_SLEEP task won't be called.
> >you may talk about calling enqueue_entity() when the task is woken up,
> >not just when it is moved back. right?
> 
> Exactly.
> 
> >
> >even if yes, i think place_entity() should not be used directly for
> >calculating an absolute vruntime. it should be called after non/normalizing
> >operations.
> 
> The se->vruntime += cfs_rq->min_vruntime(in your switched_to_fair())
> which means that se->vruntime is bigger than cfs_rq->min_vruntime,

it is not always true since se->vruntime can have a negative value (even
though it is a unsigned type.. i think it can be another problem) by 
se->vruntime -= cfs_rq->min_vruntime in detach_task_cfs_rq().

> however, fair sleeper bonus is min_vuntime - sysctl_sched_latency/2,
> which means that max_vruntime() will select the absolute vruntime
> which is caculated in your switched_to_fair() as the se->vruntime,

since se->vruntime can have a negative value, max_vruntime() may select
the fair sleeper bonused value.

by the way, this logic is unchanged by my patch. which part of my patch
changed this kind of logic?

thanks,
byungchul

> then the fair sleeper bonus is lost in this case.
> 
> Regards,
> Wanpeng Li
> 
> >
> >>thanks,
> >>byungchul
> >>
> >>>Regards,
> >>>Wanpeng Li
> >>>
> >>>>>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/
> 
> --
> 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/

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

[Wanpeng Li]

On 9/8/15 3:11 PM, Byungchul Park wrote:
> On Tue, Sep 08, 2015 at 02:42:52PM +0800, Wanpeng Li wrote:
>> On 9/8/15 2:32 PM, Byungchul Park wrote:
>>> On Tue, Sep 08, 2015 at 03:14:26PM +0900, Byungchul Park wrote:
>>>> On Tue, Sep 08, 2015 at 01:38:08PM +0800, Wanpeng Li wrote:
>>>>> On 9/8/15 1:28 PM, 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
>>>>> Please refer why the relative vruntime caculation is introduced to
>>>>> switched_from_fair(): https://lkml.org/lkml/2011/1/17/129
>>>> hello,
>>>>
>>>> it is just a bug caused by not calculating a relative vruntime when
>>>> detached a task from cfs_rq, which is necessary though.
>>>>
>>>>>> 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
>>>>>> 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.
>>>>> Absolute vruntime is caculated in place_entity() which is called by
>>>>> enqueue_entity() for DEQUEUE_SLEEP task.
>>>> as you may know, place_entity() is not for calculating an absolute
>>>> vruntime though.. anyway the important thing here is that, when a
>>>> sleeping task is moved back to fair class, enqueue_entity() for
>>>> DEQUEUE_SLEEP task won't be called.
>>> you may talk about calling enqueue_entity() when the task is woken up,
>>> not just when it is moved back. right?
>> Exactly.
>>
>>> even if yes, i think place_entity() should not be used directly for
>>> calculating an absolute vruntime. it should be called after non/normalizing
>>> operations.
>> The se->vruntime += cfs_rq->min_vruntime(in your switched_to_fair())
>> which means that se->vruntime is bigger than cfs_rq->min_vruntime,
> it is not always true since se->vruntime can have a negative value (even
> though it is a unsigned type.. i think it can be another problem) by
> se->vruntime -= cfs_rq->min_vruntime in detach_task_cfs_rq().

Yeah, it can be negative.

>
>> however, fair sleeper bonus is min_vuntime - sysctl_sched_latency/2,
>> which means that max_vruntime() will select the absolute vruntime
>> which is caculated in your switched_to_fair() as the se->vruntime,
> since se->vruntime can have a negative value, max_vruntime() may select
> the fair sleeper bonused value.
>
> by the way, this logic is unchanged by my patch. which part of my patch
> changed this kind of logic?

However, if se->vruntime -= cfs_rq->min_vruntime is positive, the 
behavior is different after your patch. e.g. se->vruntime(the relative 
vruntime in switched_to_fair()) < min_vruntime - sysctl_sched_latency/2

before your patch:

se->vruntime = min_vruntime - sysctl_sched_latency/2 (place_entity())

after your patch:

se->vruntime += cfs->min_vruntime  (switched_to_fair())
se->vruntime = se->vruntime  (place_entity())


Regards,
Wanpeng Li

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

[Byungchul Park]

On Tue, Sep 08, 2015 at 03:30:08PM +0800, Wanpeng Li wrote:
> On 9/8/15 3:11 PM, Byungchul Park wrote:
> >On Tue, Sep 08, 2015 at 02:42:52PM +0800, Wanpeng Li wrote:
> >>On 9/8/15 2:32 PM, Byungchul Park wrote:
> >>>On Tue, Sep 08, 2015 at 03:14:26PM +0900, Byungchul Park wrote:
> >>>>On Tue, Sep 08, 2015 at 01:38:08PM +0800, Wanpeng Li wrote:
> >>>>>On 9/8/15 1:28 PM, 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
> >>>>>Please refer why the relative vruntime caculation is introduced to
> >>>>>switched_from_fair(): https://lkml.org/lkml/2011/1/17/129
> >>>>hello,
> >>>>
> >>>>it is just a bug caused by not calculating a relative vruntime when
> >>>>detached a task from cfs_rq, which is necessary though.
> >>>>
> >>>>>>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
> >>>>>>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.
> >>>>>Absolute vruntime is caculated in place_entity() which is called by
> >>>>>enqueue_entity() for DEQUEUE_SLEEP task.
> >>>>as you may know, place_entity() is not for calculating an absolute
> >>>>vruntime though.. anyway the important thing here is that, when a
> >>>>sleeping task is moved back to fair class, enqueue_entity() for
> >>>>DEQUEUE_SLEEP task won't be called.
> >>>you may talk about calling enqueue_entity() when the task is woken up,
> >>>not just when it is moved back. right?
> >>Exactly.
> >>
> >>>even if yes, i think place_entity() should not be used directly for
> >>>calculating an absolute vruntime. it should be called after non/normalizing
> >>>operations.
> >>The se->vruntime += cfs_rq->min_vruntime(in your switched_to_fair())
> >>which means that se->vruntime is bigger than cfs_rq->min_vruntime,
> >it is not always true since se->vruntime can have a negative value (even
> >though it is a unsigned type.. i think it can be another problem) by
> >se->vruntime -= cfs_rq->min_vruntime in detach_task_cfs_rq().
> 
> Yeah, it can be negative.
> 
> >
> >>however, fair sleeper bonus is min_vuntime - sysctl_sched_latency/2,
> >>which means that max_vruntime() will select the absolute vruntime
> >>which is caculated in your switched_to_fair() as the se->vruntime,
> >since se->vruntime can have a negative value, max_vruntime() may select
> >the fair sleeper bonused value.
> >
> >by the way, this logic is unchanged by my patch. which part of my patch
> >changed this kind of logic?
> 
> However, if se->vruntime -= cfs_rq->min_vruntime is positive, the
> behavior is different after your patch. e.g. se->vruntime(the
> relative vruntime in switched_to_fair()) < min_vruntime -
> sysctl_sched_latency/2
> 
> before your patch:
> 
> se->vruntime = min_vruntime - sysctl_sched_latency/2 (place_entity())

my patch is based on ff277d4 commit at tip/sched/core.

there's no change between before and after.

check it please.

and this logic seems to be no problem to me. :(

> 
> after your patch:
> 
> se->vruntime += cfs->min_vruntime  (switched_to_fair())
> se->vruntime = se->vruntime  (place_entity())
> 
> 
> 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/

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

[Wanpeng Li]

On 9/8/15 3:57 PM, Byungchul Park wrote:
> On Tue, Sep 08, 2015 at 03:30:08PM +0800, Wanpeng Li wrote:
>> On 9/8/15 3:11 PM, Byungchul Park wrote:
>>> On Tue, Sep 08, 2015 at 02:42:52PM +0800, Wanpeng Li wrote:
>>>> On 9/8/15 2:32 PM, Byungchul Park wrote:
>>>>> On Tue, Sep 08, 2015 at 03:14:26PM +0900, Byungchul Park wrote:
>>>>>> On Tue, Sep 08, 2015 at 01:38:08PM +0800, Wanpeng Li wrote:
>>>>>>> On 9/8/15 1:28 PM, 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
>>>>>>> Please refer why the relative vruntime caculation is introduced to
>>>>>>> switched_from_fair(): https://lkml.org/lkml/2011/1/17/129
>>>>>> hello,
>>>>>>
>>>>>> it is just a bug caused by not calculating a relative vruntime when
>>>>>> detached a task from cfs_rq, which is necessary though.
>>>>>>
>>>>>>>> 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
>>>>>>>> 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.
>>>>>>> Absolute vruntime is caculated in place_entity() which is called by
>>>>>>> enqueue_entity() for DEQUEUE_SLEEP task.
>>>>>> as you may know, place_entity() is not for calculating an absolute
>>>>>> vruntime though.. anyway the important thing here is that, when a
>>>>>> sleeping task is moved back to fair class, enqueue_entity() for
>>>>>> DEQUEUE_SLEEP task won't be called.
>>>>> you may talk about calling enqueue_entity() when the task is woken up,
>>>>> not just when it is moved back. right?
>>>> Exactly.
>>>>
>>>>> even if yes, i think place_entity() should not be used directly for
>>>>> calculating an absolute vruntime. it should be called after non/normalizing
>>>>> operations.
>>>> The se->vruntime += cfs_rq->min_vruntime(in your switched_to_fair())
>>>> which means that se->vruntime is bigger than cfs_rq->min_vruntime,
>>> it is not always true since se->vruntime can have a negative value (even
>>> though it is a unsigned type.. i think it can be another problem) by
>>> se->vruntime -= cfs_rq->min_vruntime in detach_task_cfs_rq().
>> Yeah, it can be negative.
>>
>>>> however, fair sleeper bonus is min_vuntime - sysctl_sched_latency/2,
>>>> which means that max_vruntime() will select the absolute vruntime
>>>> which is caculated in your switched_to_fair() as the se->vruntime,
>>> since se->vruntime can have a negative value, max_vruntime() may select
>>> the fair sleeper bonused value.
>>>
>>> by the way, this logic is unchanged by my patch. which part of my patch
>>> changed this kind of logic?
>> However, if se->vruntime -= cfs_rq->min_vruntime is positive, the
>> behavior is different after your patch. e.g. se->vruntime(the
>> relative vruntime in switched_to_fair()) < min_vruntime -
>> sysctl_sched_latency/2
>>
>> before your patch:
>>
>> se->vruntime = min_vruntime - sysctl_sched_latency/2 (place_entity())
> my patch is based on ff277d4 commit at tip/sched/core.
>
> there's no change between before and after.
>
> check it please.
>
> and this logic seems to be no problem to me. :(

Your logic will lose fair sleeper bonus in the scenario which I pointed out.

>
>> after your patch:
>>
>> se->vruntime += cfs->min_vruntime  (switched_to_fair())
>> se->vruntime = se->vruntime  (place_entity())
>>
>>
>> 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/

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

[Byungchul Park]

On Tue, Sep 08, 2015 at 04:04:49PM +0800, Wanpeng Li wrote:
> >>However, if se->vruntime -= cfs_rq->min_vruntime is positive, the
> >>behavior is different after your patch. e.g. se->vruntime(the
> >>relative vruntime in switched_to_fair()) < min_vruntime -
> >>sysctl_sched_latency/2
> >>
> >>before your patch:
> >>
> >>se->vruntime = min_vruntime - sysctl_sched_latency/2 (place_entity())
> >my patch is based on ff277d4 commit at tip/sched/core.
> >
> >there's no change between before and after.
> >
> >check it please.
> >
> >and this logic seems to be no problem to me. :(
> 
> Your logic will lose fair sleeper bonus in the scenario which I pointed out.

i mean in ff277d4 commit:

se->vruntime += cfs->min_vruntime  (switched_to_fair())
se->vruntime = se->vruntime or bonused value (place_entity())

after my patch:

se->vruntime += cfs->min_vruntime  (switched_to_fair())
se->vruntime = se->vruntime or bonused value (place_entity())

---

SAME!!!

in addtion, se->vruntime already had a bonused value if eligible,
when it was detached from cfs_rq.

> 
> >
> >>after your patch:
> >>
> >>se->vruntime += cfs->min_vruntime  (switched_to_fair())
> >>se->vruntime = se->vruntime  (place_entity())
> >>
> >>
> >>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/
> 
> --
> 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/

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

[Wanpeng Li]

On 9/8/15 4:22 PM, Byungchul Park wrote:
> On Tue, Sep 08, 2015 at 04:04:49PM +0800, Wanpeng Li wrote:
>>>> However, if se->vruntime -= cfs_rq->min_vruntime is positive, the
>>>> behavior is different after your patch. e.g. se->vruntime(the
>>>> relative vruntime in switched_to_fair()) < min_vruntime -
>>>> sysctl_sched_latency/2
>>>>
>>>> before your patch:
>>>>
>>>> se->vruntime = min_vruntime - sysctl_sched_latency/2 (place_entity())
>>> my patch is based on ff277d4 commit at tip/sched/core.
>>>
>>> there's no change between before and after.
>>>
>>> check it please.
>>>
>>> and this logic seems to be no problem to me. :(
>> Your logic will lose fair sleeper bonus in the scenario which I pointed out.
> i mean in ff277d4 commit:

Please include the commit subject when you point out a commit, do you 
mean this one?

commit ff277d4250fe715b6666219b1a3423b863418794
Author: Andrea Parri <parri.andrea@gmail.com>
Date:   Wed Aug 5 15:56:19 2015 +0200

     sched/deadline: Fix comment in enqueue_task_dl()

     The "dl_boosted" flag is set by comparing *absolute* deadlines
     (c.f., rt_mutex_setprio()).


What's the relationship w/ this patch?

Regards,
Wanpeng Li

>
> se->vruntime += cfs->min_vruntime  (switched_to_fair())
> se->vruntime = se->vruntime or bonused value (place_entity())
>
> after my patch:
>
> se->vruntime += cfs->min_vruntime  (switched_to_fair())
> se->vruntime = se->vruntime or bonused value (place_entity())
>
> ---
>
> SAME!!!
>
> in addtion, se->vruntime already had a bonused value if eligible,
> when it was detached from cfs_rq.
>
>>>> after your patch:
>>>>
>>>> se->vruntime += cfs->min_vruntime  (switched_to_fair())
>>>> se->vruntime = se->vruntime  (place_entity())
>>>>
>>>>
>>>> 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/
>> --
>> 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/

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

[Wanpeng Li]

On 9/8/15 4:38 PM, Wanpeng Li wrote:
> On 9/8/15 4:22 PM, Byungchul Park wrote:
>> On Tue, Sep 08, 2015 at 04:04:49PM +0800, Wanpeng Li wrote:
>>>>> However, if se->vruntime -= cfs_rq->min_vruntime is positive, the
>>>>> behavior is different after your patch. e.g. se->vruntime(the
>>>>> relative vruntime in switched_to_fair()) < min_vruntime -
>>>>> sysctl_sched_latency/2
>>>>>
>>>>> before your patch:
>>>>>
>>>>> se->vruntime = min_vruntime - sysctl_sched_latency/2 (place_entity())
>>>> my patch is based on ff277d4 commit at tip/sched/core.
>>>>
>>>> there's no change between before and after.
>>>>
>>>> check it please.
>>>>
>>>> and this logic seems to be no problem to me. :(
>>> Your logic will lose fair sleeper bonus in the scenario which I 
>>> pointed out.
>> i mean in ff277d4 commit:
>
> Please include the commit subject when you point out a commit, do you 
> mean this one?
>
> commit ff277d4250fe715b6666219b1a3423b863418794
> Author: Andrea Parri <parri.andrea@gmail.com>
> Date:   Wed Aug 5 15:56:19 2015 +0200
>
>     sched/deadline: Fix comment in enqueue_task_dl()
>
>     The "dl_boosted" flag is set by comparing *absolute* deadlines
>     (c.f., rt_mutex_setprio()).
>
>
> What's the relationship w/ this patch?

I think you mean your commit:

commit 7855a35ac07a350e2cd26f09568a6d8e372be358
Author: Byungchul Park <byungchul.park@lge.com>
Date:   Mon Aug 10 18:02:55 2015 +0900

     sched: Ensure a task has a non-normalized vruntime when returning 
back to CFS


However, that is wrong in the scenario which I mentioned.

Regards,
Wanpeng Li

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

[byungchul.park]

> -----Original Message-----
> From: Wanpeng Li [mailto:wanpeng.li@hotmail.com]
> Sent: Tuesday, September 08, 2015 5:46 PM
> To: Byungchul Park
> Cc: Peter Zijlstra; Ingo Molnar; linux-kernel@vger.kernel.org;
> yuyang.du@intel.com
> Subject: Re: [PATCH] sched: fix lose fair sleeper bonus in
switch_to_fair()
> 
> On 9/8/15 4:38 PM, Wanpeng Li wrote:
> > On 9/8/15 4:22 PM, Byungchul Park wrote:
> >> On Tue, Sep 08, 2015 at 04:04:49PM +0800, Wanpeng Li wrote:
> >>>>> However, if se->vruntime -= cfs_rq->min_vruntime is positive, the
> >>>>> behavior is different after your patch. e.g. se->vruntime(the
> >>>>> relative vruntime in switched_to_fair()) < min_vruntime -
> >>>>> sysctl_sched_latency/2
> >>>>>
> >>>>> before your patch:
> >>>>>
> >>>>> se->vruntime = min_vruntime - sysctl_sched_latency/2
(place_entity())
> >>>> my patch is based on ff277d4 commit at tip/sched/core.
> >>>>
> >>>> there's no change between before and after.
> >>>>
> >>>> check it please.
> >>>>
> >>>> and this logic seems to be no problem to me. :(
> >>> Your logic will lose fair sleeper bonus in the scenario which I
> >>> pointed out.
> >> i mean in ff277d4 commit:
> >
> > Please include the commit subject when you point out a commit, do you
> > mean this one?
> >
> > commit ff277d4250fe715b6666219b1a3423b863418794
> > Author: Andrea Parri <parri.andrea@gmail.com>
> > Date:   Wed Aug 5 15:56:19 2015 +0200
> >
> >     sched/deadline: Fix comment in enqueue_task_dl()
> >
> >     The "dl_boosted" flag is set by comparing *absolute* deadlines
> >     (c.f., rt_mutex_setprio()).
> >
> >
> > What's the relationship w/ this patch?
> 
> I think you mean your commit:
> 
> commit 7855a35ac07a350e2cd26f09568a6d8e372be358
> Author: Byungchul Park <byungchul.park@lge.com>
> Date:   Mon Aug 10 18:02:55 2015 +0900
> 
>      sched: Ensure a task has a non-normalized vruntime when returning
> back to CFS
> 
> 
> However, that is wrong in the scenario which I mentioned.

Ah.. Ok. What you are pointing out against, is this commit.
I will focus on verifying if this commit is wrong or not from now.
Let me think about this commit more.

Thanks,
byungchul

> 
> Regards,
> Wanpeng Li

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

[Byungchul Park]

On Tue, Sep 08, 2015 at 04:45:46PM +0800, Wanpeng Li wrote:
> On 9/8/15 4:38 PM, Wanpeng Li wrote:
> >On 9/8/15 4:22 PM, Byungchul Park wrote:
> >>On Tue, Sep 08, 2015 at 04:04:49PM +0800, Wanpeng Li wrote:
> >>>>>However, if se->vruntime -= cfs_rq->min_vruntime is positive, the
> >>>>>behavior is different after your patch. e.g. se->vruntime(the
> >>>>>relative vruntime in switched_to_fair()) < min_vruntime -
> >>>>>sysctl_sched_latency/2
> >>>>>
> >>>>>before your patch:
> >>>>>
> >>>>>se->vruntime = min_vruntime - sysctl_sched_latency/2 (place_entity())
> >>>>my patch is based on ff277d4 commit at tip/sched/core.
> >>>>
> >>>>there's no change between before and after.
> >>>>
> >>>>check it please.
> >>>>
> >>>>and this logic seems to be no problem to me. :(
> >>>Your logic will lose fair sleeper bonus in the scenario which
> >>>I pointed out.
> >>i mean in ff277d4 commit:
> >
> >Please include the commit subject when you point out a commit, do
> >you mean this one?
> >
> >commit ff277d4250fe715b6666219b1a3423b863418794
> >Author: Andrea Parri <parri.andrea@gmail.com>
> >Date:   Wed Aug 5 15:56:19 2015 +0200
> >
> >    sched/deadline: Fix comment in enqueue_task_dl()
> >
> >    The "dl_boosted" flag is set by comparing *absolute* deadlines
> >    (c.f., rt_mutex_setprio()).
> >
> >
> >What's the relationship w/ this patch?
> 
> I think you mean your commit:
> 
> commit 7855a35ac07a350e2cd26f09568a6d8e372be358
> Author: Byungchul Park <byungchul.park@lge.com>
> Date:   Mon Aug 10 18:02:55 2015 +0900
> 
>     sched: Ensure a task has a non-normalized vruntime when
> returning back to CFS
> 
> 
> However, that is wrong in the scenario which I mentioned.

then.. can i think you are not talking about a commit (23ec30ddd7c1306:
'sched: add two functions for att(det)aching a task to(from) a cfs_rq'),
but talking about a commit (7855a35ac07a350: 'sched: Ensure a task has a
non-normalized vruntime when returning back to CFS')?

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().

> 
> 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/

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

[Peter Zijlstra]

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.

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

[byungchul.park]

> -----Original Message-----
> From: Wanpeng Li [mailto:wanpeng.li@hotmail.com]
> Sent: Tuesday, September 08, 2015 5:39 PM
> To: Byungchul Park
> Cc: Peter Zijlstra; Ingo Molnar; linux-kernel@vger.kernel.org;
> yuyang.du@intel.com
> Subject: Re: [PATCH] sched: fix lose fair sleeper bonus in
switch_to_fair()
> 
> On 9/8/15 4:22 PM, Byungchul Park wrote:
> > On Tue, Sep 08, 2015 at 04:04:49PM +0800, Wanpeng Li wrote:
> >>>> However, if se->vruntime -= cfs_rq->min_vruntime is positive, the
> >>>> behavior is different after your patch. e.g. se->vruntime(the
> >>>> relative vruntime in switched_to_fair()) < min_vruntime -
> >>>> sysctl_sched_latency/2
> >>>>
> >>>> before your patch:
> >>>>
> >>>> se->vruntime = min_vruntime - sysctl_sched_latency/2 (place_entity())
> >>> my patch is based on ff277d4 commit at tip/sched/core.

Look at this.

> >>>
> >>> there's no change between before and after.
> >>>
> >>> check it please.
> >>>
> >>> and this logic seems to be no problem to me. :(
> >> Your logic will lose fair sleeper bonus in the scenario which I pointed
> out.
> > i mean in ff277d4 commit:
> 
> Please include the commit subject when you point out a commit, do you
> mean this one?

I said "I based my patch on ff277d4 commit".

> 
> commit ff277d4250fe715b6666219b1a3423b863418794
> Author: Andrea Parri <parri.andrea@gmail.com>
> Date:   Wed Aug 5 15:56:19 2015 +0200
> 
>      sched/deadline: Fix comment in enqueue_task_dl()
> 
>      The "dl_boosted" flag is set by comparing *absolute* deadlines
>      (c.f., rt_mutex_setprio()).
> 
> 
> What's the relationship w/ this patch?

Nothing. Just a base commit.

> 
> Regards,
> Wanpeng Li
> 
> >
> > se->vruntime += cfs->min_vruntime  (switched_to_fair())
> > se->vruntime = se->vruntime or bonused value (place_entity())
> >
> > after my patch:
> >
> > se->vruntime += cfs->min_vruntime  (switched_to_fair())
> > se->vruntime = se->vruntime or bonused value (place_entity())
> >
> > ---
> >
> > SAME!!!
> >
> > in addtion, se->vruntime already had a bonused value if eligible,
> > when it was detached from cfs_rq.
> >
> >>>> after your patch:
> >>>>
> >>>> se->vruntime += cfs->min_vruntime  (switched_to_fair())
> >>>> se->vruntime = se->vruntime  (place_entity())
> >>>>
> >>>>
> >>>> 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/
> >> --
> >> 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/

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

[Byungchul Park]

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/

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

[Peter Zijlstra]

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.

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

[Byungchul Park]

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/

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

[Wanpeng Li]

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