[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0d4418ab-d531-413c-bbbd-6c97ea320490@linux.ibm.com>
Date: Sat, 8 Feb 2025 13:31:27 +0530
From: Shrikanth Hegde <sshegde@...ux.ibm.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
linux-kernel@...r.kernel.org, linux-rt-devel@...ts.linux.dev
Cc: Ben Segall <bsegall@...gle.com>,
Catalin Marinas
<catalin.marinas@....com>,
Dietmar Eggemann <dietmar.eggemann@....com>,
Ingo Molnar <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>,
Mel Gorman <mgorman@...e.de>, Peter Zijlstra <peterz@...radead.org>,
Steven Rostedt <rostedt@...dmis.org>,
Thomas Gleixner <tglx@...utronix.de>,
Valentin Schneider <vschneid@...hat.com>,
Vincent Guittot <vincent.guittot@...aro.org>,
Will Deacon <will@...nel.org>
Subject: Re: [PATCH v2 1/9] sched: Add a generic function to return the
preemption string.
On 2/3/25 19:46, Sebastian Andrzej Siewior wrote:
> The individual architectures often add the preemption model to the begin
> of the backtrace. This is the case on X86 or ARM64 for the "die" case
> but not for regular warning. With the addition of DYNAMIC_PREEMPT for
> PREEMPT_RT we end up with CONFIG_PREEMPT and CONFIG_PREEMPT_RT set
> simultaneously. That means that everyone who tried to add that piece of
> information gets it wrong for PREEMPT_RT because PREEMPT is checked
> first.
>
> Provide a generic function which returns the current scheduling model
> considering LAZY preempt and the current state of PREEMPT_DYNAMIC.
> [ The dynamic building of the string can lead to an empty string if the
> function is invoked simultaneously on two CPUs. ]
>
> Co-developed-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Co-developed-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> ---
> include/linux/preempt.h | 2 ++
> kernel/sched/core.c | 47 +++++++++++++++++++++++++++++++++++++++++
> kernel/sched/debug.c | 10 +++++----
> kernel/sched/sched.h | 1 +
> 4 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/preempt.h b/include/linux/preempt.h
> index ca86235ac15c0..3e9808f2b5491 100644
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -515,6 +515,8 @@ static inline bool preempt_model_rt(void)
> return IS_ENABLED(CONFIG_PREEMPT_RT);
> }
>
> +extern const char *preempt_model_str(void);
> +
> /*
> * Does the preemption model allow non-cooperative preemption?
> *
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 165c90ba64ea9..bd2664755b09a 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7646,10 +7646,57 @@ PREEMPT_MODEL_ACCESSOR(lazy);
>
> #else /* !CONFIG_PREEMPT_DYNAMIC: */
>
> +#define preempt_dynamic_mode -1
> +
> static inline void preempt_dynamic_init(void) { }
>
> #endif /* CONFIG_PREEMPT_DYNAMIC */
>
> +const char *preempt_modes[] = {
> + "none", "voluntary", "full", "lazy", NULL,
> +};
> +
> +const char *preempt_model_str(void)
> +{
> + bool brace = IS_ENABLED(CONFIG_PREEMPT_RT) &&
> + (IS_ENABLED(CONFIG_PREEMPT_DYNAMIC) ||
> + IS_ENABLED(CONFIG_PREEMPT_LAZY));
> + static char buf[128];
> +
> + if (IS_ENABLED(CONFIG_PREEMPT_BUILD)) {
> + struct seq_buf s;
> +
> + seq_buf_init(&s, buf, 128);
> + seq_buf_puts(&s, "PREEMPT");
> +
> + if (IS_ENABLED(CONFIG_PREEMPT_RT))
> + seq_buf_printf(&s, "%sRT%s",
> + brace ? "_{" : "_",
> + brace ? "," : "");
> +
> + if (IS_ENABLED(CONFIG_PREEMPT_DYNAMIC)) {
> + seq_buf_printf(&s, "(%s)%s",
> + preempt_dynamic_mode > 0 ?
> + preempt_modes[preempt_dynamic_mode] : "undef",
> + brace ? "}" : "");
> + return seq_buf_str(&s);
> + }
> +
> + if (IS_ENABLED(CONFIG_PREEMPT_LAZY)) {
> + seq_buf_printf(&s, "LAZY%s",
> + brace ? "}" : "");
> + return seq_buf_str(&s);
> + }
> +
> + return seq_buf_str(&s);
> + }
> +
> + if (IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY_BUILD))
> + return "VOLUNTARY";
> +
> + return "NONE";
> +}
nit: This means when it preempt=full, it is going to show PREEMPT or
PREEMPT_RT. Maybe that could be put into changelog or add code similar
to lazy for full as well, so it is easier for user to know if it is in
preempt=full model. In all other models, there is a suffix like
PREEMPTLAZY. Dynamic modes looks good.
Reviewed-by: Shrikanth Hegde <sshegde@...ux.ibm.com>
> +
> int io_schedule_prepare(void)
> {
> int old_iowait = current->in_iowait;
> diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
> index fd7e852207157..755232ac88de9 100644
> --- a/kernel/sched/debug.c
> +++ b/kernel/sched/debug.c
> @@ -244,11 +244,13 @@ static ssize_t sched_dynamic_write(struct file *filp, const char __user *ubuf,
>
> static int sched_dynamic_show(struct seq_file *m, void *v)
> {
> - static const char * preempt_modes[] = {
> - "none", "voluntary", "full", "lazy",
> - };
> - int j = ARRAY_SIZE(preempt_modes) - !IS_ENABLED(CONFIG_ARCH_HAS_PREEMPT_LAZY);
> int i = IS_ENABLED(CONFIG_PREEMPT_RT) * 2;
> + int j;
> +
> + /* Count entries in NULL terminated preempt_modes */
> + for (j = 0; preempt_modes[j]; j++)
> + ;
> + j -= !IS_ENABLED(CONFIG_ARCH_HAS_PREEMPT_LAZY);
>
> for (; i < j; i++) {
> if (preempt_dynamic_mode == i)
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 38e0e323dda26..fb4486ceb978a 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3619,6 +3619,7 @@ extern int preempt_dynamic_mode;
> extern int sched_dynamic_mode(const char *str);
> extern void sched_dynamic_update(int mode);
> #endif
> +extern const char *preempt_modes[];
>
> #ifdef CONFIG_SCHED_MM_CID
>
Powered by blists - more mailing lists