diff mbox series

[RFC,34/86] thread_info: accessors for TIF_NEED_RESCHED*

Message ID 20231107215742.363031-35-ankur.a.arora@oracle.com
State New
Headers
Series Make the kernel preemptible |

Commit Message

Ankur Arora Nov. 7, 2023, 9:57 p.m. UTC 
  Add tif_resched() which will be used as an accessor for TIF_NEED_RESCHED
and TIF_NEED_RESCHED_LAZY. The intent is to force the caller to make an
explicit choice of how eagerly they want a reschedule.

This interface will be used almost entirely from core kernel code, so
forcing a choice shouldn't be too onerous.

Originally-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>

---

1) Adding an enum for an interface that doesn't do all that much, seems
   to be overkill. This could have been an int/bool etc, but that seemed
   much less clear and thus more error prone.

2) Also there's no fallback path for architectures that don't define
   define TIF_NEED_RESCHD_LAZY. That's because arch support is easy
   to add (modulo ARCH_NO_PREEMPT, discussed in a different patch)
   so it will be simple to do that instead of thinking through what
   seemed like a slightly convoluted alternative model.

---
 include/linux/thread_info.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Peter Zijlstra Nov. 8, 2023, 8:58 a.m. UTC | #1 
On Tue, Nov 07, 2023 at 01:57:20PM -0800, Ankur Arora wrote:
> Add tif_resched() which will be used as an accessor for TIF_NEED_RESCHED
> and TIF_NEED_RESCHED_LAZY. The intent is to force the caller to make an
> explicit choice of how eagerly they want a reschedule.
> 
> This interface will be used almost entirely from core kernel code, so
> forcing a choice shouldn't be too onerous.
> 
> Originally-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>

> ---
>  include/linux/thread_info.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index 9ea0b28068f4..4eb22b13bf64 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -59,6 +59,27 @@ enum syscall_work_bit {
>  
>  #include <asm/thread_info.h>
>  
> +#ifndef TIF_NEED_RESCHED_LAZY
> +#error "Arch needs to define TIF_NEED_RESCHED_LAZY"
> +#endif
> +
> +#define TIF_NEED_RESCHED_LAZY_OFFSET	(TIF_NEED_RESCHED_LAZY - TIF_NEED_RESCHED)
> +
> +typedef enum {
> +	RESCHED_eager = 0,
> +	RESCHED_lazy = TIF_NEED_RESCHED_LAZY_OFFSET,
> +} resched_t;
> +
> +static inline int tif_resched(resched_t r)
> +{
> +	return TIF_NEED_RESCHED + r;
> +}
> +
> +static inline int _tif_resched(resched_t r)
> +{
> +	return 1 << tif_resched(r);
> +}

So either I'm confused or I'm thinking this is wrong. If you want to
preempt eagerly you want to preempt more than when you're not eager to
preempt, right?

So an eager preemption site wants to include the LAZY bit.

Whereas a site that wants to lazily preempt would prefer to not preempt
until forced, and hence would not include LAZY bit.
Ankur Arora Nov. 21, 2023, 5:59 a.m. UTC | #2 
Peter Zijlstra <peterz@infradead.org> writes:

> On Tue, Nov 07, 2023 at 01:57:20PM -0800, Ankur Arora wrote:
>> Add tif_resched() which will be used as an accessor for TIF_NEED_RESCHED
>> and TIF_NEED_RESCHED_LAZY. The intent is to force the caller to make an
>> explicit choice of how eagerly they want a reschedule.
>>
>> This interface will be used almost entirely from core kernel code, so
>> forcing a choice shouldn't be too onerous.
>>
>> Originally-by: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>
>> ---
>>  include/linux/thread_info.h | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
>> index 9ea0b28068f4..4eb22b13bf64 100644
>> --- a/include/linux/thread_info.h
>> +++ b/include/linux/thread_info.h
>> @@ -59,6 +59,27 @@ enum syscall_work_bit {
>>
>>  #include <asm/thread_info.h>
>>
>> +#ifndef TIF_NEED_RESCHED_LAZY
>> +#error "Arch needs to define TIF_NEED_RESCHED_LAZY"
>> +#endif
>> +
>> +#define TIF_NEED_RESCHED_LAZY_OFFSET	(TIF_NEED_RESCHED_LAZY - TIF_NEED_RESCHED)
>> +
>> +typedef enum {
>> +	RESCHED_eager = 0,
>> +	RESCHED_lazy = TIF_NEED_RESCHED_LAZY_OFFSET,
>> +} resched_t;
>> +
>> +static inline int tif_resched(resched_t r)
>> +{
>> +	return TIF_NEED_RESCHED + r;
>> +}
>> +
>> +static inline int _tif_resched(resched_t r)
>> +{
>> +	return 1 << tif_resched(r);
>> +}
>
> So either I'm confused or I'm thinking this is wrong. If you want to
> preempt eagerly you want to preempt more than when you're not eager to
> preempt, right?
>
> So an eager preemption site wants to include the LAZY bit.
>
> Whereas a site that wants to lazily preempt would prefer to not preempt
> until forced, and hence would not include LAZY bit.

This wasn't meant to be quite that sophisticated.
tif_resched(RESCHED_eager) means you preempt immediately/eagerly and
tif_resched(RESCHED_lazy) means you want deferred preemption.

I changed it to:

typedef enum {
	NR_now = 0,
	NR_lazy = TIF_NEED_RESCHED_LAZY_OFFSET,
} resched_t;

So, to get the respective bit we would have: tif_resched(NR_now) or
tif_resched(NR_lazy).

And the immediate preemption checks would be...

	if (tif_need_resched(NR_now))
		preempt_schedule_irq();

Does this read better?

--
ankur
diff mbox series

Patch

diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 9ea0b28068f4..4eb22b13bf64 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -59,6 +59,27 @@  enum syscall_work_bit {
 
 #include <asm/thread_info.h>
 
+#ifndef TIF_NEED_RESCHED_LAZY
+#error "Arch needs to define TIF_NEED_RESCHED_LAZY"
+#endif
+
+#define TIF_NEED_RESCHED_LAZY_OFFSET	(TIF_NEED_RESCHED_LAZY - TIF_NEED_RESCHED)
+
+typedef enum {
+	RESCHED_eager = 0,
+	RESCHED_lazy = TIF_NEED_RESCHED_LAZY_OFFSET,
+} resched_t;
+
+static inline int tif_resched(resched_t r)
+{
+	return TIF_NEED_RESCHED + r;
+}
+
+static inline int _tif_resched(resched_t r)
+{
+	return 1 << tif_resched(r);
+}
+
 #ifdef __KERNEL__
 
 #ifndef arch_set_restart_data