| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 28 May 2024 18:09:05 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Ankur Arora <ankur.a.arora@...cle.com>
Cc: linux-kernel@...r.kernel.org, tglx@...utronix.de,
torvalds@...ux-foundation.org, paulmck@...nel.org,
rostedt@...dmis.org, mark.rutland@....com, juri.lelli@...hat.com,
joel@...lfernandes.org, raghavendra.kt@....com,
sshegde@...ux.ibm.com, boris.ostrovsky@...cle.com,
konrad.wilk@...cle.com, Ingo Molnar <mingo@...hat.com>,
Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH v2 07/35] sched: define *_tsk_need_resched_lazy() helpers
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@...radead.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Juri Lelli <juri.lelli@...hat.com>
> Cc: Vincent Guittot <vincent.guittot@...aro.org>
> Cc: Paul E. McKenney <paulmck@...nel.org>
> Originally-by: Thomas Gleixner <tglx@...utronix.de>
> Link: https://lore.kernel.org/lkml/87jzshhexi.ffs@tglx/
> Signed-off-by: Ankur Arora <ankur.a.arora@...cle.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.
Powered by blists - more mailing lists