Re: [PATCH] tools/mm: add hwpoison-panic tool

On Fri, Jul 31, 2026 at 05:58:22PM -0700, SJ Park wrote:
> > This lives in tools/mm rather than selftests/mm because every successful
> > run crashes the machine, which is not something to run from CI.
> 
> If the purpose of the new program is testing, would it make more sense to put
> it under tools/testing/ ?

Sorry, this was discussed here:

https://lore.kernel.org/all/[email protected]/

> Also, does this test really need to be end-to-end test that cause panic?  How
> about doing unit test of core functions, using tools like kunit?  Or,
> live-patching panic() to only print messages and return before running the
> test?

What I want covered is the wiring, not the individual functions:
get_any_page() classifying a kernel-owned page as -ENOTRECOVERABLE,
action_result() reaching panic_on_unrecoverable_mf(), and the box
actually going down. 

Under kunit I would have to fabricate the page and stub panic(), so the
parts that break would be the parts replaced by the harness.

Unit tests for the classification helpers would be worth having on their
own. They would not replace this one.
> >  BUILD_TARGETS=page-types slabinfo page_owner_sort page_owner_filter thp_swap_allocator_test
> > +BUILD_TARGETS+=memory-failure/hwpoison-panic
> 
> I was initially thought why not underscore but dash on the name?  But, I
> realize it is obviously too persoanl taste.

Dash matches page-types.c in the same directory, I am happy using
userscore if that is the rigth approach.
> >
> > diff --git a/tools/mm/memory-failure/hwpoison-panic.c b/tools/mm/memory-failure/hwpoison-panic.c
> > new file mode 100644
> > index 0000000000000..b8076d86da15b
> > --- /dev/null
> > +++ b/tools/mm/memory-failure/hwpoison-panic.c
> > @@ -0,0 +1,353 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * hwpoison-panic: verify vm.panic_on_unrecoverable_memory_failure by
> > + * injecting a hwpoison error on a kernel-owned page and confirming the
> > + * kernel panics.
> 
> While reading the above explanation, I was expecting this program will directly
> say me if the kernel panic-ed or not.  But this only says if it was not
> panic-ed iiuc?  How about saying, like, "Trigger intentional kernel panic by
> ..." ?

You read it correctly, the program can only ever report the absence of
a panic. I will reword the header along your suggestion for v2.

> > +	kind = parse_args(argc, argv, &force);
> > +	page_size = getpagesize();
> > +	check_prereqs(force);
> 
> Maybe check_prereqs() can be called before getpagesize()?

Yes, nothing in check_prereqs() needs page_size. Moving it up in v2.

Thanks for the review,
--breno


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv