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

[Guilherme G. Piccoli]

Hi Mike, thanks a bunch for your review! Good ideas, I'll change the
implementation to follow the suggestions and improve the changelog, as
you suggested. I have some questions though, that I will comment inline,
below.

First of all, what should we do regarding patch 1? Should I resubmit as
part of V2, even with no changes - or pick it now and I only submit
patch 2 as V2, with changes?



On 21/02/2026 05:52, Mike Rapoport wrote:
> [...]
>> +			string_get_size((u64)(map->size), 1, STRING_UNITS_2, txtsz, 16);
> 
> phys_addr_t should be casted automatically to u64 IMO.
> And please, no magic numbers.

Specifically here, by magic number you mean my choice of 16, right? What
do you suggest me to pick? It's the length of the string carrying the
size of reserved_mem, some number must be selected for this
length...lemme know WDYT.



> [...] 
> You can define "reserve_mem" attribute separately, and leave the existing
> memblock_debug_show() as it was.
> 
>> +	} else
>> +		memblock_debugfs_files(m);
>> +
>>  	return 0;
>>  }
>>  DEFINE_SHOW_ATTRIBUTE(memblock_debug);
>> @@ -2762,6 +2791,9 @@ static int __init memblock_init_debugfs(void)
>>  {
>>  	struct dentry *root = debugfs_create_dir("memblock", NULL);
> 
> No need to create memblock directory in debugfs if there's nothing to show.
> Could be something like
> 
> 	if (i!(IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK) || reserved_mem_count))
> 		return;
> 
> 	root = debugfs_create_dir("memblock", NULL);

Very good suggestions here, but just let me clarify: so I could continue
showing the "reserved_mem_param" inside the "<debugfs>/memblock" folder,
just using a different function for that attribute?

I understood that, based on your (good) suggestion to hide the memblock
folder if ARCH_KEEP_MEMBLOCK is not defined and there is no reserved_mem
set ... just want to confirm to follow-up the implementation.
Cheers,


Guilherme