[PATCH 0/2] Fix SCHED_IDLE behavior on wakeup preemption

[PATCH 0/2] Fix SCHED_IDLE behavior on wakeup preemption

[Abel Wu]

Patch 1: Fixes unintended gap between SCHED_IDLE tasks and entities.
Patch 2: Fixes scope of WAKEUP_PREEMPTION to meet SCHED_IDLE semantics.

Abel Wu (2):
  sched/fair: Do not let idle entities preempt others
  sched/fair: Fix premature check of WAKEUP_PREEMPTION

 kernel/sched/fair.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

-- 
2.37.3

[PATCH 1/2] sched/fair: Do not let idle entities preempt others

[Abel Wu]

A task with SCHED_IDLE policy doesn't preempt others by definition, and
the semantics are intended to be preserved when extending to cgroups
introduced in commit 304000390f88 ("sched: Cgroup SCHED_IDLE support").

But current implementation allows idle entities to preempt each other
on wakeup, which seems not behave as expected especially after
commit faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
so fix this by explicitly skip wakeup preemption for idle entities.

Fixes: 304000390f88 ("sched: Cgroup SCHED_IDLE support")
Signed-off-by: Abel Wu <wuyun.abel@bytedance.com>
---
 kernel/sched/fair.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 1c0ef435a7aa..4340178f29b7 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8778,12 +8778,15 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
 	pse_is_idle = se_is_idle(pse);
 
 	/*
-	 * Preempt an idle entity in favor of a non-idle entity (and don't preempt
-	 * in the inverse case).
+	 * Preempt an idle entity in favor of a non-idle entity.
 	 */
 	if (cse_is_idle && !pse_is_idle)
 		goto preempt;
-	if (cse_is_idle != pse_is_idle)
+
+	/*
+	 * IDLE entities do not preempt others.
+	 */
+	if (unlikely(pse_is_idle))
 		return;
 
 	/*
-- 
2.37.3

[PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Abel Wu]

Idle tasks are by definition preempted by non-idle tasks whether feat
WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since
commit faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
which gives priority to WAKEUP_PREEMPTION, so when !FAIR_GROUP_SCHED,
SCHED_IDLE tasks do not preempt by non-idle tasks.

Fixes: faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
Signed-off-by: Abel Wu <wuyun.abel@bytedance.com>
---
 kernel/sched/fair.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 4340178f29b7..de9a2689de9c 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8768,9 +8768,6 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
 	if (test_tsk_need_resched(rq->curr))
 		return;
 
-	if (!sched_feat(WAKEUP_PREEMPTION))
-		return;
-
 	find_matching_se(&se, &pse);
 	WARN_ON_ONCE(!pse);
 
@@ -8783,6 +8780,9 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
 	if (cse_is_idle && !pse_is_idle)
 		goto preempt;
 
+	if (!sched_feat(WAKEUP_PREEMPTION))
+		return;
+
 	/*
 	 * IDLE entities do not preempt others.
 	 */
-- 
2.37.3

Re: [PATCH 1/2] sched/fair: Do not let idle entities preempt others

[Vincent Guittot]

On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
>
> A task with SCHED_IDLE policy doesn't preempt others by definition, and
> the semantics are intended to be preserved when extending to cgroups
> introduced in commit 304000390f88 ("sched: Cgroup SCHED_IDLE support").
>
> But current implementation allows idle entities to preempt each other
> on wakeup, which seems not behave as expected especially after
> commit faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
> so fix this by explicitly skip wakeup preemption for idle entities.
>
> Fixes: 304000390f88 ("sched: Cgroup SCHED_IDLE support")
> Signed-off-by: Abel Wu <wuyun.abel@bytedance.com>
> ---
>  kernel/sched/fair.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 1c0ef435a7aa..4340178f29b7 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8778,12 +8778,15 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>         pse_is_idle = se_is_idle(pse);
>
>         /*
> -        * Preempt an idle entity in favor of a non-idle entity (and don't preempt
> -        * in the inverse case).
> +        * Preempt an idle entity in favor of a non-idle entity.
>          */
>         if (cse_is_idle && !pse_is_idle)
>                 goto preempt;
> -       if (cse_is_idle != pse_is_idle)
> +
> +       /*
> +        * IDLE entities do not preempt others.
> +        */
> +       if (unlikely(pse_is_idle))
>                 return;

Fair enough

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

>
>         /*

> --
> 2.37.3
>

Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Vincent Guittot]

On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
>
> Idle tasks are by definition preempted by non-idle tasks whether feat
> WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since

I don't think it's true, only "sched_idle never preempts others" is
always true but sched_feat(WAKEUP_PREEMPTION) is mainly there for
debug purpose so if WAKEUP_PREEMPTION is false then nobody preempts
others at wakeup, idle, batch or normal


> commit faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
> which gives priority to WAKEUP_PREEMPTION, so when !FAIR_GROUP_SCHED,
> SCHED_IDLE tasks do not preempt by non-idle tasks.
>
> Fixes: faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
> Signed-off-by: Abel Wu <wuyun.abel@bytedance.com>
> ---
>  kernel/sched/fair.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 4340178f29b7..de9a2689de9c 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8768,9 +8768,6 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>         if (test_tsk_need_resched(rq->curr))
>                 return;
>
> -       if (!sched_feat(WAKEUP_PREEMPTION))
> -               return;
> -
>         find_matching_se(&se, &pse);
>         WARN_ON_ONCE(!pse);
>
> @@ -8783,6 +8780,9 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>         if (cse_is_idle && !pse_is_idle)
>                 goto preempt;
>
> +       if (!sched_feat(WAKEUP_PREEMPTION))
> +               return;
> +
>         /*
>          * IDLE entities do not preempt others.
>          */
> --
> 2.37.3
>

Re: Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Abel Wu]

On 2/21/25 7:49 PM, Vincent Guittot Wrote:
> On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
>>
>> Idle tasks are by definition preempted by non-idle tasks whether feat
>> WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since
> 
> I don't think it's true, only "sched_idle never preempts others" is
> always true but sched_feat(WAKEUP_PREEMPTION) is mainly there for
> debug purpose so if WAKEUP_PREEMPTION is false then nobody preempts
> others at wakeup, idle, batch or normal

Hi Vincent, thanks for your comment!

The SCHED_IDLE "definition" of being preempted by non-idle tasks comes
from commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation")
which said:

	- no SCHED_IDLE buddies
	- never let SCHED_IDLE preempt on wakeup
	- always preempt SCHED_IDLE on wakeup
	- limit SLEEPER fairness for SCHED_IDLE

and that commit let it be preempted before checking WAKEUP_PREEMPTION.
The rules were introduced in 2009, and to the best of my knowledge there
seemed no behavior change ever since. Please correct me if I missed
anything.

Best Regards,
	Abel

Re: [PATCH 1/2] sched/fair: Do not let idle entities preempt others

[Josh Don]

On Fri, Feb 21, 2025 at 3:12 AM Abel Wu <wuyun.abel@bytedance.com> wrote:
>
> A task with SCHED_IDLE policy doesn't preempt others by definition, and
> the semantics are intended to be preserved when extending to cgroups
> introduced in commit 304000390f88 ("sched: Cgroup SCHED_IDLE support").
>
> But current implementation allows idle entities to preempt each other
> on wakeup, which seems not behave as expected especially after
> commit faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
> so fix this by explicitly skip wakeup preemption for idle entities.

Thanks, looks good to me.

Reviewed-by: Josh Don <joshdon@google.com>

Re: [PATCH 1/2] sched/fair: Do not let idle entities preempt others

[Madadi Vineeth Reddy]

Hi Abel,

On 21/02/25 16:42, Abel Wu wrote:
> A task with SCHED_IDLE policy doesn't preempt others by definition, and
> the semantics are intended to be preserved when extending to cgroups
> introduced in commit 304000390f88 ("sched: Cgroup SCHED_IDLE support").
> 
> But current implementation allows idle entities to preempt each other
> on wakeup, which seems not behave as expected especially after
> commit faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
> so fix this by explicitly skip wakeup preemption for idle entities.
> 
> Fixes: 304000390f88 ("sched: Cgroup SCHED_IDLE support")
> Signed-off-by: Abel Wu <wuyun.abel@bytedance.com>
> ---
>  kernel/sched/fair.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 1c0ef435a7aa..4340178f29b7 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8778,12 +8778,15 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>  	pse_is_idle = se_is_idle(pse);
>  
>  	/*
> -	 * Preempt an idle entity in favor of a non-idle entity (and don't preempt
> -	 * in the inverse case).
> +	 * Preempt an idle entity in favor of a non-idle entity.
>  	 */
>  	if (cse_is_idle && !pse_is_idle)
>  		goto preempt;
> -	if (cse_is_idle != pse_is_idle)
> +
> +	/*
> +	 * IDLE entities do not preempt others.
> +	 */
> +	if (unlikely(pse_is_idle))
>  		return;

It makes sense that idle tasks don't preempt each other. The patch
preserves the original behavior: preemption occurs when the currently
running entity (cse) is SCHED_IDLE while the waking entity (pse) is not,
and it correctly returns when cse is not SCHED_IDLE while pse is
SCHED_IDLE.

Reviewed-by: Madadi Vineeth Reddy <vineethr@linux.ibm.com>

Thanks,
Madadi Vineeth Reddy

>  
>  	/*

Re: [PATCH 1/2] sched/fair: Do not let idle entities preempt others

[Madadi Vineeth Reddy]

On 21/02/25 16:42, Abel Wu wrote:
> A task with SCHED_IDLE policy doesn't preempt others by definition, and
> the semantics are intended to be preserved when extending to cgroups
> introduced in commit 304000390f88 ("sched: Cgroup SCHED_IDLE support").

[snip]

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 1c0ef435a7aa..4340178f29b7 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8778,12 +8778,15 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>  	pse_is_idle = se_is_idle(pse);
>  
>  	/*
> -	 * Preempt an idle entity in favor of a non-idle entity (and don't preempt
> -	 * in the inverse case).
> +	 * Preempt an idle entity in favor of a non-idle entity.
>  	 */
>  	if (cse_is_idle && !pse_is_idle)
>  		goto preempt;

This patch doesn't apply cleanly on top of tip/sched/core because of the
commit f553741ac8c0 ("sched: Cancel the slice protection of the idle entity").
Please rebase it.

Thanks,
Madadi Vineeth Reddy

> -	if (cse_is_idle != pse_is_idle)
> +
> +	/*
> +	 * IDLE entities do not preempt others.
> +	 */
> +	if (unlikely(pse_is_idle))
>  		return;
>  
>  	/*

Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Madadi Vineeth Reddy]

On 21/02/25 21:27, Abel Wu wrote:
> On 2/21/25 7:49 PM, Vincent Guittot Wrote:
>> On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
>>>
>>> Idle tasks are by definition preempted by non-idle tasks whether feat
>>> WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since
>>
>> I don't think it's true, only "sched_idle never preempts others" is
>> always true but sched_feat(WAKEUP_PREEMPTION) is mainly there for
>> debug purpose so if WAKEUP_PREEMPTION is false then nobody preempts
>> others at wakeup, idle, batch or normal
> 
> Hi Vincent, thanks for your comment!
> 
> The SCHED_IDLE "definition" of being preempted by non-idle tasks comes
> from commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation")
> which said:
> 
>     - no SCHED_IDLE buddies
>     - never let SCHED_IDLE preempt on wakeup
>     - always preempt SCHED_IDLE on wakeup
>     - limit SLEEPER fairness for SCHED_IDLE
> 
> and that commit let it be preempted before checking WAKEUP_PREEMPTION.
> The rules were introduced in 2009, and to the best of my knowledge there
> seemed no behavior change ever since. Please correct me if I missed
> anything.

As Vincent mentioned, WAKEUP_PREEMPTION is primarily for debugging. Maybe
it would help to document that SCHED_IDLE tasks are not preempted by non-idle
tasks when WAKEUP_PREEMPTION is disabled. Otherwise, the intent of having no
preemptions for debugging would be lost.

Thoughts?

Thanks,
Madadi Vineeth Reddy

> 
> Best Regards,
>     Abel
> 

Re: Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Abel Wu]

Hi Madadi,

On 2/23/25 2:16 AM, Madadi Vineeth Reddy Wrote:
> On 21/02/25 21:27, Abel Wu wrote:
>> On 2/21/25 7:49 PM, Vincent Guittot Wrote:
>>> On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
>>>>
>>>> Idle tasks are by definition preempted by non-idle tasks whether feat
>>>> WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since
>>>
>>> I don't think it's true, only "sched_idle never preempts others" is
>>> always true but sched_feat(WAKEUP_PREEMPTION) is mainly there for
>>> debug purpose so if WAKEUP_PREEMPTION is false then nobody preempts
>>> others at wakeup, idle, batch or normal
>>
>> Hi Vincent, thanks for your comment!
>>
>> The SCHED_IDLE "definition" of being preempted by non-idle tasks comes
>> from commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation")
>> which said:
>>
>>      - no SCHED_IDLE buddies
>>      - never let SCHED_IDLE preempt on wakeup
>>      - always preempt SCHED_IDLE on wakeup
>>      - limit SLEEPER fairness for SCHED_IDLE
>>
>> and that commit let it be preempted before checking WAKEUP_PREEMPTION.
>> The rules were introduced in 2009, and to the best of my knowledge there
>> seemed no behavior change ever since. Please correct me if I missed
>> anything.
> 
> As Vincent mentioned, WAKEUP_PREEMPTION is primarily for debugging. Maybe
> it would help to document that SCHED_IDLE tasks are not preempted by non-idle
> tasks when WAKEUP_PREEMPTION is disabled. Otherwise, the intent of having no
> preemptions for debugging would be lost.
> 
> Thoughts?

I am not sure I really understand the purpose of this debug feature.
If it wants to provide a way to check whether a performance degrade of
certain workload is due to overscheduling or not, then do we really
care about performance of SCHED_IDLE workloads and why?

IMHO preempting SCHED_IDLE before WAKEUP_PREEMPTION is to preserve the
IDLE semantics trying to behave like real idle task. It is somehow
weird to me that we treat sched-idle cpus as idle while don't let the
non-idle tasks run immediately on sched-idle cpus on debug case.

Thanks,
	Abel

Re: Re: [PATCH 1/2] sched/fair: Do not let idle entities preempt others

[Abel Wu]

On 2/23/25 1:48 AM, Madadi Vineeth Reddy Wrote:
> On 21/02/25 16:42, Abel Wu wrote:
>> A task with SCHED_IDLE policy doesn't preempt others by definition, and
>> the semantics are intended to be preserved when extending to cgroups
>> introduced in commit 304000390f88 ("sched: Cgroup SCHED_IDLE support").
> 
> [snip]
> 
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 1c0ef435a7aa..4340178f29b7 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -8778,12 +8778,15 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>>   	pse_is_idle = se_is_idle(pse);
>>   
>>   	/*
>> -	 * Preempt an idle entity in favor of a non-idle entity (and don't preempt
>> -	 * in the inverse case).
>> +	 * Preempt an idle entity in favor of a non-idle entity.
>>   	 */
>>   	if (cse_is_idle && !pse_is_idle)
>>   		goto preempt;
> 
> This patch doesn't apply cleanly on top of tip/sched/core because of the
> commit f553741ac8c0 ("sched: Cancel the slice protection of the idle entity").
> Please rebase it.

Will rebase. Thanks!

> 
> Thanks,
> Madadi Vineeth Reddy
> 
>> -	if (cse_is_idle != pse_is_idle)
>> +
>> +	/*
>> +	 * IDLE entities do not preempt others.
>> +	 */
>> +	if (unlikely(pse_is_idle))
>>   		return;
>>   
>>   	/*
> 

Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Madadi Vineeth Reddy]

On 23/02/25 14:14, Abel Wu wrote:
> Hi Madadi,
> 
> On 2/23/25 2:16 AM, Madadi Vineeth Reddy Wrote:
>> On 21/02/25 21:27, Abel Wu wrote:
>>> On 2/21/25 7:49 PM, Vincent Guittot Wrote:
>>>> On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
>>>>>
>>>>> Idle tasks are by definition preempted by non-idle tasks whether feat
>>>>> WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since
>>>>
>>>> I don't think it's true, only "sched_idle never preempts others" is
>>>> always true but sched_feat(WAKEUP_PREEMPTION) is mainly there for
>>>> debug purpose so if WAKEUP_PREEMPTION is false then nobody preempts
>>>> others at wakeup, idle, batch or normal
>>>
>>> Hi Vincent, thanks for your comment!
>>>
>>> The SCHED_IDLE "definition" of being preempted by non-idle tasks comes
>>> from commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation")
>>> which said:
>>>
>>>      - no SCHED_IDLE buddies
>>>      - never let SCHED_IDLE preempt on wakeup
>>>      - always preempt SCHED_IDLE on wakeup
>>>      - limit SLEEPER fairness for SCHED_IDLE
>>>
>>> and that commit let it be preempted before checking WAKEUP_PREEMPTION.
>>> The rules were introduced in 2009, and to the best of my knowledge there
>>> seemed no behavior change ever since. Please correct me if I missed
>>> anything.
>>
>> As Vincent mentioned, WAKEUP_PREEMPTION is primarily for debugging. Maybe
>> it would help to document that SCHED_IDLE tasks are not preempted by non-idle
>> tasks when WAKEUP_PREEMPTION is disabled. Otherwise, the intent of having no
>> preemptions for debugging would be lost.
>>
>> Thoughts?
> 
> I am not sure I really understand the purpose of this debug feature.
> If it wants to provide a way to check whether a performance degrade of
> certain workload is due to overscheduling or not, then do we really
> care about performance of SCHED_IDLE workloads and why?

It's true that we may not be too concerned about performance with
SCHED_IDLE. The issue is preserve the original SCHED_IDLE definition
versus WAKEUP_PREEMPTION, which applies across all policies. Since by
default the feature is true. I am not sure. Either way seems ok to me.

Thanks,
Madadi Vineeth Reddy

> 
> IMHO preempting SCHED_IDLE before WAKEUP_PREEMPTION is to preserve the
> IDLE semantics trying to behave like real idle task. It is somehow
> weird to me that we treat sched-idle cpus as idle while don't let the
> non-idle tasks run immediately on sched-idle cpus on debug case.
> 
> Thanks,
>     Abel
> 

Re: Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Abel Wu]

On 2/23/25 6:25 PM, Madadi Vineeth Reddy Wrote:
> On 23/02/25 14:14, Abel Wu wrote:
>> Hi Madadi,
>>
>> On 2/23/25 2:16 AM, Madadi Vineeth Reddy Wrote:
>>> On 21/02/25 21:27, Abel Wu wrote:
>>>> On 2/21/25 7:49 PM, Vincent Guittot Wrote:
>>>>> On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
>>>>>>
>>>>>> Idle tasks are by definition preempted by non-idle tasks whether feat
>>>>>> WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since
>>>>>
>>>>> I don't think it's true, only "sched_idle never preempts others" is
>>>>> always true but sched_feat(WAKEUP_PREEMPTION) is mainly there for
>>>>> debug purpose so if WAKEUP_PREEMPTION is false then nobody preempts
>>>>> others at wakeup, idle, batch or normal
>>>>
>>>> Hi Vincent, thanks for your comment!
>>>>
>>>> The SCHED_IDLE "definition" of being preempted by non-idle tasks comes
>>>> from commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation")
>>>> which said:
>>>>
>>>>       - no SCHED_IDLE buddies
>>>>       - never let SCHED_IDLE preempt on wakeup
>>>>       - always preempt SCHED_IDLE on wakeup
>>>>       - limit SLEEPER fairness for SCHED_IDLE
>>>>
>>>> and that commit let it be preempted before checking WAKEUP_PREEMPTION.
>>>> The rules were introduced in 2009, and to the best of my knowledge there
>>>> seemed no behavior change ever since. Please correct me if I missed
>>>> anything.
>>>
>>> As Vincent mentioned, WAKEUP_PREEMPTION is primarily for debugging. Maybe
>>> it would help to document that SCHED_IDLE tasks are not preempted by non-idle
>>> tasks when WAKEUP_PREEMPTION is disabled. Otherwise, the intent of having no
>>> preemptions for debugging would be lost.
>>>
>>> Thoughts?
>>
>> I am not sure I really understand the purpose of this debug feature.
>> If it wants to provide a way to check whether a performance degrade of
>> certain workload is due to overscheduling or not, then do we really
>> care about performance of SCHED_IDLE workloads and why?
> 
> It's true that we may not be too concerned about performance with
> SCHED_IDLE. The issue is preserve the original SCHED_IDLE definition
> versus WAKEUP_PREEMPTION, which applies across all policies. Since by

Yes, exactly.

> default the feature is true. I am not sure. Either way seems ok to me.

Hi Vincent,

Since Peter gave the priority to SCHED_IDLE semantics over WAKEUP_PREEMPTION
in his commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation"),
and the choice is kept unchanged for quite a long time until the recent merged
commit faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
which seemed not intend to change it, shall we restore the choice for now and
leave the discussion of the scope of WAKEUP_PREEMPTION to the future once any
usecase shows up?

Thanks,
	Abel

Re: Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Vincent Guittot]

On Sun, 23 Feb 2025 at 09:45, Abel Wu <wuyun.abel@bytedance.com> wrote:
>
> Hi Madadi,
>
> On 2/23/25 2:16 AM, Madadi Vineeth Reddy Wrote:
> > On 21/02/25 21:27, Abel Wu wrote:
> >> On 2/21/25 7:49 PM, Vincent Guittot Wrote:
> >>> On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
> >>>>
> >>>> Idle tasks are by definition preempted by non-idle tasks whether feat
> >>>> WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since
> >>>
> >>> I don't think it's true, only "sched_idle never preempts others" is
> >>> always true but sched_feat(WAKEUP_PREEMPTION) is mainly there for
> >>> debug purpose so if WAKEUP_PREEMPTION is false then nobody preempts
> >>> others at wakeup, idle, batch or normal
> >>
> >> Hi Vincent, thanks for your comment!
> >>
> >> The SCHED_IDLE "definition" of being preempted by non-idle tasks comes
> >> from commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation")
> >> which said:
> >>
> >>      - no SCHED_IDLE buddies
> >>      - never let SCHED_IDLE preempt on wakeup
> >>      - always preempt SCHED_IDLE on wakeup
> >>      - limit SLEEPER fairness for SCHED_IDLE
> >>
> >> and that commit let it be preempted before checking WAKEUP_PREEMPTION.
> >> The rules were introduced in 2009, and to the best of my knowledge there
> >> seemed no behavior change ever since. Please correct me if I missed
> >> anything.
> >
> > As Vincent mentioned, WAKEUP_PREEMPTION is primarily for debugging. Maybe
> > it would help to document that SCHED_IDLE tasks are not preempted by non-idle
> > tasks when WAKEUP_PREEMPTION is disabled. Otherwise, the intent of having no
> > preemptions for debugging would be lost.
> >
> > Thoughts?
>
> I am not sure I really understand the purpose of this debug feature.

I'm not sure that it has been even be used or useful for the last
couple of years. Should we just remove it ?

> If it wants to provide a way to check whether a performance degrade of
> certain workload is due to overscheduling or not, then do we really
> care about performance of SCHED_IDLE workloads and why?
>
> IMHO preempting SCHED_IDLE before WAKEUP_PREEMPTION is to preserve the
> IDLE semantics trying to behave like real idle task. It is somehow
> weird to me that we treat sched-idle cpus as idle while don't let the
> non-idle tasks run immediately on sched-idle cpus on debug case.
>
> Thanks,
>         Abel
>

Re: Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Vincent Guittot]

On Sun, 23 Feb 2025 at 12:22, Abel Wu <wuyun.abel@bytedance.com> wrote:
>
> On 2/23/25 6:25 PM, Madadi Vineeth Reddy Wrote:
> > On 23/02/25 14:14, Abel Wu wrote:
> >> Hi Madadi,
> >>
> >> On 2/23/25 2:16 AM, Madadi Vineeth Reddy Wrote:
> >>> On 21/02/25 21:27, Abel Wu wrote:
> >>>> On 2/21/25 7:49 PM, Vincent Guittot Wrote:
> >>>>> On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
> >>>>>>
> >>>>>> Idle tasks are by definition preempted by non-idle tasks whether feat
> >>>>>> WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since
> >>>>>
> >>>>> I don't think it's true, only "sched_idle never preempts others" is
> >>>>> always true but sched_feat(WAKEUP_PREEMPTION) is mainly there for
> >>>>> debug purpose so if WAKEUP_PREEMPTION is false then nobody preempts
> >>>>> others at wakeup, idle, batch or normal
> >>>>
> >>>> Hi Vincent, thanks for your comment!
> >>>>
> >>>> The SCHED_IDLE "definition" of being preempted by non-idle tasks comes
> >>>> from commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation")
> >>>> which said:
> >>>>
> >>>>       - no SCHED_IDLE buddies
> >>>>       - never let SCHED_IDLE preempt on wakeup
> >>>>       - always preempt SCHED_IDLE on wakeup
> >>>>       - limit SLEEPER fairness for SCHED_IDLE
> >>>>
> >>>> and that commit let it be preempted before checking WAKEUP_PREEMPTION.
> >>>> The rules were introduced in 2009, and to the best of my knowledge there
> >>>> seemed no behavior change ever since. Please correct me if I missed
> >>>> anything.
> >>>
> >>> As Vincent mentioned, WAKEUP_PREEMPTION is primarily for debugging. Maybe
> >>> it would help to document that SCHED_IDLE tasks are not preempted by non-idle
> >>> tasks when WAKEUP_PREEMPTION is disabled. Otherwise, the intent of having no
> >>> preemptions for debugging would be lost.
> >>>
> >>> Thoughts?
> >>
> >> I am not sure I really understand the purpose of this debug feature.
> >> If it wants to provide a way to check whether a performance degrade of
> >> certain workload is due to overscheduling or not, then do we really
> >> care about performance of SCHED_IDLE workloads and why?
> >
> > It's true that we may not be too concerned about performance with
> > SCHED_IDLE. The issue is preserve the original SCHED_IDLE definition
> > versus WAKEUP_PREEMPTION, which applies across all policies. Since by
>
> Yes, exactly.
>
> > default the feature is true. I am not sure. Either way seems ok to me.
>
> Hi Vincent,
>
> Since Peter gave the priority to SCHED_IDLE semantics over WAKEUP_PREEMPTION
> in his commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation"),
> and the choice is kept unchanged for quite a long time until the recent merged
> commit faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
> which seemed not intend to change it, shall we restore the choice for now and
> leave the discussion of the scope of WAKEUP_PREEMPTION to the future once any
> usecase shows up?

Or we should just remove it. I'm curious to know who used it during
the last couple of years ? Having in mind that lazy preemption adds
another level as check_preempt_wakeup_fair()  uses it so sched-idle
tasks might not always be immediately preempted anyway.


>
> Thanks,
>         Abel
>

Re: Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Phil Auld]

On Mon, Feb 24, 2025 at 02:47:13PM +0100 Vincent Guittot wrote:
> On Sun, 23 Feb 2025 at 12:22, Abel Wu <wuyun.abel@bytedance.com> wrote:
> >
> > On 2/23/25 6:25 PM, Madadi Vineeth Reddy Wrote:
> > > On 23/02/25 14:14, Abel Wu wrote:
> > >> Hi Madadi,
> > >>
> > >> On 2/23/25 2:16 AM, Madadi Vineeth Reddy Wrote:
> > >>> On 21/02/25 21:27, Abel Wu wrote:
> > >>>> On 2/21/25 7:49 PM, Vincent Guittot Wrote:
> > >>>>> On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
> > >>>>>>
> > >>>>>> Idle tasks are by definition preempted by non-idle tasks whether feat
> > >>>>>> WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since
> > >>>>>
> > >>>>> I don't think it's true, only "sched_idle never preempts others" is
> > >>>>> always true but sched_feat(WAKEUP_PREEMPTION) is mainly there for
> > >>>>> debug purpose so if WAKEUP_PREEMPTION is false then nobody preempts
> > >>>>> others at wakeup, idle, batch or normal
> > >>>>
> > >>>> Hi Vincent, thanks for your comment!
> > >>>>
> > >>>> The SCHED_IDLE "definition" of being preempted by non-idle tasks comes
> > >>>> from commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation")
> > >>>> which said:
> > >>>>
> > >>>>       - no SCHED_IDLE buddies
> > >>>>       - never let SCHED_IDLE preempt on wakeup
> > >>>>       - always preempt SCHED_IDLE on wakeup
> > >>>>       - limit SLEEPER fairness for SCHED_IDLE
> > >>>>
> > >>>> and that commit let it be preempted before checking WAKEUP_PREEMPTION.
> > >>>> The rules were introduced in 2009, and to the best of my knowledge there
> > >>>> seemed no behavior change ever since. Please correct me if I missed
> > >>>> anything.
> > >>>
> > >>> As Vincent mentioned, WAKEUP_PREEMPTION is primarily for debugging. Maybe
> > >>> it would help to document that SCHED_IDLE tasks are not preempted by non-idle
> > >>> tasks when WAKEUP_PREEMPTION is disabled. Otherwise, the intent of having no
> > >>> preemptions for debugging would be lost.
> > >>>
> > >>> Thoughts?
> > >>
> > >> I am not sure I really understand the purpose of this debug feature.
> > >> If it wants to provide a way to check whether a performance degrade of
> > >> certain workload is due to overscheduling or not, then do we really
> > >> care about performance of SCHED_IDLE workloads and why?
> > >
> > > It's true that we may not be too concerned about performance with
> > > SCHED_IDLE. The issue is preserve the original SCHED_IDLE definition
> > > versus WAKEUP_PREEMPTION, which applies across all policies. Since by
> >
> > Yes, exactly.
> >
> > > default the feature is true. I am not sure. Either way seems ok to me.
> >
> > Hi Vincent,
> >
> > Since Peter gave the priority to SCHED_IDLE semantics over WAKEUP_PREEMPTION
> > in his commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation"),
> > and the choice is kept unchanged for quite a long time until the recent merged
> > commit faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
> > which seemed not intend to change it, shall we restore the choice for now and
> > leave the discussion of the scope of WAKEUP_PREEMPTION to the future once any
> > usecase shows up?
> 
> Or we should just remove it. I'm curious to know who used it during
> the last couple of years ? Having in mind that lazy preemption adds
> another level as check_preempt_wakeup_fair()  uses it so sched-idle
> tasks might not always be immediately preempted anyway.
> 

It can be helpful to be able to turn that off when chasing performance
issues. See the DELAY_DEQUEUE thread from a few months back. In that
case we never got to a good answer, but did use NO_WAKEUP_PREEMPTION
during debugging to take out some variables at least. FWIW.


Cheers,
Phil

> 
> >
> > Thanks,
> >         Abel
> >
> 

-- 

Re: Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Abel Wu]

On 2/24/25 9:47 PM, Vincent Guittot wrote:
> 
> Or we should just remove it. I'm curious to know who used it during
> the last couple of years ? Having in mind that lazy preemption adds

TBH I have never used this feature. But since Phil mentioned a case
in debugging DELAY_DEQUEUE, I think we'd better keep it, what do you
think?

> another level as check_preempt_wakeup_fair()  uses it so sched-idle
> tasks might not always be immediately preempted anyway.

Right, thanks for mention that.

> 
> 
>>
>> Thanks,
>>          Abel
>>

Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Tianchen Ding]

On 2/24/25 9:47 PM, Vincent Guittot wrote:
[...]
> 
> Or we should just remove it. I'm curious to know who used it during
> the last couple of years ? Having in mind that lazy preemption adds
> another level as check_preempt_wakeup_fair()  uses it so sched-idle
> tasks might not always be immediately preempted anyway.
> 

I just remembered that I've mentioned this issue in another thread[1] 
before. Can we do preempt SCHED_IDLE immediately even in PREEMPT_LAZY? (to 
achieve better response time for SCHED_NORMAL)

[1] 
https://lore.kernel.org/all/8e6f02a0-2bd0-4e75-9055-2cb7c508ce4e@linux.alibaba.com/

Re: Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Vincent Guittot]

On Mon, 24 Feb 2025 at 15:10, Phil Auld <pauld@redhat.com> wrote:
>
> On Mon, Feb 24, 2025 at 02:47:13PM +0100 Vincent Guittot wrote:
> > On Sun, 23 Feb 2025 at 12:22, Abel Wu <wuyun.abel@bytedance.com> wrote:
> > >
> > > On 2/23/25 6:25 PM, Madadi Vineeth Reddy Wrote:
> > > > On 23/02/25 14:14, Abel Wu wrote:
> > > >> Hi Madadi,
> > > >>
> > > >> On 2/23/25 2:16 AM, Madadi Vineeth Reddy Wrote:
> > > >>> On 21/02/25 21:27, Abel Wu wrote:
> > > >>>> On 2/21/25 7:49 PM, Vincent Guittot Wrote:
> > > >>>>> On Fri, 21 Feb 2025 at 12:12, Abel Wu <wuyun.abel@bytedance.com> wrote:
> > > >>>>>>
> > > >>>>>> Idle tasks are by definition preempted by non-idle tasks whether feat
> > > >>>>>> WAKEUP_PREEMPTION is enabled or not. This isn't true any longer since
> > > >>>>>
> > > >>>>> I don't think it's true, only "sched_idle never preempts others" is
> > > >>>>> always true but sched_feat(WAKEUP_PREEMPTION) is mainly there for
> > > >>>>> debug purpose so if WAKEUP_PREEMPTION is false then nobody preempts
> > > >>>>> others at wakeup, idle, batch or normal
> > > >>>>
> > > >>>> Hi Vincent, thanks for your comment!
> > > >>>>
> > > >>>> The SCHED_IDLE "definition" of being preempted by non-idle tasks comes
> > > >>>> from commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation")
> > > >>>> which said:
> > > >>>>
> > > >>>>       - no SCHED_IDLE buddies
> > > >>>>       - never let SCHED_IDLE preempt on wakeup
> > > >>>>       - always preempt SCHED_IDLE on wakeup
> > > >>>>       - limit SLEEPER fairness for SCHED_IDLE
> > > >>>>
> > > >>>> and that commit let it be preempted before checking WAKEUP_PREEMPTION.
> > > >>>> The rules were introduced in 2009, and to the best of my knowledge there
> > > >>>> seemed no behavior change ever since. Please correct me if I missed
> > > >>>> anything.
> > > >>>
> > > >>> As Vincent mentioned, WAKEUP_PREEMPTION is primarily for debugging. Maybe
> > > >>> it would help to document that SCHED_IDLE tasks are not preempted by non-idle
> > > >>> tasks when WAKEUP_PREEMPTION is disabled. Otherwise, the intent of having no
> > > >>> preemptions for debugging would be lost.
> > > >>>
> > > >>> Thoughts?
> > > >>
> > > >> I am not sure I really understand the purpose of this debug feature.
> > > >> If it wants to provide a way to check whether a performance degrade of
> > > >> certain workload is due to overscheduling or not, then do we really
> > > >> care about performance of SCHED_IDLE workloads and why?
> > > >
> > > > It's true that we may not be too concerned about performance with
> > > > SCHED_IDLE. The issue is preserve the original SCHED_IDLE definition
> > > > versus WAKEUP_PREEMPTION, which applies across all policies. Since by
> > >
> > > Yes, exactly.
> > >
> > > > default the feature is true. I am not sure. Either way seems ok to me.
> > >
> > > Hi Vincent,
> > >
> > > Since Peter gave the priority to SCHED_IDLE semantics over WAKEUP_PREEMPTION
> > > in his commit 6bc912b71b6f ("sched: SCHED_OTHER vs SCHED_IDLE isolation"),
> > > and the choice is kept unchanged for quite a long time until the recent merged
> > > commit faa42d29419d ("sched/fair: Make SCHED_IDLE entity be preempted in strict hierarchy")
> > > which seemed not intend to change it, shall we restore the choice for now and
> > > leave the discussion of the scope of WAKEUP_PREEMPTION to the future once any
> > > usecase shows up?
> >
> > Or we should just remove it. I'm curious to know who used it during
> > the last couple of years ? Having in mind that lazy preemption adds
> > another level as check_preempt_wakeup_fair()  uses it so sched-idle
> > tasks might not always be immediately preempted anyway.
> >
>
> It can be helpful to be able to turn that off when chasing performance
> issues. See the DELAY_DEQUEUE thread from a few months back. In that
> case we never got to a good answer, but did use NO_WAKEUP_PREEMPTION
> during debugging to take out some variables at least. FWIW.

ok, I didn't remember it has been used

>
>
> Cheers,
> Phil
>
> >
> > >
> > > Thanks,
> > >         Abel
> > >
> >
>
> --
>

Re: Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Vincent Guittot]

On Tue, 25 Feb 2025 at 07:29, Abel Wu <wuyun.abel@bytedance.com> wrote:
>
> On 2/24/25 9:47 PM, Vincent Guittot wrote:
> >
> > Or we should just remove it. I'm curious to know who used it during
> > the last couple of years ? Having in mind that lazy preemption adds
>
> TBH I have never used this feature. But since Phil mentioned a case
> in debugging DELAY_DEQUEUE, I think we'd better keep it, what do you
> think?

Yes. And we need to figure out how to deal with the below as well

>
> > another level as check_preempt_wakeup_fair()  uses it so sched-idle
> > tasks might not always be immediately preempted anyway.
>
> Right, thanks for mention that.
>
> >
> >
> >>
> >> Thanks,
> >>          Abel
> >>
>

Re: Re: [PATCH 2/2] sched/fair: Fix premature check of WAKEUP_PREEMPTION

[Abel Wu]

On 2/26/25 1:15 AM, Vincent Guittot wrote:
> On Tue, 25 Feb 2025 at 07:29, Abel Wu <wuyun.abel@bytedance.com> wrote:
>>
>> On 2/24/25 9:47 PM, Vincent Guittot wrote:
>>>
>>> Or we should just remove it. I'm curious to know who used it during
>>> the last couple of years ? Having in mind that lazy preemption adds
>>
>> TBH I have never used this feature. But since Phil mentioned a case
>> in debugging DELAY_DEQUEUE, I think we'd better keep it, what do you
>> think?
> 
> Yes. And we need to figure out how to deal with the below as well

Hi Vincent, Tianchen,

I'm not sure this is the right way to do to let SCHED_IDLE be promoted
to the full NEED_RESCHED, as LAZY has relaxed responsiveness of normal
tasks to TICK_NSEC/2 in avg and workloads using fair policies should
adjust their expectations on it. And I would also recommend scheduling
policies playing with each other inside the scope of policy, while the
preemption model is another scope. Tying the two scopes together might
make things complicate, although I can imagine that certain workloads
or scenarios will benefit from it.

Best Regards,
	Abel

> 
>>
>>> another level as check_preempt_wakeup_fair()  uses it so sched-idle
>>> tasks might not always be immediately preempted anyway.
>>
>> Right, thanks for mention that.
>>
>>>
>>>
>>>>
>>>> Thanks,
>>>>           Abel
>>>>
>>