Re: [PATCH 03/30] thread_info: tif_need_resched() now takes resched_t as param

[Ankur Arora]

Mark Rutland <mark.rutland@xxxxxxx> writes:

> On Mon, Feb 12, 2024 at 09:55:27PM -0800, Ankur Arora wrote:
>> tif_need_resched() now takes a resched_t parameter to decide the
>> immediacy of the need-resched.
>
> I see at the end of the series, most callers pass a constant:
>
> [mark@lakrids:~/src/linux]% git grep -w tif_need_resched
> arch/s390/include/asm/preempt.h:        return !--S390_lowcore.preempt_count && tif_need_resched(NR_now);
> arch/s390/include/asm/preempt.h:                        tif_need_resched(NR_now));
> include/asm-generic/preempt.h:  return !--*preempt_count_ptr() && tif_need_resched(NR_now);
> include/asm-generic/preempt.h:                  tif_need_resched(NR_now));
> include/linux/preempt.h:        if (tif_need_resched(NR_now)) \
> include/linux/sched.h:  return unlikely(tif_need_resched(NR_now));
> include/linux/sched.h:          unlikely(tif_need_resched(NR_lazy));
> include/linux/thread_info.h:static __always_inline bool tif_need_resched(resched_t rs)
> include/linux/thread_info.h:     * With !PREEMPT_AUTO tif_need_resched(NR_lazy) is defined
> kernel/entry/common.c:          if (tif_need_resched(NR_now))
> kernel/sched/debug.c:   nr = tif_need_resched(NR_now) ? "need_resched" : "need_resched_lazy";
> kernel/trace/trace.c:   if (tif_need_resched(NR_now))
> kernel/trace/trace.c:   if (tif_need_resched(NR_lazy))
>
> I think it'd be clearer if we had tif_need_resched_now() and
> tif_need_resched_lazy() wrappers rather than taking a parameter. I think that
> if we did similar elsewhere (e.g. {set,test}_tsk_need_resched_{now,lazy}()),
> it'd be a bit cleaner overall, since we can special-case the lazy behaviour
> more easily/clearly.

So, we have three need-resched interfaces:

1. need_resched(), need_resched_lazy()
 These are used all over non-core (and idle) code, and I don't
 see a case where the user would find it useful to dynamically
 choose one or the other.
 So, here two separate interfaces, need_resched()/need_resched_lazy()
 make sense.

2. tif_need_resched()
 This is mostly used from preempt.h or scheduler adjacent code to drive
 preemption and at least in current uses, the resched_t param is a
 compile time constant.

 I think the scheduler might find it useful in some cases to parametrize
 it (ex. maybe the scheduler knows how long which bit has been set for
 over long and wants to pass that on to resched_latency_warn().)

 But that's a contrived example. I think this one would be fine
 either way. Will try it out and see which (tif_need_resched(rs),
 or tif_need_resched_now()/tif_need_resched_lazy()) seems cleaner.

3. *_tsk_need_resched()
 This is is used almost entirely from the scheduler and RCU.

 One place where I found the ability to parametrize quite useful
 was __resched_curr(). So this I would like to keep.

All of that said, and I wonder if we need these new interfaces at all.
Most of the code only uses the NR_now interface. Only the scheduler and
the entry code need to distinguish between lazy and eager.
(Plus, this way lazy and eager becomes an implementation detail which
doesn't need to be known outside the scheduler. Which is also kind of
the point of PREEMPT_AUTO.)

Say something like the patch below (and similar for tif_need_resched(),
need_resched() etc.)

What do you think?

Ankur

---------
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 58e6ea7572a0..b836b238b117 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1953,7 +1953,7 @@ static inline bool test_tsk_thread_flag(struct task_struct *tsk, int flag)
  * tif_resched(NR_now). Add a check in the helpers below to ensure
  * we don't touch the tif_reshed(NR_now) bit unnecessarily.
  */
-static inline void set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
+static inline void __set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
 {
 	if (IS_ENABLED(CONFIG_PREEMPT_AUTO) || rs == NR_now)
 		set_tsk_thread_flag(tsk, tif_resched(rs));
@@ -1964,6 +1964,11 @@ static inline void set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
 		BUG();
 }

+static inline void set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
+{
+	__set_tsk_need_resched(tsk, NR_now);
+}
+
 static inline void clear_tsk_need_resched(struct task_struct *tsk)
 {
 	clear_tsk_thread_flag(tsk, tif_resched(NR_now));
@@ -1972,7 +1977,7 @@ static inline void clear_tsk_need_resched(struct task_struct *tsk)
 		clear_tsk_thread_flag(tsk, tif_resched(NR_lazy));
 }

-static inline bool test_tsk_need_resched(struct task_struct *tsk, resched_t rs)
+static inline bool __test_tsk_need_resched(struct task_struct *tsk, resched_t rs)
 {
 	if (IS_ENABLED(CONFIG_PREEMPT_AUTO) || rs == NR_now)
 		return unlikely(test_tsk_thread_flag(tsk, tif_resched(rs)));
@@ -1980,6 +1985,11 @@ static inline bool test_tsk_need_resched(struct task_struct *tsk, resched_t rs)
 		return false;
 }

+static inline bool test_tsk_need_resched(struct task_struct *tsk, resched_t rs)
+{
+	return __test_tsk_need_resched(tsk, NR_now);
+}
+
 /*
  * cond_resched() and cond_resched_lock(): latency reduction via
  * explicit rescheduling in places that are safe. The return