Re: [PATCH v2 07/35] sched: define *_tsk_need_resched_lazy() helpers

Ankur Arora
2024-05-30T09:03:43Z

Peter Zijlstra <peterz@infradead.org> writes:

> On Mon, May 27, 2024 at 05:34:53PM -0700, Ankur Arora wrote:
>> Define __{set,test}_tsk_need_resched() to test for the immediacy of the
>> need-resched.
>>
>> The current helpers, {set,test}_tsk_need_resched(...) stay the same.
>>
>> In scheduler code, switch to the more explicit variants,
>> __set_tsk_need_resched(...), __test_tsk_need_resched(...).
>>
>> Note that clear_tsk_need_resched() is only used from __schedule()
>> to clear the flags before switching context. Now it clears all the
>> need-resched flags.
>>
>> Cc: Peter Ziljstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Juri Lelli <juri.lelli@redhat.com>
>> Cc: Vincent Guittot <vincent.guittot@linaro.org>
>> Cc: Paul E. McKenney <paulmck@kernel.org>
>> Originally-by: Thomas Gleixner <tglx@linutronix.de>
>> Link: https://lore.kernel.org/lkml/87jzshhexi.ffs@tglx/
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> ---
>> include/linux/sched.h | 45 +++++++++++++++++++++++++++++++++++++----
>> kernel/sched/core.c | 9 +++++----
>> kernel/sched/deadline.c | 4 ++--
>> kernel/sched/fair.c | 2 +-
>> kernel/sched/rt.c | 4 ++--
>> 5 files changed, 51 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 37a51115b691..804a76e6f3c5 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1952,19 +1952,56 @@ static inline bool test_tsk_thread_flag(struct task_struct *tsk, int flag)
>> return test_ti_thread_flag(task_thread_info(tsk), flag);
>> }
>>
>> -static inline void set_tsk_need_resched(struct task_struct *tsk)
>> +/*
>> + * With !CONFIG_PREEMPT_AUTO, tif_resched(RESCHED_LAZY) reduces to
>> + * tif_resched(RESCHED_NOW). Add a check in the helpers below to ensure
>> + * we don't touch the tif_reshed(RESCHED_NOW) bit unnecessarily.
>> + */
>> +static inline void __set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
>> {
>> - set_tsk_thread_flag(tsk,TIF_NEED_RESCHED);
>> + if (IS_ENABLED(CONFIG_PREEMPT_AUTO) || rs == RESCHED_NOW)
>> + set_tsk_thread_flag(tsk, tif_resched(rs));
>> + else
>> + /*
>> + * RESCHED_LAZY is only touched under CONFIG_PREEMPT_AUTO.
>> + */
>> + BUG();
>> }
>
> This straight up violates coding style and would require a dose of {}.
>
> if (!IS_ENABLED(CONFIG_PREEMPT_AUTO && rs == RESCHED_LAZY)
> BUG();
>
> set_tsk_thread_flag(tsk, tif_resched(rs));
>
> seems much saner to me.
>
>> static inline void clear_tsk_need_resched(struct task_struct *tsk)
>> {
>> - clear_tsk_thread_flag(tsk,TIF_NEED_RESCHED);
>> + clear_tsk_thread_flag(tsk, tif_resched(RESCHED_NOW));
>> +
>> + if (IS_ENABLED(CONFIG_PREEMPT_AUTO))
>> + clear_tsk_thread_flag(tsk, tif_resched(RESCHED_LAZY));
>> +}
>> +
>> +static inline bool __test_tsk_need_resched(struct task_struct *tsk, resched_t rs)
>> +{
>> + if (IS_ENABLED(CONFIG_PREEMPT_AUTO) || rs == RESCHED_NOW)
>> + return unlikely(test_tsk_thread_flag(tsk, tif_resched(rs)));
>> + else
>> + return false;
>> }
>
> if (!IS_ENABLED(CONFIG_PREEMPT_AUTO) && rs == RESCHED_LAZY)
> return false;
>
> return unlikely(test_tsk_thread_flag(tsk, tif_resched(rs)));
>
>>
>> static inline bool test_tsk_need_resched(struct task_struct *tsk)
>> {
>> - return unlikely(test_tsk_thread_flag(tsk,TIF_NEED_RESCHED));
>> + return __test_tsk_need_resched(tsk, RESCHED_NOW);
>> +}
>> +
>> +static inline bool test_tsk_need_resched_lazy(struct task_struct *tsk)
>> +{
>> + return __test_tsk_need_resched(tsk, RESCHED_LAZY);
>> +}
>> +
>> +static inline void set_tsk_need_resched(struct task_struct *tsk)
>> +{
>> + return __set_tsk_need_resched(tsk, RESCHED_NOW);
>> +}
>> +
>> +static inline void set_tsk_need_resched_lazy(struct task_struct *tsk)
>> +{
>> + return __set_tsk_need_resched(tsk, RESCHED_LAZY);
>> }
>>
>> /*
>
> So far so good, however:
>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 7019a40457a6..d00d7b45303e 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -933,7 +933,7 @@ static bool set_nr_if_polling(struct task_struct *p)
>> #else
>> static inline bool set_nr_and_not_polling(struct task_struct *p)
>> {
>> - set_tsk_need_resched(p);
>> + __set_tsk_need_resched(p, RESCHED_NOW);
>> return true;
>> }
>>
>> @@ -1045,13 +1045,13 @@ void resched_curr(struct rq *rq)
>>
>> lockdep_assert_rq_held(rq);
>>
>> - if (test_tsk_need_resched(curr))
>> + if (__test_tsk_need_resched(curr, RESCHED_NOW))
>> return;
>>
>> cpu = cpu_of(rq);
>>
>> if (cpu == smp_processor_id()) {
>> - set_tsk_need_resched(curr);
>> + __set_tsk_need_resched(curr, RESCHED_NOW);
>> set_preempt_need_resched();
>> return;
>> }
>> @@ -2245,7 +2245,8 @@ void wakeup_preempt(struct rq *rq, struct task_struct *p, int flags)
>> * A queue event has occurred, and we're going to schedule. In
>> * this case, we can save a useless back to back clock update.
>> */
>> - if (task_on_rq_queued(rq->curr) && test_tsk_need_resched(rq->curr))
>> + if (task_on_rq_queued(rq->curr) &&
>> + __test_tsk_need_resched(rq->curr, RESCHED_NOW))
>> rq_clock_skip_update(rq);
>> }
>>
>> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>> index a04a436af8cc..d24d6bfee293 100644
>> --- a/kernel/sched/deadline.c
>> +++ b/kernel/sched/deadline.c
>> @@ -2035,7 +2035,7 @@ static void wakeup_preempt_dl(struct rq *rq, struct task_struct *p,
>> * let us try to decide what's the best thing to do...
>> */
>> if ((p->dl.deadline == rq->curr->dl.deadline) &&
>> - !test_tsk_need_resched(rq->curr))
>> + !__test_tsk_need_resched(rq->curr, RESCHED_NOW))
>> check_preempt_equal_dl(rq, p);
>> #endif /* CONFIG_SMP */
>> }
>> @@ -2564,7 +2564,7 @@ static void pull_dl_task(struct rq *this_rq)
>> static void task_woken_dl(struct rq *rq, struct task_struct *p)
>> {
>> if (!task_on_cpu(rq, p) &&
>> - !test_tsk_need_resched(rq->curr) &&
>> + !__test_tsk_need_resched(rq->curr, RESCHED_NOW) &&
>> p->nr_cpus_allowed > 1 &&
>> dl_task(rq->curr) &&
>> (rq->curr->nr_cpus_allowed < 2 ||
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index c62805dbd608..c5171c247466 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -8316,7 +8316,7 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>> * prevents us from potentially nominating it as a false LAST_BUDDY
>> * below.
>> */
>> - if (test_tsk_need_resched(curr))
>> + if (__test_tsk_need_resched(curr, RESCHED_NOW))
>> return;
>>
>> /* Idle tasks are by definition preempted by non-idle tasks. */
>> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
>> index 3261b067b67e..f0a6c9bb890b 100644
>> --- a/kernel/sched/rt.c
>> +++ b/kernel/sched/rt.c
>> @@ -1680,7 +1680,7 @@ static void wakeup_preempt_rt(struct rq *rq, struct task_struct *p, int flags)
>> * to move current somewhere else, making room for our non-migratable
>> * task.
>> */
>> - if (p->prio == rq->curr->prio && !test_tsk_need_resched(rq->curr))
>> + if (p->prio == rq->curr->prio && !__test_tsk_need_resched(rq->curr, RESCHED_NOW))
>> check_preempt_equal_prio(rq, p);
>> #endif
>> }
>> @@ -2415,7 +2415,7 @@ static void pull_rt_task(struct rq *this_rq)
>> static void task_woken_rt(struct rq *rq, struct task_struct *p)
>> {
>> bool need_to_push = !task_on_cpu(rq, p) &&
>> - !test_tsk_need_resched(rq->curr) &&
>> + !__test_tsk_need_resched(rq->curr, RESCHED_NOW) &&
>> p->nr_cpus_allowed > 1 &&
>> (dl_task(rq->curr) || rt_task(rq->curr)) &&
>> (rq->curr->nr_cpus_allowed < 2 ||
>
> These are all NO-OPs.... Changelog says:
>
>> In scheduler code, switch to the more explicit variants,
>> __set_tsk_need_resched(...), __test_tsk_need_resched(...).
>
> But leaves me wondering *WHY* ?!?
>
> I can't help but feel this patch attempts to do 2 things and fails to
> justify at least one of them.

So, yes all of the scheduler changes are NOP. In later patches the
scheduler will care about specifying the specific type of resched_t
and so cannot use need_resched() or need_resched_lazy().

Changed that here to minimize the interface change noise later-on.

Does something like the following help justify why they should be here?

In scheduler code, switch to the more explicit variants,
__set_tsk_need_resched(...), __test_tsk_need_resched(...) as a
preparatory step for PREEMPT_AUTO support.

Thanks for all the comments, btw.

--
ankur