Re: [PATCH -next v2] memblock: unify memblock dump and debugfs show

[Mike Rapoport]

Hi Kefeng,

On Fri, May 26, 2023 at 08:05:05PM +0800, Kefeng Wang wrote:
> There are two interfaces to show the memblock information, memblock_dump_all()
> and /sys/kernel/debug/memblock/, but the content is displayed separately,
> let's unify them in case of more different changes over time.
 
I don't see much value in this unifications, especially as it must change
the format of one of the dumps. Although these are not ABIs, keeping the
existing formats seems more important to me that having a single dump
function.

> Signed-off-by: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
> ---
> v2:
> - fix wrong count since we add MEMBLOCK_MAX_UNKNOWN
> - drop __initdata_memblock for flagname
> 
>  include/linux/memblock.h |  1 +
>  mm/memblock.c            | 80 ++++++++++++++++++++--------------------
>  2 files changed, 42 insertions(+), 39 deletions(-)
> 
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index f82ee3fac1cd..d68826e8c97b 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -47,6 +47,7 @@ enum memblock_flags {
>  	MEMBLOCK_MIRROR		= 0x2,	/* mirrored region */
>  	MEMBLOCK_NOMAP		= 0x4,	/* don't add to kernel direct mapping */
>  	MEMBLOCK_DRIVER_MANAGED = 0x8,	/* always detected via a driver */
> +	MEMBLOCK_MAX_UNKNOWN	= 0x10,	/* unknow flags */
>  };
>  
>  /**
> diff --git a/mm/memblock.c b/mm/memblock.c
> index c5c80d9bcea3..04eb7c665026 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1899,16 +1899,35 @@ phys_addr_t __init_memblock memblock_get_current_limit(void)
>  	return memblock.current_limit;
>  }
>  
> -static void __init_memblock memblock_dump(struct memblock_type *type)
> +#define memblock_printf(m, to_dmesg, fmt, args...)		\
> +({								\
> +	if (to_dmesg)						\
> +		pr_info(fmt, ##args);				\
> +	else							\
> +		seq_printf(m, fmt, ##args);			\
> +})
> +
> +static const char * const flagname[] = {
> +	[ilog2(MEMBLOCK_HOTPLUG)]		= "HOTPLUG",
> +	[ilog2(MEMBLOCK_MIRROR)]		= "MIRROR",
> +	[ilog2(MEMBLOCK_NOMAP)]			= "NOMAP",
> +	[ilog2(MEMBLOCK_DRIVER_MANAGED)]	= "DRV_MNG",
> +	[ilog2(MEMBLOCK_MAX_UNKNOWN)]		= "UNKNOWN",
> +};
> +
> +static void __init_memblock memblock_dump(struct memblock_type *type,
> +					  struct seq_file *m, bool to_dmesg)
>  {
> +	unsigned count = ARRAY_SIZE(flagname) - 1;
>  	phys_addr_t base, end, size;
>  	enum memblock_flags flags;
> -	int idx;
>  	struct memblock_region *rgn;
> +	int idx, i;
>  
> -	pr_info(" %s.cnt  = 0x%lx\n", type->name, type->cnt);
> +	memblock_printf(m, to_dmesg, " %s.cnt  = 0x%lx\n", type->name, type->cnt);
>  
>  	for_each_memblock_type(idx, type, rgn) {
> +		const char *fp = "NONE";
>  		char nid_buf[32] = "";
>  
>  		base = rgn->base;
> @@ -1920,8 +1939,19 @@ static void __init_memblock memblock_dump(struct memblock_type *type)
>  			snprintf(nid_buf, sizeof(nid_buf), " on node %d",
>  				 memblock_get_region_node(rgn));
>  #endif
> -		pr_info(" %s[%#x]\t[%pa-%pa], %pa bytes%s flags: %#x\n",
> -			type->name, idx, &base, &end, &size, nid_buf, flags);
> +		if (flags) {
> +			fp = flagname[count];
> +
> +			for (i = 0; i < count; i++) {
> +				if (flags & (1U << i)) {
> +					fp = flagname[i];
> +					break;
> +				}
> +			}
> +		}
> +
> +		memblock_printf(m, to_dmesg, " %s[%#x]\t[%pa-%pa], %pa bytes%s flags: %s\n",
> +			type->name, idx, &base, &end, &size, nid_buf, fp);
>  	}
>  }
>  
> @@ -1932,10 +1962,10 @@ static void __init_memblock __memblock_dump_all(void)
>  		&memblock.memory.total_size,
>  		&memblock.reserved.total_size);
>  
> -	memblock_dump(&memblock.memory);
> -	memblock_dump(&memblock.reserved);
> +	memblock_dump(&memblock.memory, NULL, true);
> +	memblock_dump(&memblock.reserved, NULL, true);
>  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
> -	memblock_dump(&physmem);
> +	memblock_dump(&physmem, NULL, true);
>  #endif
>  }
>  
> @@ -2158,41 +2188,13 @@ void __init memblock_free_all(void)
>  }
>  
>  #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK)
> -static const char * const flagname[] = {
> -	[ilog2(MEMBLOCK_HOTPLUG)] = "HOTPLUG",
> -	[ilog2(MEMBLOCK_MIRROR)] = "MIRROR",
> -	[ilog2(MEMBLOCK_NOMAP)] = "NOMAP",
> -	[ilog2(MEMBLOCK_DRIVER_MANAGED)] = "DRV_MNG",
> -};
>  
>  static int memblock_debug_show(struct seq_file *m, void *private)
>  {
>  	struct memblock_type *type = m->private;
> -	struct memblock_region *reg;
> -	int i, j;
> -	unsigned int count = ARRAY_SIZE(flagname);
> -	phys_addr_t end;
> -
> -	for (i = 0; i < type->cnt; i++) {
> -		reg = &type->regions[i];
> -		end = reg->base + reg->size - 1;
> -
> -		seq_printf(m, "%4d: ", i);
> -		seq_printf(m, "%pa..%pa ", &reg->base, &end);
> -		seq_printf(m, "%4d ", memblock_get_region_node(reg));
> -		if (reg->flags) {
> -			for (j = 0; j < count; j++) {
> -				if (reg->flags & (1U << j)) {
> -					seq_printf(m, "%s\n", flagname[j]);
> -					break;
> -				}
> -			}
> -			if (j == count)
> -				seq_printf(m, "%s\n", "UNKNOWN");
> -		} else {
> -			seq_printf(m, "%s\n", "NONE");
> -		}
> -	}
> +
> +	memblock_dump(type, m, false);
> +
>  	return 0;
>  }
>  DEFINE_SHOW_ATTRIBUTE(memblock_debug);
> -- 
> 2.35.3
> 

-- 
Sincerely yours,
Mike.