[RFC,36/86] entry: irqentry_exit only preempts TIF_NEED_RESCHED

Commit Message

Ankur Arora Nov. 7, 2023, 9:57 p.m. UTC

The scheduling policy for RESCHED_lazy (TIF_NEED_RESCHED_LAZY) is
to let anything running in the kernel run to completion.
Accordingly, while deciding whether to call preempt_schedule_irq()
narrow the check to tif_need_resched(RESCHED_eager).

Also add a comment about why we need to check at all, given that we
have aleady checked the preempt_count().

Signed-off-by: Ankur Arora <[e-mail]>
---
 kernel/entry/common.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Peter Zijlstra Nov. 8, 2023, 9:01 a.m. UTC | #1

On Tue, Nov 07, 2023 at 01:57:22PM -0800, Ankur Arora wrote:
> The scheduling policy for RESCHED_lazy (TIF_NEED_RESCHED_LAZY) is
> to let anything running in the kernel run to completion.
> Accordingly, while deciding whether to call preempt_schedule_irq()
> narrow the check to tif_need_resched(RESCHED_eager).
> 
> Also add a comment about why we need to check at all, given that we
> have aleady checked the preempt_count().
> 
> Signed-off-by: Ankur Arora <[e-mail]>
> ---
>  kernel/entry/common.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> index 0d055c39690b..6433e6c77185 100644
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -384,7 +384,15 @@ void irqentry_exit_cond_resched(void)
>  		rcu_irq_exit_check_preempt();
>  		if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
>  			WARN_ON_ONCE(!on_thread_stack());
> -		if (need_resched())
> +
> +		/*
> +		 * If the scheduler really wants us to preempt while returning
> +		 * to kernel, it would set TIF_NEED_RESCHED.
> +		 * On some archs the flag gets folded in preempt_count, and
> +		 * thus would be covered in the conditional above, but not all
> +		 * archs do that, so check explicitly.
> +		 */
> +		if (tif_need_resched(RESCHED_eager))
>  			preempt_schedule_irq();

See, I'm reading this like if we're eager to preempt, but then it's not
actually eager at all and only wants to preempt when forced.

This naming sucks...

Ankur Arora Nov. 21, 2023, 6 a.m. UTC | #2

Peter Zijlstra <[e-mail]> writes:

> On Tue, Nov 07, 2023 at 01:57:22PM -0800, Ankur Arora wrote:
>> The scheduling policy for RESCHED_lazy (TIF_NEED_RESCHED_LAZY) is
>> to let anything running in the kernel run to completion.
>> Accordingly, while deciding whether to call preempt_schedule_irq()
>> narrow the check to tif_need_resched(RESCHED_eager).
>>
>> Also add a comment about why we need to check at all, given that we
>> have aleady checked the preempt_count().
>>
>> Signed-off-by: Ankur Arora <[e-mail]>
>> ---
>>  kernel/entry/common.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
>> index 0d055c39690b..6433e6c77185 100644
>> --- a/kernel/entry/common.c
>> +++ b/kernel/entry/common.c
>> @@ -384,7 +384,15 @@ void irqentry_exit_cond_resched(void)
>>  		rcu_irq_exit_check_preempt();
>>  		if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
>>  			WARN_ON_ONCE(!on_thread_stack());
>> -		if (need_resched())
>> +
>> +		/*
>> +		 * If the scheduler really wants us to preempt while returning
>> +		 * to kernel, it would set TIF_NEED_RESCHED.
>> +		 * On some archs the flag gets folded in preempt_count, and
>> +		 * thus would be covered in the conditional above, but not all
>> +		 * archs do that, so check explicitly.
>> +		 */
>> +		if (tif_need_resched(RESCHED_eager))
>>  			preempt_schedule_irq();
>
> See, I'm reading this like if we're eager to preempt, but then it's not
> actually eager at all and only wants to preempt when forced.
>
> This naming sucks...

Yeah, it reads like it's trying to say something when it is just trying to
check a bit.

Does the new one read better?

--
ankur

Patch

diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 0d055c39690b..6433e6c77185 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -384,7 +384,15 @@  void irqentry_exit_cond_resched(void)
 		rcu_irq_exit_check_preempt();
 		if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
 			WARN_ON_ONCE(!on_thread_stack());
-		if (need_resched())
+
+		/*
+		 * If the scheduler really wants us to preempt while returning
+		 * to kernel, it would set TIF_NEED_RESCHED.
+		 * On some archs the flag gets folded in preempt_count, and
+		 * thus would be covered in the conditional above, but not all
+		 * archs do that, so check explicitly.
+		 */
+		if (tif_need_resched(RESCHED_eager))
 			preempt_schedule_irq();
 	}
 }