Re: [PATCH V2 2/2] mm/memblock: Add reserve_mem debugfs info

[SeongJae Park]

On Fri, 13 Mar 2026 18:09:10 -0300 "Guilherme G. Piccoli" <gpiccoli@xxxxxxxxxx> wrote:

> Hi SJ and Mike, thanks for both your review! I'll respond the 2 messages
> here.
> 
> First regarding Mike's concern from the other email:
> 
> >> +#include <linux/string_helpers.h>
> >This will break tests in tools/testing/memblock, please add a stub there.
> 
> Sure, will do it!
> 
> On 04/03/2026 22:44, SeongJae Park wrote:
> >[...]
> >> There is no easy way to determine if this kernel parameter is properly
> >> set though; the kernel doesn't show information about this memory in
> >> memblock debugfs, neither in /proc/iomem nor dmesg. This is a relevant
> >> information for tools like kdumpst[0], to determine if it's reliable
> >> to use the reserved area as ramoops persistent storage; checking only
> >> /proc/cmdline is not sufficient as it doesn't tell if the reservation
> >> effectively succeeded or not.
> > 
> > Asking out of curiosity.  The previous patch has added the error log for
> > failure cases.  Could checking the kernel log to see if it was failed be an
> > option?
> > 
> > I think this debugfs approach is easier to check for the user space, though.
> > If that is the reason of this patch, adding the clarity would be nice for a
> > theoretical case that debugfs cannot be mounted.
> >
> 
> Exactly that! Not only debugfs way is more convenient for checking that,
> but it feels a bit weird for me to assume the reservation worked by
> checking for errors and if none, all fine. It's valid, but as a matter
> of taste, I prefer checking the file. And not only that: imagine we have
> more than one setting in the cmdline, for other usages like ftrace.
> Easier to have the debugfs showing the succeeding ones, by name and
> size, ready to be used heheh

Makes sense to me, thank you for kindly clarifying this! :)

> 
> Do you have a suggestion on how should I mention this in the commit
> message? Is that discussion enough maybe, for future reference? I feel
> mentioning that userspace will use the information from the file seems
> enough for a commit message; but I'm totally open for your suggestions
> on how to improve the wording here

The above clarification on this thread is good enough for me.

> 
> 
> >> +	if (!(IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK) || reserved_mem_count))
> >> +		return 0;
> > 
> > One trivial comment.  I'd slightly prefer having one less parentheses level as
> > a tradeoff of having one more exclamation mark, e.g.,
> > 
> >     if (!IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK) && !reserved_mem_count)
> > 
> 
> Good! Unless Mike is against that, I can easily change it.
> 
> 
> > 
> > So, we get two level of nested ifdef...  I'm wondering if returning earlier
> > when CONFIG_ARCH_KEEP_MEMBLOCK is undefined is easier to read.  E.g.,
> > 
> > 	#ifndef CONFIG_ARCH_KEEP_MEMBLOCK
> > 		return 0;
> > 	#endif
> > 
> > 		debugfs_create_file("memory", 0444, root,
> > 	[...]
> > 
> >>  	return 0;
> > 
> > Very trivial comment.  Why don't you keep the original blank line above the
> > return statemtnt?
> > 
> 
> About the ifdefs, as per Mike's comment, I'll rework it in a helper. And
> the blank like, I have no answer heh
> Likely a braino? I can certainly keep it.

Sounds good!

> 
> Thanks again you two for the suggestions,

:)


Thanks,
SJ

[...]