This allows the reuse of the UD2 based 'struct bug_entry' low-overhead
_BUG_FLAGS() implementation and string-printing backend, without
having to add a new field.
An example:
If we have the following WARN_ON_ONCE() in kernel/sched/core.c:
WARN_ON_ONCE(idx < 0 && ptr);
Then previously _BUG_FLAGS() would store this string in bug_entry::file:
"kernel/sched/core.c"
After this patch, it would store and print:
"[idx < 0 && ptr] kernel/sched/core.c"
Which is an extended string that will be printed in warnings.
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/include/asm/bug.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index aff1c6b7a7f3..e966199c8ef7 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -50,7 +50,7 @@ do { \
"\t.org 2b+%c3\n" \
".popsection\n" \
extra \
- : : "i" (__FILE__), "i" (__LINE__), \
+ : : "i" (cond_str __FILE__), "i" (__LINE__), \
"i" (flags), \
"i" (sizeof(struct bug_entry))); \
} while (0)
--
2.45.2On Wed, Mar 26, 2025 at 09:47:49AM +0100, Ingo Molnar wrote:
> This allows the reuse of the UD2 based 'struct bug_entry' low-overhead
> _BUG_FLAGS() implementation and string-printing backend, without
> having to add a new field.
>
> An example:
>
> If we have the following WARN_ON_ONCE() in kernel/sched/core.c:
>
> WARN_ON_ONCE(idx < 0 && ptr);
>
> Then previously _BUG_FLAGS() would store this string in bug_entry::file:
>
> "kernel/sched/core.c"
>
> After this patch, it would store and print:
>
> "[idx < 0 && ptr] kernel/sched/core.c"
>
> Which is an extended string that will be printed in warnings.
>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
> arch/x86/include/asm/bug.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
> index aff1c6b7a7f3..e966199c8ef7 100644
> --- a/arch/x86/include/asm/bug.h
> +++ b/arch/x86/include/asm/bug.h
> @@ -50,7 +50,7 @@ do { \
> "\t.org 2b+%c3\n" \
> ".popsection\n" \
> extra \
> - : : "i" (__FILE__), "i" (__LINE__), \
> + : : "i" (cond_str __FILE__), "i" (__LINE__), \
> "i" (flags), \
> "i" (sizeof(struct bug_entry))); \
> } while (0)
Sneaky :-) Do we want to touch up all the other archs? I mean, you
already touched them anyway earlier in the series in order to push this
argument through.
* Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 26, 2025 at 09:47:49AM +0100, Ingo Molnar wrote:
> > This allows the reuse of the UD2 based 'struct bug_entry' low-overhead
> > _BUG_FLAGS() implementation and string-printing backend, without
> > having to add a new field.
> >
> > An example:
> >
> > If we have the following WARN_ON_ONCE() in kernel/sched/core.c:
> >
> > WARN_ON_ONCE(idx < 0 && ptr);
> >
> > Then previously _BUG_FLAGS() would store this string in bug_entry::file:
> >
> > "kernel/sched/core.c"
> >
> > After this patch, it would store and print:
> >
> > "[idx < 0 && ptr] kernel/sched/core.c"
> >
> > Which is an extended string that will be printed in warnings.
> >
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > ---
> > arch/x86/include/asm/bug.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
> > index aff1c6b7a7f3..e966199c8ef7 100644
> > --- a/arch/x86/include/asm/bug.h
> > +++ b/arch/x86/include/asm/bug.h
> > @@ -50,7 +50,7 @@ do { \
> > "\t.org 2b+%c3\n" \
> > ".popsection\n" \
> > extra \
> > - : : "i" (__FILE__), "i" (__LINE__), \
> > + : : "i" (cond_str __FILE__), "i" (__LINE__), \
> > "i" (flags), \
> > "i" (sizeof(struct bug_entry))); \
> > } while (0)
>
> Sneaky :-)
BTW., any reason why we go all the trouble with the bug_entry::line u16
number, instead of storing it in the bug_entry::file string with a
:__LINE__ postfix or so?
Using 4 bytes doesn't even save any RAM, given that the average line
position number within the kernel is around 3 bytes:
$ for N in $(git grep -lE 'WARN_ON|BUG_ON|WARN\(|BUG\(' -- '*.[ch]'); do echo -n $(($(cat $N | wc -l)/2)) | wc -c; done | sort -n | uniq -c
10 1
1209 2
6645 3
1582 4
10 5
( This is the histogram of the length of average line numbers within
the kernel's ~9,400 .[ch] source code files that are using these
facilities. )
So concatenation would save on complexity, IMHO, and it would give us
flexibility as well, if we passed in the string from higher layers. We
wouldn't have to change architecture level code at all for this series
for example.
Not to mention that some files within the kernel are beyond the 16-bit
limit already, 38K to 222K lines of code:
starship:~/tip> wc -l drivers/gpu/drm/amd/include/asic_reg/dcn/dcn_3_2_0_sh_mask.h
222,948 drivers/gpu/drm/amd/include/asic_reg/dcn/dcn_3_2_0_sh_mask.h
starship:~/tip> wc -l crypto/testmgr.h
38,897 crypto/testmgr.h
So u16 line numbers are also a (minor) breakage waiting to happen.
Thanks,
Ingo
On Thu, 27 Mar 2025 at 02:36, Ingo Molnar <mingo@kernel.org> wrote:
>
> BTW., any reason why we go all the trouble with the bug_entry::line u16
> number, instead of storing it in the bug_entry::file string with a
> :__LINE__ postfix or so?
The compiler will happily share the same storage for identical
strings, so that was an issue: re-using the same memory for the same
filename being repeated multiple times.
That obviously doesn't work anyway once you add the warning string to
it, so that makes that whole argument go away.
Linus
* Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, 27 Mar 2025 at 02:36, Ingo Molnar <mingo@kernel.org> wrote: > > > > BTW., any reason why we go all the trouble with the bug_entry::line u16 > > number, instead of storing it in the bug_entry::file string with a > > :__LINE__ postfix or so? > > The compiler will happily share the same storage for identical > strings, so that was an issue: re-using the same memory for the same > filename being repeated multiple times. ohhh ... TIL. > That obviously doesn't work anyway once you add the warning string to > it, so that makes that whole argument go away. Yeah. Explains the +100K increase in .data as well, which was more than what I expected. Thanks, Ingo
On Thu, Mar 27, 2025 at 10:36:32AM +0100, Ingo Molnar wrote:
> > > diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
> > > index aff1c6b7a7f3..e966199c8ef7 100644
> > > --- a/arch/x86/include/asm/bug.h
> > > +++ b/arch/x86/include/asm/bug.h
> > > @@ -50,7 +50,7 @@ do { \
> > > "\t.org 2b+%c3\n" \
> > > ".popsection\n" \
> > > extra \
> > > - : : "i" (__FILE__), "i" (__LINE__), \
> > > + : : "i" (cond_str __FILE__), "i" (__LINE__), \
> > > "i" (flags), \
> > > "i" (sizeof(struct bug_entry))); \
> > > } while (0)
> >
> > Sneaky :-)
>
> BTW., any reason why we go all the trouble with the bug_entry::line u16
> number, instead of storing it in the bug_entry::file string with a
> :__LINE__ postfix or so?
I have no clue; this is well before my time. But yes, that seems a
viable option indeed.
* Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 26, 2025 at 09:47:49AM +0100, Ingo Molnar wrote:
> > This allows the reuse of the UD2 based 'struct bug_entry' low-overhead
> > _BUG_FLAGS() implementation and string-printing backend, without
> > having to add a new field.
> >
> > An example:
> >
> > If we have the following WARN_ON_ONCE() in kernel/sched/core.c:
> >
> > WARN_ON_ONCE(idx < 0 && ptr);
> >
> > Then previously _BUG_FLAGS() would store this string in bug_entry::file:
> >
> > "kernel/sched/core.c"
> >
> > After this patch, it would store and print:
> >
> > "[idx < 0 && ptr] kernel/sched/core.c"
> >
> > Which is an extended string that will be printed in warnings.
> >
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > ---
> > arch/x86/include/asm/bug.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
> > index aff1c6b7a7f3..e966199c8ef7 100644
> > --- a/arch/x86/include/asm/bug.h
> > +++ b/arch/x86/include/asm/bug.h
> > @@ -50,7 +50,7 @@ do { \
> > "\t.org 2b+%c3\n" \
> > ".popsection\n" \
> > extra \
> > - : : "i" (__FILE__), "i" (__LINE__), \
> > + : : "i" (cond_str __FILE__), "i" (__LINE__), \
> > "i" (flags), \
> > "i" (sizeof(struct bug_entry))); \
> > } while (0)
>
> Sneaky :-) Do we want to touch up all the other archs? I mean, you
> already touched them anyway earlier in the series in order to push this
> argument through.
Sneaky how you make it sound so simple ;-)
... some time later:
c9bb718f4d8a bugs/sh: Use 'cond_str' in __WARN_FLAGS()
6fd6983325f7 bugs/parisc: Use 'cond_str' in __WARN_FLAGS()
cc6f8cdc5438 bugs/riscv: Pass in 'cond_str' to __BUG_FLAGS() and use it
b2becbe8b469 bugs/s390: Pass in 'cond_str' to __EMIT_BUG() and use it
8d1deb72c07f bugs/LoongArch: Pass in 'cond_str' to __BUG_ENTRY() and use it
f4a1a3f7f1bb bugs/powerpc: Pass in 'cond_str' to BUG_ENTRY() and use it
There were like 5 separate variants of how architectures make use of
__WARN_FLAGS(), and the 6 patches above are totally untested. Combo
patch below.
Thanks,
Ingo
============================>
arch/loongarch/include/asm/bug.h | 22 +++++++++++-----------
arch/parisc/include/asm/bug.h | 2 +-
arch/powerpc/include/asm/bug.h | 12 ++++++------
arch/riscv/include/asm/bug.h | 10 +++++-----
arch/s390/include/asm/bug.h | 10 +++++-----
arch/sh/include/asm/bug.h | 2 +-
6 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/arch/loongarch/include/asm/bug.h b/arch/loongarch/include/asm/bug.h
index 51c2cb98d728..b8b4d9f569c1 100644
--- a/arch/loongarch/include/asm/bug.h
+++ b/arch/loongarch/include/asm/bug.h
@@ -20,39 +20,39 @@
#endif
#ifndef CONFIG_GENERIC_BUG
-#define __BUG_ENTRY(flags)
+#define __BUG_ENTRY(cond_str, flags)
#else
-#define __BUG_ENTRY(flags) \
+#define __BUG_ENTRY(cond_str, flags) \
.pushsection __bug_table, "aw"; \
.align 2; \
10000: .long 10001f - .; \
- _BUGVERBOSE_LOCATION(__FILE__, __LINE__) \
- .short flags; \
+ _BUGVERBOSE_LOCATION(cond_str __FILE__, __LINE__) \
+ .short flags; \
.popsection; \
10001:
#endif
-#define ASM_BUG_FLAGS(flags) \
- __BUG_ENTRY(flags) \
+#define ASM_BUG_FLAGS(cond_str, flags) \
+ __BUG_ENTRY(cond_str, flags) \
break BRK_BUG;
-#define ASM_BUG() ASM_BUG_FLAGS(0)
+#define ASM_BUG() ASM_BUG_FLAGS("", 0)
-#define __BUG_FLAGS(flags, extra) \
- asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags)) \
+#define __BUG_FLAGS(cond_str, flags, extra) \
+ asm_inline volatile (__stringify(ASM_BUG_FLAGS(cond_str, flags)) \
extra);
#define __WARN_FLAGS(cond_str, flags) \
do { \
instrumentation_begin(); \
- __BUG_FLAGS(BUGFLAG_WARNING|(flags), ANNOTATE_REACHABLE(10001b));\
+ __BUG_FLAGS(cond_str, BUGFLAG_WARNING|(flags), ANNOTATE_REACHABLE(10001b));\
instrumentation_end(); \
} while (0)
#define BUG() \
do { \
instrumentation_begin(); \
- __BUG_FLAGS(0, ""); \
+ __BUG_FLAGS("", 0, ""); \
unreachable(); \
} while (0)
diff --git a/arch/parisc/include/asm/bug.h b/arch/parisc/include/asm/bug.h
index 1a87cf80ec3c..2d14d6cf21f3 100644
--- a/arch/parisc/include/asm/bug.h
+++ b/arch/parisc/include/asm/bug.h
@@ -61,7 +61,7 @@
"\t.short %1, %2\n" \
"\t.blockz %3-2*4-2*2\n" \
"\t.popsection" \
- : : "i" (__FILE__), "i" (__LINE__), \
+ : : "i" (cond_str __FILE__), "i" (__LINE__), \
"i" (BUGFLAG_WARNING|(flags)), \
"i" (sizeof(struct bug_entry)) ); \
} while(0)
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 34d39ec79720..a2120fbedd09 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -51,11 +51,11 @@
".previous\n"
#endif
-#define BUG_ENTRY(insn, flags, ...) \
+#define BUG_ENTRY(cond_str, insn, flags, ...) \
__asm__ __volatile__( \
"1: " insn "\n" \
_EMIT_BUG_ENTRY \
- : : "i" (__FILE__), "i" (__LINE__), \
+ : : "i" (cond_str __FILE__), "i" (__LINE__), \
"i" (flags), \
"i" (sizeof(struct bug_entry)), \
##__VA_ARGS__)
@@ -67,12 +67,12 @@
*/
#define BUG() do { \
- BUG_ENTRY("twi 31, 0, 0", 0); \
+ BUG_ENTRY("", "twi 31, 0, 0", 0); \
unreachable(); \
} while (0)
#define HAVE_ARCH_BUG
-#define __WARN_FLAGS(cond_str, flags) BUG_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags))
+#define __WARN_FLAGS(cond_str, flags) BUG_ENTRY(cond_str, "twi 31, 0, 0", BUGFLAG_WARNING | (flags))
#ifdef CONFIG_PPC64
#define BUG_ON(x) do { \
@@ -80,7 +80,7 @@
if (x) \
BUG(); \
} else { \
- BUG_ENTRY(PPC_TLNEI " %4, 0", 0, "r" ((__force long)(x))); \
+ BUG_ENTRY(#x, PPC_TLNEI " %4, 0", 0, "r" ((__force long)(x))); \
} \
} while (0)
@@ -90,7 +90,7 @@
if (__ret_warn_on) \
__WARN(); \
} else { \
- BUG_ENTRY(PPC_TLNEI " %4, 0", \
+ BUG_ENTRY(#x, PPC_TLNEI " %4, 0", \
BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \
"r" (__ret_warn_on)); \
} \
diff --git a/arch/riscv/include/asm/bug.h b/arch/riscv/include/asm/bug.h
index b22ee4d2c882..6278523dd2d1 100644
--- a/arch/riscv/include/asm/bug.h
+++ b/arch/riscv/include/asm/bug.h
@@ -50,7 +50,7 @@ typedef u32 bug_insn_t;
#endif
#ifdef CONFIG_GENERIC_BUG
-#define __BUG_FLAGS(flags) \
+#define __BUG_FLAGS(cond_str, flags) \
do { \
__asm__ __volatile__ ( \
"1:\n\t" \
@@ -61,22 +61,22 @@ do { \
".org 2b + %3\n\t" \
".popsection" \
: \
- : "i" (__FILE__), "i" (__LINE__), \
+ : "i" (cond_str __FILE__), "i" (__LINE__), \
"i" (flags), \
"i" (sizeof(struct bug_entry))); \
} while (0)
#else /* CONFIG_GENERIC_BUG */
-#define __BUG_FLAGS(flags) do { \
+#define __BUG_FLAGS(cond_str, flags) do { \
__asm__ __volatile__ ("ebreak\n"); \
} while (0)
#endif /* CONFIG_GENERIC_BUG */
#define BUG() do { \
- __BUG_FLAGS(0); \
+ __BUG_FLAGS("", 0); \
unreachable(); \
} while (0)
-#define __WARN_FLAGS(cond_str, flags) __BUG_FLAGS(BUGFLAG_WARNING|(flags))
+#define __WARN_FLAGS(cond_str, flags) __BUG_FLAGS(cond_str, BUGFLAG_WARNING|(flags))
#define HAVE_ARCH_BUG
diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h
index ef3e495ec1e3..e3d839517c17 100644
--- a/arch/s390/include/asm/bug.h
+++ b/arch/s390/include/asm/bug.h
@@ -8,7 +8,7 @@
#ifdef CONFIG_DEBUG_BUGVERBOSE
-#define __EMIT_BUG(x) do { \
+#define __EMIT_BUG(cond_str, x) do { \
asm_inline volatile( \
"0: mc 0,0\n" \
".section .rodata.str,\"aMS\",@progbits,1\n" \
@@ -20,14 +20,14 @@
" .short %0,%1\n" \
" .org 2b+%2\n" \
".previous\n" \
- : : "i" (__LINE__), \
+ : : "i" (cond_str __LINE__), \
"i" (x), \
"i" (sizeof(struct bug_entry))); \
} while (0)
#else /* CONFIG_DEBUG_BUGVERBOSE */
-#define __EMIT_BUG(x) do { \
+#define __EMIT_BUG(cond_str, x) do { \
asm_inline volatile( \
"0: mc 0,0\n" \
".section __bug_table,\"aw\"\n" \
@@ -42,12 +42,12 @@
#endif /* CONFIG_DEBUG_BUGVERBOSE */
#define BUG() do { \
- __EMIT_BUG(0); \
+ __EMIT_BUG("", 0); \
unreachable(); \
} while (0)
#define __WARN_FLAGS(cond_str, flags) do { \
- __EMIT_BUG(BUGFLAG_WARNING|(flags)); \
+ __EMIT_BUG(cond_str, BUGFLAG_WARNING|(flags)); \
} while (0)
#define WARN_ON(x) ({ \
diff --git a/arch/sh/include/asm/bug.h b/arch/sh/include/asm/bug.h
index 834c621ab249..20d5220bf452 100644
--- a/arch/sh/include/asm/bug.h
+++ b/arch/sh/include/asm/bug.h
@@ -59,7 +59,7 @@ do { \
_EMIT_BUG_ENTRY \
: \
: "n" (TRAPA_BUG_OPCODE), \
- "i" (__FILE__), \
+ "i" (cond_str __FILE__), \
"i" (__LINE__), \
"i" (BUGFLAG_WARNING|(flags)), \
"i" (sizeof(struct bug_entry))); \
* Ingo Molnar <mingo@kernel.org> wrote:
> ... some time later:
>
> b2becbe8b469 bugs/s390: Pass in 'cond_str' to __EMIT_BUG() and use it
> --- a/arch/s390/include/asm/bug.h
> +++ b/arch/s390/include/asm/bug.h
> @@ -8,7 +8,7 @@
>
> #ifdef CONFIG_DEBUG_BUGVERBOSE
>
> -#define __EMIT_BUG(x) do { \
> +#define __EMIT_BUG(cond_str, x) do { \
> asm_inline volatile( \
> "0: mc 0,0\n" \
> ".section .rodata.str,\"aMS\",@progbits,1\n" \
> @@ -20,14 +20,14 @@
> " .short %0,%1\n" \
> " .org 2b+%2\n" \
> ".previous\n" \
> - : : "i" (__LINE__), \
> + : : "i" (cond_str __LINE__), \
> "i" (x), \
> "i" (sizeof(struct bug_entry))); \
> } while (0)
>
> #else /* CONFIG_DEBUG_BUGVERBOSE */
>
> -#define __EMIT_BUG(x) do { \
> +#define __EMIT_BUG(cond_str, x) do { \
> asm_inline volatile( \
> "0: mc 0,0\n" \
> ".section __bug_table,\"aw\"\n" \
> @@ -42,12 +42,12 @@
> #endif /* CONFIG_DEBUG_BUGVERBOSE */
>
> #define BUG() do { \
> - __EMIT_BUG(0); \
> + __EMIT_BUG("", 0); \
> unreachable(); \
> } while (0)
>
> #define __WARN_FLAGS(cond_str, flags) do { \
> - __EMIT_BUG(BUGFLAG_WARNING|(flags)); \
> + __EMIT_BUG(cond_str, BUGFLAG_WARNING|(flags)); \
> } while (0)
>
> #define WARN_ON(x) ({ \
So the fix below makes it build on S390, but it won't link:
kernel/exit.o:(__bug_table+0x8): relocation truncated to fit: R_390_16 against `.rodata.str1.2'
kernel/exit.o:(__bug_table+0x14): relocation truncated to fit: R_390_16 against `.rodata.str1.2'
kernel/exit.o:(__bug_table+0x20): relocation truncated to fit: R_390_16 against `.rodata.str1.2'
kernel/exit.o:(__bug_table+0x2c): relocation truncated to fit: R_390_16 against `.rodata.str1.2'
kernel/exit.o:(__bug_table+0x38): relocation truncated to fit: R_390_16 against `.rodata.str1.2'
kernel/exit.o:(__bug_table+0x44): relocation truncated to fit: R_390_16 against `.rodata.str1.2'
kernel/exit.o:(__bug_table+0x50): relocation truncated to fit: R_390_16 against `.rodata.str1.2'
init/main.o:(__bug_table+0x8): relocation truncated to fit: R_390_16 against `.rodata.str1.2'
init/main.o:(__bug_table+0x14): relocation truncated to fit: R_390_16 against `.rodata.str1.2'
init/main.o:(__bug_table+0x20): relocation truncated to fit: R_390_16 against `.rodata.str1.2'
init/main.o:(__bug_table+0x2c): additional relocation overflows omitted from the output
From the _16 name my rough guess is that the larger string table has
overflown 16 bits (64k)?
At which point I'd much rather go back to plan-A: pass in cond_str to
__WARN_FLAGS(), and wash my hands and leave architectures to make use
of them as they wish to. :-)
Thanks,
Ingo
=====================>
arch/s390/include/asm/bug.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h
index e3d839517c17..7eb9b44f78cb 100644
--- a/arch/s390/include/asm/bug.h
+++ b/arch/s390/include/asm/bug.h
@@ -20,7 +20,7 @@
" .short %0,%1\n" \
" .org 2b+%2\n" \
".previous\n" \
- : : "i" (cond_str __LINE__), \
+ : : "i" (__stringify(cond_str __LINE__)), \
"i" (x), \
"i" (sizeof(struct bug_entry))); \
} while (0)
© 2016 - 2025 Red Hat, Inc.