Remove unused `next_buddy_marked` in `check_preempt_wakeup_fair`
Signed-off-by: Wang Jinchao <[email protected]>
---
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 Thu, 14 Dec 2023 at 06:20, Wang Jinchao <[email protected]> wrote:
>
> Remove unused `next_buddy_marked` in `check_preempt_wakeup_fair`
>
Fixes: 5e963f2bd465 ("sched/fair: Commit to EEVDF")
> Signed-off-by: Wang Jinchao <[email protected]>
Reviewed-by: Vincent Guittot <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> 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 Thu, 14 Dec 2023 at 13:23, Abel Wu <[email protected]> wrote:
>
> On 12/14/23 4:18 PM, Vincent Guittot Wrote:
> > On Thu, 14 Dec 2023 at 06:20, Wang Jinchao <[email protected]> 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.
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 <[email protected]> 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.
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 <[email protected]>
AuthorDate: Thu, 14 Dec 2023 13:20:29 +08:00
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Reviewed-by: Vincent Guittot <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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;
}
/*