sched/fair: remove next_buddy_marked

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

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
>
  

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.
  

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

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.
  

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.