[PATCH v1] sched/cfs: Fix pick_next_entity() implementation error

[PATCH v1] sched/cfs: Fix pick_next_entity() implementation error

[Zijun Hu]

sched_entity @se not static @left should be compared
to pick up @cfs_rq->next.

Signed-off-by: Zijun Hu <zijuhu@codeaurora.org>
---
 kernel/sched/fair.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 658aa7a2ae6f..4790f2d851ad 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4452,13 +4452,13 @@ pick_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *curr)
 	/*
 	 * Prefer last buddy, try to return the CPU to a preempted task.
 	 */
-	if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1)
+	if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, se) < 1)
 		se = cfs_rq->last;
 
 	/*
 	 * Someone really wants this to run. If it's not unfair, run it.
 	 */
-	if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
+	if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, se) < 1)
 		se = cfs_rq->next;
 
 	clear_buddies(cfs_rq, se);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH v1] sched/cfs: Fix pick_next_entity() implementation error

[Vincent Guittot]

On Wed, 1 Jul 2020 at 12:07, Zijun Hu <zijuhu@codeaurora.org> wrote:
>
> sched_entity @se not static @left should be compared
> to pick up @cfs_rq->next.

Could you elaborate why ?

left is the leftmost sched_entity and the one that should be used.

se != left means that left should be skipped after a yield and the
next se in the rbtree is not "far" from left although it has higher
vruntime

if we finally want to use last or next instead of se, we must ensure
that they are still not "far" from left otherwise you can promote a
sched entity that ends up having a really high vruntime

>
> Signed-off-by: Zijun Hu <zijuhu@codeaurora.org>
> ---
>  kernel/sched/fair.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 658aa7a2ae6f..4790f2d851ad 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4452,13 +4452,13 @@ pick_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *curr)
>         /*
>          * Prefer last buddy, try to return the CPU to a preempted task.
>          */
> -       if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1)
> +       if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, se) < 1)
>                 se = cfs_rq->last;
>
>         /*
>          * Someone really wants this to run. If it's not unfair, run it.
>          */
> -       if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
> +       if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, se) < 1)
>                 se = cfs_rq->next;
>
>         clear_buddies(cfs_rq, se);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
>

Re: [PATCH v1] sched/cfs: Fix pick_next_entity() implementation error

[Zijun Hu]

thanks for your explanation.
you are right. @lest should be used as reference point to compare.
Please ignore this patch.

On 7/1/2020 6:47 PM, Vincent Guittot wrote:
> On Wed, 1 Jul 2020 at 12:07, Zijun Hu <zijuhu@codeaurora.org> wrote:
>>
>> sched_entity @se not static @left should be compared
>> to pick up @cfs_rq->next.
> 
> Could you elaborate why ?
> 
> left is the leftmost sched_entity and the one that should be used.
> 
> se != left means that left should be skipped after a yield and the
> next se in the rbtree is not "far" from left although it has higher
> vruntime
> 
> if we finally want to use last or next instead of se, we must ensure
> that they are still not "far" from left otherwise you can promote a
> sched entity that ends up having a really high vruntime
> 
>>
>> Signed-off-by: Zijun Hu <zijuhu@codeaurora.org>
>> ---
>>  kernel/sched/fair.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 658aa7a2ae6f..4790f2d851ad 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -4452,13 +4452,13 @@ pick_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *curr)
>>         /*
>>          * Prefer last buddy, try to return the CPU to a preempted task.
>>          */
>> -       if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1)
>> +       if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, se) < 1)
>>                 se = cfs_rq->last;
>>
>>         /*
>>          * Someone really wants this to run. If it's not unfair, run it.
>>          */
>> -       if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
>> +       if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, se) < 1)
>>                 se = cfs_rq->next;
>>
>>         clear_buddies(cfs_rq, se);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
>>