Some unit tests intentionally trigger warning backtraces by passing bad
parameters to kernel API functions. Such unit tests typically check the
return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable
nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be
investigated and has to be marked to be ignored, for example by
adjusting filter scripts. Such filters are ad-hoc because there is
no real standard format for warnings. On top of that, such filter
scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected
warning backtraces start / end here" to the kernel log. However, that
would again require filter scripts, it might result in missing real
problematic warning backtraces triggered while the test is running, and
the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific
warning backtraces while executing test code. Support suppressing multiple
backtraces while at the same time limiting changes to generic code to the
absolute minimum. Architecture specific changes are kept at minimum by
retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure.
The second patch introduces support for counting suppressed backtraces.
This capability is used in patch three to implement unit tests.
Patch four documents the new API.
The next two patches add support for suppressing backtraces in drm_rect
and dev_addr_lists unit tests. These patches are intended to serve as
examples for the use of the functionality introduced with this series.
The remaining patches implement the necessary changes for all
architectures with GENERIC_BUG support.
This series is based on the RFC patch and subsequent discussion at
https://patchwork.kernel.org/project/linux-kselftest/patch/02546e59-1afe-4b08-ba81-d94f3b691c9a@moroto.mountain/
and offers a more comprehensive solution of the problem discussed there.
Design note:
Function pointers are only added to the __bug_table section if both
CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
size increases if CONFIG_KUNIT=n. There would be some benefits to
adding those pointers all the time (reduced complexity, ability to
display function names in BUG/WARNING messages). That change, if
desired, can be made later.
Checkpatch note:
Remaining checkpatch errors and warnings were deliberately ignored.
Some are triggered by matching coding style or by comments interpreted
as code, others by assembler macros which are disliked by checkpatch.
Suggestions for improvements are welcome.
Changes since RFC:
- Minor cleanups and bug fixes
- Added support for all affected architectures
- Added support for counting suppressed warnings
- Added unit tests using those counters
- Added patch to suppress warning backtraces in dev_addr_lists tests
Some unit tests intentionally trigger warning backtraces by passing
bad parameters to API functions. Such unit tests typically check the
return value from those calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable
nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be
investigated and has to be marked to be ignored, for example by
adjusting filter scripts. Such filters are ad-hoc because there is
no real standard format for warnings. On top of that, such filter
scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected
warning backtraces start / end here" to the kernel log. However, that
would again require filter scripts, it might result in missing real
problematic warning backtraces triggered while the test is running, and
the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific
warning backtraces while executing test code.
Cc: Dan Carpenter <redacted>
Cc: Daniel Diaz <redacted>
Cc: Naresh Kamboju <redacted>
Cc: Kees Cook <redacted>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
include/asm-generic/bug.h | 16 +++++++++---
include/kunit/bug.h | 51 +++++++++++++++++++++++++++++++++++++++
include/kunit/test.h | 1 +
include/linux/bug.h | 13 ++++++++++
lib/bug.c | 51 ++++++++++++++++++++++++++++++++++++---
lib/kunit/Makefile | 6 +++--
lib/kunit/bug.c | 40 ++++++++++++++++++++++++++++++
7 files changed, 169 insertions(+), 9 deletions(-)
create mode 100644 include/kunit/bug.h
create mode 100644 lib/kunit/bug.c
@@ -14,8 +14,10 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y)kunit-objs+=debugfs.oendif-# KUnit 'hooks' are built-in even when KUnit is built as a module.-obj-y+=hooks.o+# KUnit 'hooks' and bug handling are built-in even when KUnit is built+# as a module.+obj-y+=hooks.o\+bug.oobj-$(CONFIG_KUNIT_TEST)+=kunit-test.o
Count suppressed warning backtraces to enable code which suppresses
warning backtraces to check if the expected backtrace(s) have been
observed.
Using atomics for the backtrace count resulted in build errors on some
architectures due to include file recursion, so use a plain integer
for now.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
include/kunit/bug.h | 7 ++++++-
lib/kunit/bug.c | 4 +++-
2 files changed, 9 insertions(+), 2 deletions(-)
Add unit tests to verify that warning backtrace suppression works.
If backtrace suppression does _not_ work, the unit tests will likely
trigger unsuppressed backtraces, which should actually help to get
the affected architectures / platforms fixed.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
lib/kunit/Makefile | 3 +-
lib/kunit/backtrace-suppression-test.c | 104 +++++++++++++++++++++++++
2 files changed, 106 insertions(+), 1 deletion(-)
create mode 100644 lib/kunit/backtrace-suppression-test.c
@@ -0,0 +1,104 @@+// SPDX-License-Identifier: GPL-2.0+/*+*KUnittestforsuppressingwarningtracebacks+*+*Copyright(C)2024,GuenterRoeck+*Author:GuenterRoeck<linux@roeck-us.net>+*/++#include<kunit/test.h>+#include<linux/bug.h>++staticvoidbacktrace_suppression_test_warn_direct(structkunit*test)+{+DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct);++START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct);+WARN(1,"This backtrace should be suppressed");+END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct);++KUNIT_EXPECT_EQ(test,SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_direct),1);+}++staticvoidtrigger_backtrace_warn(void)+{+WARN(1,"This backtrace should be suppressed");+}++staticvoidbacktrace_suppression_test_warn_indirect(structkunit*test)+{+DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn);++START_SUPPRESSED_WARNING(trigger_backtrace_warn);+trigger_backtrace_warn();+END_SUPPRESSED_WARNING(trigger_backtrace_warn);++KUNIT_EXPECT_EQ(test,SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn),1);+}++staticvoidbacktrace_suppression_test_warn_multi(structkunit*test)+{+DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn);+DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi);++START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi);+START_SUPPRESSED_WARNING(trigger_backtrace_warn);+WARN(1,"This backtrace should be suppressed");+trigger_backtrace_warn();+END_SUPPRESSED_WARNING(trigger_backtrace_warn);+END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi);++KUNIT_EXPECT_EQ(test,SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_multi),1);+KUNIT_EXPECT_EQ(test,SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn),1);+}++staticvoidbacktrace_suppression_test_warn_on_direct(structkunit*test)+{+DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct);++if(!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE)&&!IS_ENABLED(CONFIG_KALLSYMS))+kunit_skip(test,"requires CONFIG_DEBUG_BUGVERBOSE or CONFIG_KALLSYMS");++START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct);+WARN_ON(1);+END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct);++KUNIT_EXPECT_EQ(test,+SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_on_direct),1);+}++staticvoidtrigger_backtrace_warn_on(void)+{+WARN_ON(1);+}++staticvoidbacktrace_suppression_test_warn_on_indirect(structkunit*test)+{+DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn_on);++if(!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE))+kunit_skip(test,"requires CONFIG_DEBUG_BUGVERBOSE");++START_SUPPRESSED_WARNING(trigger_backtrace_warn_on);+trigger_backtrace_warn_on();+END_SUPPRESSED_WARNING(trigger_backtrace_warn_on);++KUNIT_EXPECT_EQ(test,SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn_on),1);+}++staticstructkunit_casebacktrace_suppression_test_cases[]={+KUNIT_CASE(backtrace_suppression_test_warn_direct),+KUNIT_CASE(backtrace_suppression_test_warn_indirect),+KUNIT_CASE(backtrace_suppression_test_warn_multi),+KUNIT_CASE(backtrace_suppression_test_warn_on_direct),+KUNIT_CASE(backtrace_suppression_test_warn_on_indirect),+{}+};++staticstructkunit_suitebacktrace_suppression_test_suite={+.name="backtrace-suppression-test",+.test_cases=backtrace_suppression_test_cases,+};+kunit_test_suites(&backtrace_suppression_test_suite);++MODULE_LICENSE("GPL");
@@ -157,6 +157,34 @@ Alternatively, one can take full control over the error message by using if (some_setup_function()) KUNIT_FAIL(test, "Failed to setup thing for testing");+Suppressing warning backtraces+------------------------------++Some unit tests trigger warning backtraces either intentionally or as side+effect. Such backtraces are normally undesirable since they distract from+the actual test and may result in the impression that there is a problem.++Such backtraces can be suppressed. To suppress a backtrace in some_function(),+use the following code.++..code-block:: c++ static void some_test(struct kunit *test)+ {+ DEFINE_SUPPRESSED_WARNING(some_function);++ START_SUPPRESSED_WARNING(some_function);+ trigger_backtrace();+ END_SUPPRESSED_WARNING(some_function);+ }++SUPPRESSED_WARNING_COUNT() returns the number of suppressed backtraces. If the+suppressed backtrace was triggered on purpose, this can be used to check if+the backtrace was actually triggered.++..code-block:: c++ KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(some_function), 1); Test Suites ~~~~~~~~~~~
@@ -857,4 +885,4 @@ For example: dev_managed_string = devm_kstrdup(fake_device, "Hello, World!"); // Everything is cleaned up automatically when the test ends.- }
The drm_test_rect_calc_hscale and drm_test_rect_calc_vscale unit tests
intentionally trigger warning backtraces by providing bad parameters to
the tested functions. What is tested is the return value, not the existence
of a warning backtrace. Suppress the backtraces to avoid clogging the
kernel log.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/gpu/drm/tests/drm_rect_test.c | 6 ++++++
1 file changed, 6 insertions(+)
dev_addr_lists_test generates lock warning noise at the end of tests
if lock debugging is enabled. There are two sets of warnings.
WARNING: CPU: 0 PID: 689 at kernel/locking/mutex.c:923 __mutex_unlock_slowpath.constprop.0+0x13c/0x368
DEBUG_LOCKS_WARN_ON(__owner_task(owner) != __get_current())
WARNING: kunit_try_catch/1336 still has locks held!
KUnit test cleanup is not guaranteed to run in the same thread as the test
itself. For this test, this means that rtnl_lock() and rtnl_unlock() may
be called from different threads. This triggers the warnings.
Suppress the warnings because they are irrelevant for the test and just
confusing.
The first warning can be suppressed by using START_SUPPRESSED_WARNING()
and END_SUPPRESSED_WARNING() around the call to rtnl_unlock(). To suppress
the second warning, it is necessary to set debug_locks_silent while the
rtnl lock is held.
Cc: David Gow <redacted>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
net/core/dev_addr_lists_test.c | 6 ++++++
1 file changed, 6 insertions(+)
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
arch/x86/include/asm/bug.h | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
arch/arm64/include/asm/asm-bug.h | 29 +++++++++++++++++++----------
arch/arm64/include/asm/bug.h | 8 +++++++-
2 files changed, 26 insertions(+), 11 deletions(-)
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
arch/loongarch/include/asm/bug.h | 38 +++++++++++++++++++++++---------
1 file changed, 27 insertions(+), 11 deletions(-)
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
While at it, declare assembler parameters as constants where possible.
Refine .blockz instructions to calculate the necessary padding instead
of using fixed values.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
arch/parisc/include/asm/bug.h | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
arch/s390/include/asm/bug.h | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
arch/sh/include/asm/bug.h | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
To simplify the implementation, unify the __BUG_ENTRY_ADDR and
__BUG_ENTRY_FILE macros into a single macro named __BUG_REL() which takes
the address, file, or function reference as parameter.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
arch/riscv/include/asm/bug.h | 38 ++++++++++++++++++++++++------------
1 file changed, 26 insertions(+), 12 deletions(-)
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
arch/powerpc/include/asm/bug.h | 37 +++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
On Tue, Mar 12, 2024 at 10:02:56AM -0700, Guenter Roeck wrote:
Some unit tests intentionally trigger warning backtraces by passing
bad parameters to API functions. Such unit tests typically check the
return value from those calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable
nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be
investigated and has to be marked to be ignored, for example by
adjusting filter scripts. Such filters are ad-hoc because there is
no real standard format for warnings. On top of that, such filter
scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected
warning backtraces start / end here" to the kernel log. However, that
would again require filter scripts, it might result in missing real
problematic warning backtraces triggered while the test is running, and
the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific
warning backtraces while executing test code.
Cc: Dan Carpenter <redacted>
Cc: Daniel Diaz <redacted>
Cc: Naresh Kamboju <redacted>
Cc: Kees Cook <redacted>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Yup, this looks fine to me.
Reviewed-by: Kees Cook <redacted>
--
Kees Cook
On Tue, Mar 12, 2024 at 10:02:57AM -0700, Guenter Roeck wrote:
quoted hunk
Count suppressed warning backtraces to enable code which suppresses
warning backtraces to check if the expected backtrace(s) have been
observed.
Using atomics for the backtrace count resulted in build errors on some
architectures due to include file recursion, so use a plain integer
for now.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
include/kunit/bug.h | 7 ++++++-
lib/kunit/bug.c | 4 +++-
2 files changed, 9 insertions(+), 2 deletions(-)
On Tue, Mar 12, 2024 at 10:02:58AM -0700, Guenter Roeck wrote:
Add unit tests to verify that warning backtrace suppression works.
If backtrace suppression does _not_ work, the unit tests will likely
trigger unsuppressed backtraces, which should actually help to get
the affected architectures / platforms fixed.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Thanks for the patchset.
This patch series applied on top of Linux next and tested.
Tested-by: Linux Kernel Functional Testing <redacted>
--
Linaro LKFT
https://lkft.linaro.org
Hi Günter,
On Tue, Mar 12, 2024 at 6:06 PM Guenter Roeck [off-list ref] wrote:
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
This change conflicts with commit 3938490e78f443fb ("s390/bug:
remove entry size from __bug_table section") in linus/master.
I guess it should just be dropped?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Günter,
On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck [off-list ref] wrote:
Some unit tests intentionally trigger warning backtraces by passing bad
parameters to kernel API functions. Such unit tests typically check the
return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable
nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be
investigated and has to be marked to be ignored, for example by
adjusting filter scripts. Such filters are ad-hoc because there is
no real standard format for warnings. On top of that, such filter
scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected
warning backtraces start / end here" to the kernel log. However, that
would again require filter scripts, it might result in missing real
problematic warning backtraces triggered while the test is running, and
the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific
warning backtraces while executing test code. Support suppressing multiple
backtraces while at the same time limiting changes to generic code to the
absolute minimum. Architecture specific changes are kept at minimum by
retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure.
The second patch introduces support for counting suppressed backtraces.
This capability is used in patch three to implement unit tests.
Patch four documents the new API.
The next two patches add support for suppressing backtraces in drm_rect
and dev_addr_lists unit tests. These patches are intended to serve as
examples for the use of the functionality introduced with this series.
The remaining patches implement the necessary changes for all
architectures with GENERIC_BUG support.
Thanks for your series!
I gave it a try on m68k, just running backtrace-suppression-test,
and that seems to work fine.
Design note:
Function pointers are only added to the __bug_table section if both
CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
size increases if CONFIG_KUNIT=n. There would be some benefits to
adding those pointers all the time (reduced complexity, ability to
display function names in BUG/WARNING messages). That change, if
desired, can be made later.
Unfortunately this also increases kernel size in the CONFIG_KUNIT=m
case (ca. 80 KiB for atari_defconfig), making it less attractive to have
kunit and all tests enabled as modules in my standard kernel.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Günter,
On Tue, Mar 12, 2024 at 6:06 PM Guenter Roeck [off-list ref] wrote:
quoted
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
This change conflicts with commit 3938490e78f443fb ("s390/bug:
remove entry size from __bug_table section") in linus/master.
I guess it should just be dropped?
Yes, I know. I'll send v2 rebased to v6.9-rc1 once it is available and,
yes, the change will be gone after that.
Thanks,
Guenter
Hi Günter,
On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck [off-list ref] wrote:
quoted
Some unit tests intentionally trigger warning backtraces by passing bad
parameters to kernel API functions. Such unit tests typically check the
return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable
nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be
investigated and has to be marked to be ignored, for example by
adjusting filter scripts. Such filters are ad-hoc because there is
no real standard format for warnings. On top of that, such filter
scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected
warning backtraces start / end here" to the kernel log. However, that
would again require filter scripts, it might result in missing real
problematic warning backtraces triggered while the test is running, and
the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific
warning backtraces while executing test code. Support suppressing multiple
backtraces while at the same time limiting changes to generic code to the
absolute minimum. Architecture specific changes are kept at minimum by
retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure.
The second patch introduces support for counting suppressed backtraces.
This capability is used in patch three to implement unit tests.
Patch four documents the new API.
The next two patches add support for suppressing backtraces in drm_rect
and dev_addr_lists unit tests. These patches are intended to serve as
examples for the use of the functionality introduced with this series.
The remaining patches implement the necessary changes for all
architectures with GENERIC_BUG support.
Thanks for your series!
I gave it a try on m68k, just running backtrace-suppression-test,
and that seems to work fine.
quoted
Design note:
Function pointers are only added to the __bug_table section if both
CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
size increases if CONFIG_KUNIT=n. There would be some benefits to
adding those pointers all the time (reduced complexity, ability to
display function names in BUG/WARNING messages). That change, if
desired, can be made later.
Unfortunately this also increases kernel size in the CONFIG_KUNIT=m
case (ca. 80 KiB for atari_defconfig), making it less attractive to have
kunit and all tests enabled as modules in my standard kernel.
Good point. Indeed, it does. I wanted to avoid adding a configuration option,
but maybe I should add it after all. How about something like this ?
+config KUNIT_SUPPRESS_BACKTRACE
+ bool "KUnit - Enable backtrace suppression"
+ default y
+ help
+ Enable backtrace suppression for KUnit. If enabled, backtraces
+ generated intentionally by KUnit tests can be suppressed. Disable
+ to reduce kernel image size if image size is more important than
+ suppression of backtraces generated by KUnit tests.
+
Thanks,
Guenter
From: Maxime Ripard <mripard@kernel.org> Date: 2024-03-14 15:02:02
On Thu, Mar 14, 2024 at 07:37:13AM -0700, Guenter Roeck wrote:
On 3/14/24 06:36, Geert Uytterhoeven wrote:
quoted
Hi Günter,
On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck [off-list ref] wrote:
quoted
Some unit tests intentionally trigger warning backtraces by passing bad
parameters to kernel API functions. Such unit tests typically check the
return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable
nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be
investigated and has to be marked to be ignored, for example by
adjusting filter scripts. Such filters are ad-hoc because there is
no real standard format for warnings. On top of that, such filter
scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected
warning backtraces start / end here" to the kernel log. However, that
would again require filter scripts, it might result in missing real
problematic warning backtraces triggered while the test is running, and
the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific
warning backtraces while executing test code. Support suppressing multiple
backtraces while at the same time limiting changes to generic code to the
absolute minimum. Architecture specific changes are kept at minimum by
retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure.
The second patch introduces support for counting suppressed backtraces.
This capability is used in patch three to implement unit tests.
Patch four documents the new API.
The next two patches add support for suppressing backtraces in drm_rect
and dev_addr_lists unit tests. These patches are intended to serve as
examples for the use of the functionality introduced with this series.
The remaining patches implement the necessary changes for all
architectures with GENERIC_BUG support.
Thanks for your series!
I gave it a try on m68k, just running backtrace-suppression-test,
and that seems to work fine.
quoted
Design note:
Function pointers are only added to the __bug_table section if both
CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
size increases if CONFIG_KUNIT=n. There would be some benefits to
adding those pointers all the time (reduced complexity, ability to
display function names in BUG/WARNING messages). That change, if
desired, can be made later.
Unfortunately this also increases kernel size in the CONFIG_KUNIT=m
case (ca. 80 KiB for atari_defconfig), making it less attractive to have
kunit and all tests enabled as modules in my standard kernel.
Good point. Indeed, it does. I wanted to avoid adding a configuration option,
but maybe I should add it after all. How about something like this ?
+config KUNIT_SUPPRESS_BACKTRACE
+ bool "KUnit - Enable backtrace suppression"
+ default y
+ help
+ Enable backtrace suppression for KUnit. If enabled, backtraces
+ generated intentionally by KUnit tests can be suppressed. Disable
+ to reduce kernel image size if image size is more important than
+ suppression of backtraces generated by KUnit tests.
+
How are tests using that API supposed to handle it then?
Select the config option or put an ifdef?
If the former, we end up in the same situation than without the symbol.
If the latter, we end up in a similar situation than disabling KUNIT
entirely, with some tests not being run which is just terrible.
Maxime
On Thu, Mar 14, 2024 at 07:37:13AM -0700, Guenter Roeck wrote:
quoted
On 3/14/24 06:36, Geert Uytterhoeven wrote:
quoted
Hi Günter,
On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck [off-list ref] wrote:
quoted
Some unit tests intentionally trigger warning backtraces by passing bad
parameters to kernel API functions. Such unit tests typically check the
return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable
nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be
investigated and has to be marked to be ignored, for example by
adjusting filter scripts. Such filters are ad-hoc because there is
no real standard format for warnings. On top of that, such filter
scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected
warning backtraces start / end here" to the kernel log. However, that
would again require filter scripts, it might result in missing real
problematic warning backtraces triggered while the test is running, and
the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific
warning backtraces while executing test code. Support suppressing multiple
backtraces while at the same time limiting changes to generic code to the
absolute minimum. Architecture specific changes are kept at minimum by
retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure.
The second patch introduces support for counting suppressed backtraces.
This capability is used in patch three to implement unit tests.
Patch four documents the new API.
The next two patches add support for suppressing backtraces in drm_rect
and dev_addr_lists unit tests. These patches are intended to serve as
examples for the use of the functionality introduced with this series.
The remaining patches implement the necessary changes for all
architectures with GENERIC_BUG support.
Thanks for your series!
I gave it a try on m68k, just running backtrace-suppression-test,
and that seems to work fine.
quoted
Design note:
Function pointers are only added to the __bug_table section if both
CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
size increases if CONFIG_KUNIT=n. There would be some benefits to
adding those pointers all the time (reduced complexity, ability to
display function names in BUG/WARNING messages). That change, if
desired, can be made later.
Unfortunately this also increases kernel size in the CONFIG_KUNIT=m
case (ca. 80 KiB for atari_defconfig), making it less attractive to have
kunit and all tests enabled as modules in my standard kernel.
Good point. Indeed, it does. I wanted to avoid adding a configuration option,
but maybe I should add it after all. How about something like this ?
+config KUNIT_SUPPRESS_BACKTRACE
+ bool "KUnit - Enable backtrace suppression"
+ default y
+ help
+ Enable backtrace suppression for KUnit. If enabled, backtraces
+ generated intentionally by KUnit tests can be suppressed. Disable
+ to reduce kernel image size if image size is more important than
+ suppression of backtraces generated by KUnit tests.
+
How are tests using that API supposed to handle it then?
Select the config option or put an ifdef?
If the former, we end up in the same situation than without the symbol.
If the latter, we end up in a similar situation than disabling KUNIT
entirely, with some tests not being run which is just terrible.
The API definitions are themselves within #ifdef and dummies if
KUNIT_SUPPRESS_BACKTRACE (currently CONFIG_KUNIT) is disabled.
In include/kunit/bug.h:
#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE
...
#else
#define DEFINE_SUPPRESSED_WARNING(func)
#define START_SUPPRESSED_WARNING(func)
#define END_SUPPRESSED_WARNING(func)
#define IS_SUPPRESSED_WARNING(func) (false)
#define SUPPRESSED_WARNING_COUNT(func) (0)
#endif
Only difference to the current patch series would be
- #if IS_ENABLED(CONFIG_KUNIT)
+ #ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE
in that file and elsewhere.
With KUNIT_SUPPRESS_BACKTRACE=n you'd still get warning backtraces
triggered by the tests, but the number of tests executed as well
as the test results would still be the same.
Thanks,
Guenter
Add name of functions triggering warning backtraces to the __bug_table
object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added
to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
are enabled. Otherwise, the __func__ assembly parameter is replaced with a
(dummy) NULL parameter to avoid an image size increase due to unused
__func__ entries (this is necessary because __func__ is not a define but a
virtual variable).
While at it, declare assembler parameters as constants where possible.
Refine .blockz instructions to calculate the necessary padding instead
of using fixed values.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Hi Günter,
On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck [off-list ref] wrote:
quoted
Some unit tests intentionally trigger warning backtraces by passing bad
parameters to kernel API functions. Such unit tests typically check the
return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable
nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be
investigated and has to be marked to be ignored, for example by
adjusting filter scripts. Such filters are ad-hoc because there is
no real standard format for warnings. On top of that, such filter
scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected
warning backtraces start / end here" to the kernel log. However, that
would again require filter scripts, it might result in missing real
problematic warning backtraces triggered while the test is running, and
the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific
warning backtraces while executing test code. Support suppressing multiple
backtraces while at the same time limiting changes to generic code to the
absolute minimum. Architecture specific changes are kept at minimum by
retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure.
The second patch introduces support for counting suppressed backtraces.
This capability is used in patch three to implement unit tests.
Patch four documents the new API.
The next two patches add support for suppressing backtraces in drm_rect
and dev_addr_lists unit tests. These patches are intended to serve as
examples for the use of the functionality introduced with this series.
The remaining patches implement the necessary changes for all
architectures with GENERIC_BUG support.
Thanks for your series!
I gave it a try on m68k, just running backtrace-suppression-test,
and that seems to work fine.
quoted
Design note:
Function pointers are only added to the __bug_table section if both
CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
size increases if CONFIG_KUNIT=n. There would be some benefits to
adding those pointers all the time (reduced complexity, ability to
display function names in BUG/WARNING messages). That change, if
desired, can be made later.
Unfortunately this also increases kernel size in the CONFIG_KUNIT=m
case (ca. 80 KiB for atari_defconfig), making it less attractive to have
kunit and all tests enabled as modules in my standard kernel.
Good point. Indeed, it does. I wanted to avoid adding a configuration option,
but maybe I should add it after all. How about something like this ?
+config KUNIT_SUPPRESS_BACKTRACE
+ bool "KUnit - Enable backtrace suppression"
+ default y
+ help
+ Enable backtrace suppression for KUnit. If enabled, backtraces
+ generated intentionally by KUnit tests can be suppressed. Disable
+ to reduce kernel image size if image size is more important than
+ suppression of backtraces generated by KUnit tests.
+
Any more comments / feedback on this ? Otherwise I'll introduce the
above configuration option in v2 of the series.
In this context, any suggestions if it should be enabled or disabled by
default ? I personally think it would be more important to be able to
suppress backtraces, but I understand that others may not be willing to
accept a ~1% image size increase with CONFIG_KUNIT=m unless
KUNIT_SUPPRESS_BACKTRACE is explicitly disabled.
Thanks,
Guenter
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2024-03-18 03:25:07
Guenter Roeck [off-list ref] writes:
On 3/14/24 07:37, Guenter Roeck wrote:
quoted
On 3/14/24 06:36, Geert Uytterhoeven wrote:
quoted
Hi Günter,
On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck [off-list ref] wrote:
quoted
Some unit tests intentionally trigger warning backtraces by passing bad
parameters to kernel API functions. Such unit tests typically check the
return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable
nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be
investigated and has to be marked to be ignored, for example by
adjusting filter scripts. Such filters are ad-hoc because there is
no real standard format for warnings. On top of that, such filter
scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected
warning backtraces start / end here" to the kernel log. However, that
would again require filter scripts, it might result in missing real
problematic warning backtraces triggered while the test is running, and
the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific
warning backtraces while executing test code. Support suppressing multiple
backtraces while at the same time limiting changes to generic code to the
absolute minimum. Architecture specific changes are kept at minimum by
retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure.
The second patch introduces support for counting suppressed backtraces.
This capability is used in patch three to implement unit tests.
Patch four documents the new API.
The next two patches add support for suppressing backtraces in drm_rect
and dev_addr_lists unit tests. These patches are intended to serve as
examples for the use of the functionality introduced with this series.
The remaining patches implement the necessary changes for all
architectures with GENERIC_BUG support.
Thanks for your series!
I gave it a try on m68k, just running backtrace-suppression-test,
and that seems to work fine.
quoted
Design note:
Function pointers are only added to the __bug_table section if both
CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
size increases if CONFIG_KUNIT=n. There would be some benefits to
adding those pointers all the time (reduced complexity, ability to
display function names in BUG/WARNING messages). That change, if
desired, can be made later.
Unfortunately this also increases kernel size in the CONFIG_KUNIT=m
case (ca. 80 KiB for atari_defconfig), making it less attractive to have
kunit and all tests enabled as modules in my standard kernel.
Good point. Indeed, it does. I wanted to avoid adding a configuration option,
but maybe I should add it after all. How about something like this ?
+config KUNIT_SUPPRESS_BACKTRACE
+ bool "KUnit - Enable backtrace suppression"
+ default y
+ help
+ Enable backtrace suppression for KUnit. If enabled, backtraces
+ generated intentionally by KUnit tests can be suppressed. Disable
+ to reduce kernel image size if image size is more important than
+ suppression of backtraces generated by KUnit tests.
Any more comments / feedback on this ? Otherwise I'll introduce the
above configuration option in v2 of the series.
In this context, any suggestions if it should be enabled or disabled by
default ? I personally think it would be more important to be able to
suppress backtraces, but I understand that others may not be willing to
accept a ~1% image size increase with CONFIG_KUNIT=m unless
KUNIT_SUPPRESS_BACKTRACE is explicitly disabled.
Please enable it by default.
There are multiple CI systems that will benefit from it, whereas the
number of users enabling KUNIT in severely spaced constrainted
environments is surely small - perhaps just Geert ;).
cheers