[RFC PATCH] preempt: Add a generic function to return the preemption string.

Sebastian Andrzej Siewior posted 1 patch 1 year ago
There is a newer version of this series
arch/arm/kernel/traps.c      | 11 ++---------
arch/arm64/kernel/traps.c    | 12 ++----------
arch/powerpc/kernel/traps.c  |  4 ++--
arch/s390/kernel/dumpstack.c |  9 ++-------
arch/x86/kernel/dumpstack.c  |  7 +------
arch/xtensa/kernel/traps.c   |  6 +-----
include/linux/preempt.h      |  2 ++
kernel/sched/core.c          | 24 ++++++++++++++++++++++++
kernel/trace/trace.c         |  6 +-----
lib/dump_stack.c             |  4 ++--
10 files changed, 39 insertions(+), 46 deletions(-)
[RFC PATCH] preempt: Add a generic function to return the preemption string.
Posted by Sebastian Andrzej Siewior 1 year ago
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.
This is an attempt to cover all users that I identified so far with a
generic function provided by the scheduler. While at it, extend it with
the LAZY information and add it to dump_stack_print_info().

Comments?

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm/kernel/traps.c      | 11 ++---------
 arch/arm64/kernel/traps.c    | 12 ++----------
 arch/powerpc/kernel/traps.c  |  4 ++--
 arch/s390/kernel/dumpstack.c |  9 ++-------
 arch/x86/kernel/dumpstack.c  |  7 +------
 arch/xtensa/kernel/traps.c   |  6 +-----
 include/linux/preempt.h      |  2 ++
 kernel/sched/core.c          | 24 ++++++++++++++++++++++++
 kernel/trace/trace.c         |  6 +-----
 lib/dump_stack.c             |  4 ++--
 10 files changed, 39 insertions(+), 46 deletions(-)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 6ea645939573f..1254992184d2e 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -258,13 +258,6 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl)
 	barrier();
 }
 
-#ifdef CONFIG_PREEMPT
-#define S_PREEMPT " PREEMPT"
-#elif defined(CONFIG_PREEMPT_RT)
-#define S_PREEMPT " PREEMPT_RT"
-#else
-#define S_PREEMPT ""
-#endif
 #ifdef CONFIG_SMP
 #define S_SMP " SMP"
 #else
@@ -282,8 +275,8 @@ static int __die(const char *str, int err, struct pt_regs *regs)
 	static int die_counter;
 	int ret;
 
-	pr_emerg("Internal error: %s: %x [#%d]" S_PREEMPT S_SMP S_ISA "\n",
-	         str, err, ++die_counter);
+	pr_emerg("Internal error: %s: %x [#%d] %s" S_SMP S_ISA "\n",
+		 str, err, ++die_counter, preempt_model_str());
 
 	/* trap and error numbers are mostly meaningless on ARM */
 	ret = notify_die(DIE_OOPS, str, regs, err, tsk->thread.trap_no, SIGSEGV);
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 4e26bd356a482..0b6f92fcdb304 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -172,14 +172,6 @@ static void dump_kernel_instr(const char *lvl, struct pt_regs *regs)
 	printk("%sCode: %s\n", lvl, str);
 }
 
-#ifdef CONFIG_PREEMPT
-#define S_PREEMPT " PREEMPT"
-#elif defined(CONFIG_PREEMPT_RT)
-#define S_PREEMPT " PREEMPT_RT"
-#else
-#define S_PREEMPT ""
-#endif
-
 #define S_SMP " SMP"
 
 static int __die(const char *str, long err, struct pt_regs *regs)
@@ -187,8 +179,8 @@ static int __die(const char *str, long err, struct pt_regs *regs)
 	static int die_counter;
 	int ret;
 
-	pr_emerg("Internal error: %s: %016lx [#%d]" S_PREEMPT S_SMP "\n",
-		 str, err, ++die_counter);
+	pr_emerg("Internal error: %s: %016lx [#%d] %s" S_SMP "\n",
+		 str, err, ++die_counter, preempt_model_str());
 
 	/* trap and error numbers are mostly meaningless on ARM */
 	ret = notify_die(DIE_OOPS, str, regs, err, 0, SIGSEGV);
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index edf5cabe5dfdb..d6d77d92b3358 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -263,10 +263,10 @@ static int __die(const char *str, struct pt_regs *regs, long err)
 {
 	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
 
-	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
+	printk("%s PAGE_SIZE=%luK%s %s %s%s%s%s %s\n",
 	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
 	       PAGE_SIZE / 1024, get_mmu_str(),
-	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
+	       preempt_model_str(),
 	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
 	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
 	       debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",
diff --git a/arch/s390/kernel/dumpstack.c b/arch/s390/kernel/dumpstack.c
index 1ecd0580561f6..7930fbab69dbb 100644
--- a/arch/s390/kernel/dumpstack.c
+++ b/arch/s390/kernel/dumpstack.c
@@ -198,13 +198,8 @@ void __noreturn die(struct pt_regs *regs, const char *str)
 	console_verbose();
 	spin_lock_irq(&die_lock);
 	bust_spinlocks(1);
-	printk("%s: %04x ilc:%d [#%d] ", str, regs->int_code & 0xffff,
-	       regs->int_code >> 17, ++die_counter);
-#ifdef CONFIG_PREEMPT
-	pr_cont("PREEMPT ");
-#elif defined(CONFIG_PREEMPT_RT)
-	pr_cont("PREEMPT_RT ");
-#endif
+	printk("%s: %04x ilc:%d [#%d] %s", str, regs->int_code & 0xffff,
+	       regs->int_code >> 17, ++die_counter, preempt_model_str());
 	pr_cont("SMP ");
 	if (debug_pagealloc_enabled())
 		pr_cont("DEBUG_PAGEALLOC");
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index a7d562697e50e..064b23a93c6fe 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -395,18 +395,13 @@ NOKPROBE_SYMBOL(oops_end);
 
 static void __die_header(const char *str, struct pt_regs *regs, long err)
 {
-	const char *pr = "";
-
 	/* Save the regs of the first oops for the executive summary later. */
 	if (!die_counter)
 		exec_summary_regs = *regs;
 
-	if (IS_ENABLED(CONFIG_PREEMPTION))
-		pr = IS_ENABLED(CONFIG_PREEMPT_RT) ? " PREEMPT_RT" : " PREEMPT";
-
 	printk(KERN_DEFAULT
 	       "Oops: %s: %04lx [#%d]%s%s%s%s%s\n", str, err & 0xffff,
-	       ++die_counter, pr,
+	       ++die_counter, preempt_model_str(),
 	       IS_ENABLED(CONFIG_SMP)     ? " SMP"             : "",
 	       debug_pagealloc_enabled()  ? " DEBUG_PAGEALLOC" : "",
 	       IS_ENABLED(CONFIG_KASAN)   ? " KASAN"           : "",
diff --git a/arch/xtensa/kernel/traps.c b/arch/xtensa/kernel/traps.c
index 38092d21acf8e..0edba7d8df8c7 100644
--- a/arch/xtensa/kernel/traps.c
+++ b/arch/xtensa/kernel/traps.c
@@ -629,15 +629,11 @@ DEFINE_SPINLOCK(die_lock);
 void __noreturn die(const char * str, struct pt_regs * regs, long err)
 {
 	static int die_counter;
-	const char *pr = "";
-
-	if (IS_ENABLED(CONFIG_PREEMPTION))
-		pr = IS_ENABLED(CONFIG_PREEMPT_RT) ? " PREEMPT_RT" : " PREEMPT";
 
 	console_verbose();
 	spin_lock_irq(&die_lock);
 
-	pr_info("%s: sig: %ld [#%d]%s\n", str, err, ++die_counter, pr);
+	pr_info("%s: sig: %ld [#%d]%s\n", str, err, ++die_counter, preempt_model_str());
 	show_regs(regs);
 	if (!user_mode(regs))
 		show_stack(NULL, (unsigned long *)regs->areg[1], KERN_INFO);
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 95e40895a5190..8f5517dbe07d4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7642,6 +7642,30 @@ static inline void preempt_dynamic_init(void) { }
 
 #endif /* CONFIG_PREEMPT_DYNAMIC */
 
+const char *preempt_model_str(void)
+{
+	if (IS_ENABLED(CONFIG_ARCH_HAS_PREEMPT_LAZY) && preempt_model_lazy()) {
+		if (preempt_model_rt())
+			return "PREEMPT_RT+LAZY";
+		if (preempt_model_full())
+			return "PREEMPT+LAZY";
+		if (preempt_model_voluntary())
+			return "VOLUNTARY+LAZY";
+		if (preempt_model_none())
+			return "NONE+LAZY";
+	} else {
+		if (preempt_model_rt())
+			return "PREEMPT_RT";
+		if (preempt_model_full())
+			return "PREEMPT";
+		if (preempt_model_voluntary())
+			return "VOLUNTARY";
+		if (preempt_model_none())
+			return "NONE";
+	}
+	return "UNKNOWN-PREEMPT";
+}
+
 int io_schedule_prepare(void)
 {
 	int old_iowait = current->in_iowait;
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index be62f0ea1814d..3861f53f9a434 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4266,11 +4266,7 @@ print_trace_header(struct seq_file *m, struct trace_iterator *iter)
 		   entries,
 		   total,
 		   buf->cpu,
-		   preempt_model_none()      ? "server" :
-		   preempt_model_voluntary() ? "desktop" :
-		   preempt_model_full()      ? "preempt" :
-		   preempt_model_rt()        ? "preempt_rt" :
-		   "unknown",
+		   preempt_model_str(),
 		   /* These are reserved for later use */
 		   0, 0, 0, 0);
 #ifdef CONFIG_SMP
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index 388da1aea14a5..c3e59f8992279 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -54,7 +54,7 @@ void __init dump_stack_set_arch_desc(const char *fmt, ...)
  */
 void dump_stack_print_info(const char *log_lvl)
 {
-	printk("%sCPU: %d UID: %u PID: %d Comm: %.20s %s%s %s %.*s" BUILD_ID_FMT "\n",
+	printk("%sCPU: %d UID: %u PID: %d Comm: %.20s %s%s %s %.*s %s" BUILD_ID_FMT "\n",
 	       log_lvl, raw_smp_processor_id(),
 	       __kuid_val(current_real_cred()->euid),
 	       current->pid, current->comm,
@@ -62,7 +62,7 @@ void dump_stack_print_info(const char *log_lvl)
 	       print_tainted(),
 	       init_utsname()->release,
 	       (int)strcspn(init_utsname()->version, " "),
-	       init_utsname()->version, BUILD_ID_VAL);
+	       init_utsname()->version, preempt_model_str(), BUILD_ID_VAL);
 
 	if (get_taint())
 		printk("%s%s\n", log_lvl, print_tainted_verbose());
-- 
2.45.2
Re: [RFC PATCH] preempt: Add a generic function to return the preemption string.
Posted by Peter Zijlstra 11 months, 1 week ago
On Fri, Dec 06, 2024 at 12:34:31PM +0100, Sebastian Andrzej Siewior wrote:
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 95e40895a5190..8f5517dbe07d4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7642,6 +7642,30 @@ static inline void preempt_dynamic_init(void) { }
>  
>  #endif /* CONFIG_PREEMPT_DYNAMIC */
>  
> +const char *preempt_model_str(void)
> +{
> +	if (IS_ENABLED(CONFIG_ARCH_HAS_PREEMPT_LAZY) && preempt_model_lazy()) {
> +		if (preempt_model_rt())
> +			return "PREEMPT_RT+LAZY";
> +		if (preempt_model_full())
> +			return "PREEMPT+LAZY";
> +		if (preempt_model_voluntary())
> +			return "VOLUNTARY+LAZY";
> +		if (preempt_model_none())
> +			return "NONE+LAZY";
> +	} else {
> +		if (preempt_model_rt())
> +			return "PREEMPT_RT";
> +		if (preempt_model_full())
> +			return "PREEMPT";
> +		if (preempt_model_voluntary())
> +			return "VOLUNTARY";
> +		if (preempt_model_none())
> +			return "NONE";
> +	}
> +	return "UNKNOWN-PREEMPT";
> +}

Hurmph.. a bunch of those combinations are nonsense :/

Also, we have a string thing in sched_dynamic_show().

Can't we do something like:

const char *preempt_model_str(void)
{
	static char buf[128];
	size_t off = 0, len = sizeof(buf), r;
	bool brace = IS_ENABLED(CONFIG_PREEMPT_RT) &&
		     (IS_ENABLED(CONFIG_PREEMPT_DYNAMIC) ||
		      IS_ENABLED(CONFIG_PREEMPT_LAZY));

	if (IS_ENABLED(CONFIG_PREEMPT_BUILD)) {
		r = snprintf(buf + off, len, "PREEMPT");
		off += r;
		len -= r;

		if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
			r = snprintf(buf + off, len, "%sRT%s", 
					brace ? "_{" : "_",
					brace ? "," : "");
			off += r;
			len -= r;
		}

		if (IS_ENABLED(CONFIG_PREEMPT_DYNAMIC)) {
			snprintf(buf + off, len, "(%s)%s", 
				preempt_modes[preempt_dynamic_mode],
				brace ? "}" : "");
			retun buf;
		}

		if (IS_ENABLED(CONFIG_PREEMPT_LAZY)) {
			snprintf(buf + off, len, "LAZY%s", 
				brace ? "}" : "");
			retun buf;
		}

		return buf;
	}

	if (IS_ENABLED(CONFIG_VOLUNTARY_BUILD))
		return "VOLUNTARY";

	return "NONE";
}
Re: [RFC PATCH] preempt: Add a generic function to return the preemption string.
Posted by Steven Rostedt 11 months, 1 week ago
On Thu, 9 Jan 2025 12:43:39 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> Hurmph.. a bunch of those combinations are nonsense :/
> 
> Also, we have a string thing in sched_dynamic_show().
> 
> Can't we do something like:
> 
> const char *preempt_model_str(void)
> {
> 	static char buf[128];
> 	size_t off = 0, len = sizeof(buf), r;
> 	bool brace = IS_ENABLED(CONFIG_PREEMPT_RT) &&
> 		     (IS_ENABLED(CONFIG_PREEMPT_DYNAMIC) ||
> 		      IS_ENABLED(CONFIG_PREEMPT_LAZY));
> 
> 	if (IS_ENABLED(CONFIG_PREEMPT_BUILD)) {
> 		r = snprintf(buf + off, len, "PREEMPT");
> 		off += r;
> 		len -= r;
> 
> 		if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> 			r = snprintf(buf + off, len, "%sRT%s", 
> 					brace ? "_{" : "_",
> 					brace ? "," : "");
> 			off += r;
> 			len -= r;
> 		}
> 
> 		if (IS_ENABLED(CONFIG_PREEMPT_DYNAMIC)) {
> 			snprintf(buf + off, len, "(%s)%s", 
> 				preempt_modes[preempt_dynamic_mode],
> 				brace ? "}" : "");
> 			retun buf;
> 		}
> 
> 		if (IS_ENABLED(CONFIG_PREEMPT_LAZY)) {
> 			snprintf(buf + off, len, "LAZY%s", 
> 				brace ? "}" : "");
> 			retun buf;
> 		}

I'm sorry, but I can't even tell what the above is doing without my brain
hurting. Why make code that was easy to read into a cryptic obfuscation? I
can't see this as an optimization as IS_ENABLED() is determined at compile
time.

-- Steve


> 
> 		return buf;
> 	}
> 
> 	if (IS_ENABLED(CONFIG_VOLUNTARY_BUILD))
> 		return "VOLUNTARY";
> 
> 	return "NONE";
> }
Re: [RFC PATCH] preempt: Add a generic function to return the preemption string.
Posted by Peter Zijlstra 11 months, 1 week ago
On Thu, Jan 09, 2025 at 09:37:02AM -0500, Steven Rostedt wrote:
> On Thu, 9 Jan 2025 12:43:39 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Hurmph.. a bunch of those combinations are nonsense :/
> > 
> > Also, we have a string thing in sched_dynamic_show().
> > 
> > Can't we do something like:
> > 
> > const char *preempt_model_str(void)
> > {
> > 	static char buf[128];
> > 	size_t off = 0, len = sizeof(buf), r;
> > 	bool brace = IS_ENABLED(CONFIG_PREEMPT_RT) &&
> > 		     (IS_ENABLED(CONFIG_PREEMPT_DYNAMIC) ||
> > 		      IS_ENABLED(CONFIG_PREEMPT_LAZY));
> > 
> > 	if (IS_ENABLED(CONFIG_PREEMPT_BUILD)) {
> > 		r = snprintf(buf + off, len, "PREEMPT");
> > 		off += r;
> > 		len -= r;
> > 
> > 		if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> > 			r = snprintf(buf + off, len, "%sRT%s", 
> > 					brace ? "_{" : "_",
> > 					brace ? "," : "");
> > 			off += r;
> > 			len -= r;
> > 		}
> > 
> > 		if (IS_ENABLED(CONFIG_PREEMPT_DYNAMIC)) {
> > 			snprintf(buf + off, len, "(%s)%s", 
> > 				preempt_modes[preempt_dynamic_mode],
> > 				brace ? "}" : "");
> > 			retun buf;
> > 		}
> > 
> > 		if (IS_ENABLED(CONFIG_PREEMPT_LAZY)) {
> > 			snprintf(buf + off, len, "LAZY%s", 
> > 				brace ? "}" : "");
> > 			retun buf;
> > 		}
> 
> I'm sorry, but I can't even tell what the above is doing without my brain
> hurting. Why make code that was easy to read into a cryptic obfuscation? I
> can't see this as an optimization as IS_ENABLED() is determined at compile
> time.

Upgrade brain. Also the proposed thing was just plain wrong.
Re: [RFC PATCH] preempt: Add a generic function to return the preemption string.
Posted by Steven Rostedt 11 months, 1 week ago
On Thu, 9 Jan 2025 15:37:01 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> > I'm sorry, but I can't even tell what the above is doing without my brain
> > hurting. Why make code that was easy to read into a cryptic obfuscation? I
> > can't see this as an optimization as IS_ENABLED() is determined at compile
> > time.  
> 
> Upgrade brain. Also the proposed thing was just plain wrong.

Or use seq_buf to make it a little more readable again:

const char *preempt_model_str(void)
{
	static char buf[128];
	bool brace = IS_ENABLED(CONFIG_PREEMPT_RT) &&
		     (IS_ENABLED(CONFIG_PREEMPT_DYNAMIC) ||
		      IS_ENABLED(CONFIG_PREEMPT_LAZY));

	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_modes[preempt_dynamic_mode],
				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_VOLUNTARY_BUILD))
		return "VOLUNTARY";

	return "NONE";
}


At least this removes the need for keeping track of off, len and r.

-- Steve
Re: [RFC PATCH] preempt: Add a generic function to return the preemption string.
Posted by Sebastian Andrzej Siewior 11 months, 1 week ago
On 2025-01-09 15:37:01 [+0100], Peter Zijlstra wrote:
> Upgrade brain. Also the proposed thing was just plain wrong.

Was it? For the !DYNAMIC case everything should be optimized away except
for the current case.
Let me try your thing to see what is different…

Sebastian
Re: [RFC PATCH] preempt: Add a generic function to return the preemption string.
Posted by Peter Zijlstra 11 months, 1 week ago
On Thu, Jan 09, 2025 at 03:41:24PM +0100, Sebastian Andrzej Siewior wrote:
> On 2025-01-09 15:37:01 [+0100], Peter Zijlstra wrote:
> > Upgrade brain. Also the proposed thing was just plain wrong.
> 
> Was it?

Yes, things like:

> +             if (preempt_model_voluntary())
> +                     return "VOLUNTARY+LAZY";
> +             if (preempt_model_none())
> +                     return "NONE+LAZY";

don't exist and make no sense.

> For the !DYNAMIC case everything should be optimized away except
> for the current case.
> Let me try your thing to see what is different…

For the various cases it was supposed to print something like so:

RT+DYN:	 PREEMPT_{RT,(dyn_mode)}
RT+LAZY: PREEMPT_{RT,LAZY}
RT+FULL: PREEMPT_RT

DYN:       PREEMPT_(dyn_mode)
FULL:      PREEMPT
LAZY:      PREEMPT_LAZY
VOLUNTARY: VOLUNTARY
NONE:      NONE

Re: [RFC PATCH] preempt: Add a generic function to return the preemption string.
Posted by Sebastian Andrzej Siewior 11 months, 1 week ago
On 2025-01-09 15:54:08 [+0100], Peter Zijlstra wrote:
> On Thu, Jan 09, 2025 at 03:41:24PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2025-01-09 15:37:01 [+0100], Peter Zijlstra wrote:
> > > Upgrade brain. Also the proposed thing was just plain wrong.
> > 
> > Was it?
> 
> Yes, things like:
> 
> > +             if (preempt_model_voluntary())
> > +                     return "VOLUNTARY+LAZY";
> > +             if (preempt_model_none())
> > +                     return "NONE+LAZY";
> 
> don't exist and make no sense.

point taken.

> > For the !DYNAMIC case everything should be optimized away except
> > for the current case.
> > Let me try your thing to see what is different…
> 
> For the various cases it was supposed to print something like so:
> 
> RT+DYN:	 PREEMPT_{RT,(dyn_mode)}
> RT+LAZY: PREEMPT_{RT,LAZY}
> RT+FULL: PREEMPT_RT
> 
> DYN:       PREEMPT_(dyn_mode)
> FULL:      PREEMPT
> LAZY:      PREEMPT_LAZY
> VOLUNTARY: VOLUNTARY
> NONE:      NONE

Good. I do like it. Let me make a patch.

Sebastian
Re: [RFC PATCH] preempt: Add a generic function to return the preemption string.
Posted by Sebastian Andrzej Siewior 11 months, 1 week ago
On 2025-01-09 16:27:33 [+0100], To Peter Zijlstra wrote:
> Good. I do like it. Let me make a patch.

I have this now for the sched bits. You wanted to reuse preempt_modes:

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 3e5a6bf587f91..ff460c309d27a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7638,10 +7638,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";
+}
+
 int io_schedule_prepare(void)
 {
 	int old_iowait = current->in_iowait;
diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index a1be00a988bf6..496a766e50e5e 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 c5d67a43fe524..cf52a650ada68 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -3587,6 +3587,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
 
Sebastian
Re: [RFC PATCH] preempt: Add a generic function to return the preemption string.
Posted by Sebastian Andrzej Siewior 11 months, 1 week ago
On 2024-12-06 12:34:31 [+0100], To linux-kernel@vger.kernel.org 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.
> This is an attempt to cover all users that I identified so far with a
> generic function provided by the scheduler. While at it, extend it with
> the LAZY information and add it to dump_stack_print_info().
> 
> Comments?

any comments on the approach or should I just repost it without the RFC?

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/arm/kernel/traps.c      | 11 ++---------
>  arch/arm64/kernel/traps.c    | 12 ++----------
>  arch/powerpc/kernel/traps.c  |  4 ++--
>  arch/s390/kernel/dumpstack.c |  9 ++-------
>  arch/x86/kernel/dumpstack.c  |  7 +------
>  arch/xtensa/kernel/traps.c   |  6 +-----
>  include/linux/preempt.h      |  2 ++
>  kernel/sched/core.c          | 24 ++++++++++++++++++++++++
>  kernel/trace/trace.c         |  6 +-----
>  lib/dump_stack.c             |  4 ++--
>  10 files changed, 39 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 6ea645939573f..1254992184d2e 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -258,13 +258,6 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl)
>  	barrier();
>  }
>  
> -#ifdef CONFIG_PREEMPT
> -#define S_PREEMPT " PREEMPT"
> -#elif defined(CONFIG_PREEMPT_RT)
> -#define S_PREEMPT " PREEMPT_RT"
> -#else
> -#define S_PREEMPT ""
> -#endif
>  #ifdef CONFIG_SMP
>  #define S_SMP " SMP"
>  #else
> @@ -282,8 +275,8 @@ static int __die(const char *str, int err, struct pt_regs *regs)
>  	static int die_counter;
>  	int ret;
>  
> -	pr_emerg("Internal error: %s: %x [#%d]" S_PREEMPT S_SMP S_ISA "\n",
> -	         str, err, ++die_counter);
> +	pr_emerg("Internal error: %s: %x [#%d] %s" S_SMP S_ISA "\n",
> +		 str, err, ++die_counter, preempt_model_str());
>  
>  	/* trap and error numbers are mostly meaningless on ARM */
>  	ret = notify_die(DIE_OOPS, str, regs, err, tsk->thread.trap_no, SIGSEGV);
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 4e26bd356a482..0b6f92fcdb304 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -172,14 +172,6 @@ static void dump_kernel_instr(const char *lvl, struct pt_regs *regs)
>  	printk("%sCode: %s\n", lvl, str);
>  }
>  
> -#ifdef CONFIG_PREEMPT
> -#define S_PREEMPT " PREEMPT"
> -#elif defined(CONFIG_PREEMPT_RT)
> -#define S_PREEMPT " PREEMPT_RT"
> -#else
> -#define S_PREEMPT ""
> -#endif
> -
>  #define S_SMP " SMP"
>  
>  static int __die(const char *str, long err, struct pt_regs *regs)
> @@ -187,8 +179,8 @@ static int __die(const char *str, long err, struct pt_regs *regs)
>  	static int die_counter;
>  	int ret;
>  
> -	pr_emerg("Internal error: %s: %016lx [#%d]" S_PREEMPT S_SMP "\n",
> -		 str, err, ++die_counter);
> +	pr_emerg("Internal error: %s: %016lx [#%d] %s" S_SMP "\n",
> +		 str, err, ++die_counter, preempt_model_str());
>  
>  	/* trap and error numbers are mostly meaningless on ARM */
>  	ret = notify_die(DIE_OOPS, str, regs, err, 0, SIGSEGV);
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index edf5cabe5dfdb..d6d77d92b3358 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -263,10 +263,10 @@ static int __die(const char *str, struct pt_regs *regs, long err)
>  {
>  	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
>  
> -	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
> +	printk("%s PAGE_SIZE=%luK%s %s %s%s%s%s %s\n",
>  	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
>  	       PAGE_SIZE / 1024, get_mmu_str(),
> -	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
> +	       preempt_model_str(),
>  	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>  	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
>  	       debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",
> diff --git a/arch/s390/kernel/dumpstack.c b/arch/s390/kernel/dumpstack.c
> index 1ecd0580561f6..7930fbab69dbb 100644
> --- a/arch/s390/kernel/dumpstack.c
> +++ b/arch/s390/kernel/dumpstack.c
> @@ -198,13 +198,8 @@ void __noreturn die(struct pt_regs *regs, const char *str)
>  	console_verbose();
>  	spin_lock_irq(&die_lock);
>  	bust_spinlocks(1);
> -	printk("%s: %04x ilc:%d [#%d] ", str, regs->int_code & 0xffff,
> -	       regs->int_code >> 17, ++die_counter);
> -#ifdef CONFIG_PREEMPT
> -	pr_cont("PREEMPT ");
> -#elif defined(CONFIG_PREEMPT_RT)
> -	pr_cont("PREEMPT_RT ");
> -#endif
> +	printk("%s: %04x ilc:%d [#%d] %s", str, regs->int_code & 0xffff,
> +	       regs->int_code >> 17, ++die_counter, preempt_model_str());
>  	pr_cont("SMP ");
>  	if (debug_pagealloc_enabled())
>  		pr_cont("DEBUG_PAGEALLOC");
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index a7d562697e50e..064b23a93c6fe 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -395,18 +395,13 @@ NOKPROBE_SYMBOL(oops_end);
>  
>  static void __die_header(const char *str, struct pt_regs *regs, long err)
>  {
> -	const char *pr = "";
> -
>  	/* Save the regs of the first oops for the executive summary later. */
>  	if (!die_counter)
>  		exec_summary_regs = *regs;
>  
> -	if (IS_ENABLED(CONFIG_PREEMPTION))
> -		pr = IS_ENABLED(CONFIG_PREEMPT_RT) ? " PREEMPT_RT" : " PREEMPT";
> -
>  	printk(KERN_DEFAULT
>  	       "Oops: %s: %04lx [#%d]%s%s%s%s%s\n", str, err & 0xffff,
> -	       ++die_counter, pr,
> +	       ++die_counter, preempt_model_str(),
>  	       IS_ENABLED(CONFIG_SMP)     ? " SMP"             : "",
>  	       debug_pagealloc_enabled()  ? " DEBUG_PAGEALLOC" : "",
>  	       IS_ENABLED(CONFIG_KASAN)   ? " KASAN"           : "",
> diff --git a/arch/xtensa/kernel/traps.c b/arch/xtensa/kernel/traps.c
> index 38092d21acf8e..0edba7d8df8c7 100644
> --- a/arch/xtensa/kernel/traps.c
> +++ b/arch/xtensa/kernel/traps.c
> @@ -629,15 +629,11 @@ DEFINE_SPINLOCK(die_lock);
>  void __noreturn die(const char * str, struct pt_regs * regs, long err)
>  {
>  	static int die_counter;
> -	const char *pr = "";
> -
> -	if (IS_ENABLED(CONFIG_PREEMPTION))
> -		pr = IS_ENABLED(CONFIG_PREEMPT_RT) ? " PREEMPT_RT" : " PREEMPT";
>  
>  	console_verbose();
>  	spin_lock_irq(&die_lock);
>  
> -	pr_info("%s: sig: %ld [#%d]%s\n", str, err, ++die_counter, pr);
> +	pr_info("%s: sig: %ld [#%d]%s\n", str, err, ++die_counter, preempt_model_str());
>  	show_regs(regs);
>  	if (!user_mode(regs))
>  		show_stack(NULL, (unsigned long *)regs->areg[1], KERN_INFO);
> 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 95e40895a5190..8f5517dbe07d4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7642,6 +7642,30 @@ static inline void preempt_dynamic_init(void) { }
>  
>  #endif /* CONFIG_PREEMPT_DYNAMIC */
>  
> +const char *preempt_model_str(void)
> +{
> +	if (IS_ENABLED(CONFIG_ARCH_HAS_PREEMPT_LAZY) && preempt_model_lazy()) {
> +		if (preempt_model_rt())
> +			return "PREEMPT_RT+LAZY";
> +		if (preempt_model_full())
> +			return "PREEMPT+LAZY";
> +		if (preempt_model_voluntary())
> +			return "VOLUNTARY+LAZY";
> +		if (preempt_model_none())
> +			return "NONE+LAZY";
> +	} else {
> +		if (preempt_model_rt())
> +			return "PREEMPT_RT";
> +		if (preempt_model_full())
> +			return "PREEMPT";
> +		if (preempt_model_voluntary())
> +			return "VOLUNTARY";
> +		if (preempt_model_none())
> +			return "NONE";
> +	}
> +	return "UNKNOWN-PREEMPT";
> +}
> +
>  int io_schedule_prepare(void)
>  {
>  	int old_iowait = current->in_iowait;
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index be62f0ea1814d..3861f53f9a434 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4266,11 +4266,7 @@ print_trace_header(struct seq_file *m, struct trace_iterator *iter)
>  		   entries,
>  		   total,
>  		   buf->cpu,
> -		   preempt_model_none()      ? "server" :
> -		   preempt_model_voluntary() ? "desktop" :
> -		   preempt_model_full()      ? "preempt" :
> -		   preempt_model_rt()        ? "preempt_rt" :
> -		   "unknown",
> +		   preempt_model_str(),
>  		   /* These are reserved for later use */
>  		   0, 0, 0, 0);
>  #ifdef CONFIG_SMP
> diff --git a/lib/dump_stack.c b/lib/dump_stack.c
> index 388da1aea14a5..c3e59f8992279 100644
> --- a/lib/dump_stack.c
> +++ b/lib/dump_stack.c
> @@ -54,7 +54,7 @@ void __init dump_stack_set_arch_desc(const char *fmt, ...)
>   */
>  void dump_stack_print_info(const char *log_lvl)
>  {
> -	printk("%sCPU: %d UID: %u PID: %d Comm: %.20s %s%s %s %.*s" BUILD_ID_FMT "\n",
> +	printk("%sCPU: %d UID: %u PID: %d Comm: %.20s %s%s %s %.*s %s" BUILD_ID_FMT "\n",
>  	       log_lvl, raw_smp_processor_id(),
>  	       __kuid_val(current_real_cred()->euid),
>  	       current->pid, current->comm,
> @@ -62,7 +62,7 @@ void dump_stack_print_info(const char *log_lvl)
>  	       print_tainted(),
>  	       init_utsname()->release,
>  	       (int)strcspn(init_utsname()->version, " "),
> -	       init_utsname()->version, BUILD_ID_VAL);
> +	       init_utsname()->version, preempt_model_str(), BUILD_ID_VAL);
>  
>  	if (get_taint())
>  		printk("%s%s\n", log_lvl, print_tainted_verbose());
> -- 
> 2.45.2
> 
>