[PATCH] sched/fair: remove next_buddy_marked

[PATCH] sched/fair: remove next_buddy_marked

[Wang Jinchao]

Remove unused `next_buddy_marked` in `check_preempt_wakeup_fair`

Signed-off-by: Wang Jinchao <wangjinchao@xfusion.com>
---
 kernel/sched/fair.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d7a3c63a2171..d2028bfa4e94 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8210,7 +8210,6 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
 	struct task_struct *curr = rq->curr;
 	struct sched_entity *se = &curr->se, *pse = &p->se;
 	struct cfs_rq *cfs_rq = task_cfs_rq(curr);
-	int next_buddy_marked = 0;
 	int cse_is_idle, pse_is_idle;
 
 	if (unlikely(se == pse))
@@ -8227,7 +8226,6 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
 
 	if (sched_feat(NEXT_BUDDY) && !(wake_flags & WF_FORK)) {
 		set_next_buddy(pse);
-		next_buddy_marked = 1;
 	}
 
 	/*
-- 
2.40.0

Re: [PATCH] sched/fair: remove next_buddy_marked

[Vincent Guittot]

On Thu, 14 Dec 2023 at 06:20, Wang Jinchao <wangjinchao@xfusion.com> wrote:
>
> Remove unused `next_buddy_marked` in `check_preempt_wakeup_fair`
>

Fixes: 5e963f2bd465 ("sched/fair: Commit to EEVDF")

> Signed-off-by: Wang Jinchao <wangjinchao@xfusion.com>

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

> ---
>  kernel/sched/fair.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d7a3c63a2171..d2028bfa4e94 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8210,7 +8210,6 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>         struct task_struct *curr = rq->curr;
>         struct sched_entity *se = &curr->se, *pse = &p->se;
>         struct cfs_rq *cfs_rq = task_cfs_rq(curr);
> -       int next_buddy_marked = 0;
>         int cse_is_idle, pse_is_idle;
>
>         if (unlikely(se == pse))
> @@ -8227,7 +8226,6 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>
>         if (sched_feat(NEXT_BUDDY) && !(wake_flags & WF_FORK)) {
>                 set_next_buddy(pse);
> -               next_buddy_marked = 1;
>         }
>
>         /*
> --
> 2.40.0
>

Re: Re: [PATCH] sched/fair: remove next_buddy_marked

[Abel Wu]

On 12/14/23 4:18 PM, Vincent Guittot Wrote:
> On Thu, 14 Dec 2023 at 06:20, Wang Jinchao <wangjinchao@xfusion.com> wrote:
>>
>> Remove unused `next_buddy_marked` in `check_preempt_wakeup_fair`
>>
> 
> Fixes: 5e963f2bd465 ("sched/fair: Commit to EEVDF")

After this commit @pse preempts curr without being the NEXT_BUDDY, but
IMHO it should be, so how about this?

@@ -8259,8 +8259,11 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
         /*
          * XXX pick_eevdf(cfs_rq) != se ?
          */
-       if (pick_eevdf(cfs_rq) == pse)
+       if (pick_eevdf(cfs_rq) == pse) {
+               if (!next_buddy_marked)
+                       set_next_buddy(pse);
                 goto preempt;
+       }

         return;

which will align with before.

Re: Re: [PATCH] sched/fair: remove next_buddy_marked

[Wang Jinchao]

On Thu, Dec 14, 2023 at 08:21:53PM +0800, Abel Wu wrote:
> On 12/14/23 4:18 PM, Vincent Guittot Wrote:
> > On Thu, 14 Dec 2023 at 06:20, Wang Jinchao <wangjinchao@xfusion.com> wrote:
> > > 
> > > Remove unused `next_buddy_marked` in `check_preempt_wakeup_fair`
> > > 
> > 
> > Fixes: 5e963f2bd465 ("sched/fair: Commit to EEVDF")
> 
> After this commit @pse preempts curr without being the NEXT_BUDDY, but
> IMHO it should be, so how about this?
> 
> @@ -8259,8 +8259,11 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>         /*
>          * XXX pick_eevdf(cfs_rq) != se ?
>          */
> -       if (pick_eevdf(cfs_rq) == pse)
> +       if (pick_eevdf(cfs_rq) == pse) {
> +               if (!next_buddy_marked)
> +                       set_next_buddy(pse);
>                 goto preempt;
> +       }
> 
>         return;
> 
> which will align with before.
Seizing this opportunity to inquire about a question:
What does "buddy" mean in the context of the scheduler?

Is the effect the same between 
    preempting after pick_evfd(cfs_rq) == pse 
and 
    preempting after set_next_buddy(pse) followed by pick_evfd(cfs_rq) == pse?
Would both scenarios result in pse becoming the next scheduled se?"

Re: Re: Re: [PATCH] sched/fair: remove next_buddy_marked

[Abel Wu]

On 12/14/23 9:02 PM, Wang Jinchao Wrote:
> On Thu, Dec 14, 2023 at 08:21:53PM +0800, Abel Wu wrote:
>> On 12/14/23 4:18 PM, Vincent Guittot Wrote:
>>> On Thu, 14 Dec 2023 at 06:20, Wang Jinchao <wangjinchao@xfusion.com> wrote:
>>>>
>>>> Remove unused `next_buddy_marked` in `check_preempt_wakeup_fair`
>>>>
>>>
>>> Fixes: 5e963f2bd465 ("sched/fair: Commit to EEVDF")
>>
>> After this commit @pse preempts curr without being the NEXT_BUDDY, but
>> IMHO it should be, so how about this?
>>
>> @@ -8259,8 +8259,11 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>>          /*
>>           * XXX pick_eevdf(cfs_rq) != se ?
>>           */
>> -       if (pick_eevdf(cfs_rq) == pse)
>> +       if (pick_eevdf(cfs_rq) == pse) {
>> +               if (!next_buddy_marked)
>> +                       set_next_buddy(pse);
>>                  goto preempt;
>> +       }
>>
>>          return;
>>
>> which will align with before.
> Seizing this opportunity to inquire about a question:
> What does "buddy" mean in the context of the scheduler?

struct sched_entity

> 
> Is the effect the same between
>      preempting after pick_evfd(cfs_rq) == pse
> and
>      preempting after set_next_buddy(pse) followed by pick_evfd(cfs_rq) == pse?
> Would both scenarios result in pse becoming the next scheduled se?"

Probably, since pse is the one preempts curr, pick_next_entity() could
return pse directly without walking the rbtree. So the difference is in
performance.

Re: Re: [PATCH] sched/fair: remove next_buddy_marked

[Vincent Guittot]

On Thu, 14 Dec 2023 at 13:23, Abel Wu <wuyun.abel@bytedance.com> wrote:
>
> On 12/14/23 4:18 PM, Vincent Guittot Wrote:
> > On Thu, 14 Dec 2023 at 06:20, Wang Jinchao <wangjinchao@xfusion.com> wrote:
> >>
> >> Remove unused `next_buddy_marked` in `check_preempt_wakeup_fair`
> >>
> >
> > Fixes: 5e963f2bd465 ("sched/fair: Commit to EEVDF")
>
> After this commit @pse preempts curr without being the NEXT_BUDDY, but
> IMHO it should be, so how about this?
>
> @@ -8259,8 +8259,11 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>          /*
>           * XXX pick_eevdf(cfs_rq) != se ?
>           */
> -       if (pick_eevdf(cfs_rq) == pse)
> +       if (pick_eevdf(cfs_rq) == pse) {
> +               if (!next_buddy_marked)
> +                       set_next_buddy(pse);

I don't think this is needed because :
- NEXT_BUDDY is false by default so pick_next_entity() will not take
care of this
- pick_next_entity() will call pick_eevdf() which should return pse
unless another se that want to run 1st, wakes up in the meantime and
we should probably not take into account next buddy in this case

>                  goto preempt;
> +       }
>
>          return;
>
> which will align with before.

[tip: sched/core] sched/fair: Remove unused 'next_buddy_marked' local variable in check_preempt_wakeup_fair()

[tip-bot2 for Wang Jinchao]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     fbb66ce0b1d670c72def736a13ac9176b860df4e
Gitweb:        https://git.kernel.org/tip/fbb66ce0b1d670c72def736a13ac9176b860df4e
Author:        Wang Jinchao <wangjinchao@xfusion.com>
AuthorDate:    Thu, 14 Dec 2023 13:20:29 +08:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Sat, 23 Dec 2023 16:12:21 +01:00

sched/fair: Remove unused 'next_buddy_marked' local variable in check_preempt_wakeup_fair()

This variable became unused in:

    5e963f2bd465 ("sched/fair: Commit to EEVDF")

Signed-off-by: Wang Jinchao <wangjinchao@xfusion.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
Link: https://lore.kernel.org/r/202312141319+0800-wangjinchao@xfusion.com
---
 kernel/sched/fair.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 1d561b5..9cc2085 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8221,7 +8221,6 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int 
 	struct task_struct *curr = rq->curr;
 	struct sched_entity *se = &curr->se, *pse = &p->se;
 	struct cfs_rq *cfs_rq = task_cfs_rq(curr);
-	int next_buddy_marked = 0;
 	int cse_is_idle, pse_is_idle;
 
 	if (unlikely(se == pse))
@@ -8238,7 +8237,6 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int 
 
 	if (sched_feat(NEXT_BUDDY) && !(wake_flags & WF_FORK)) {
 		set_next_buddy(pse);
-		next_buddy_marked = 1;
 	}
 
 	/*