1. Patchew
  2. linux
  3. View series

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

Kefeng Wang posted 1 patch 10 months, 3 weeks ago 
include/linux/memblock.h |  1 +
mm/memblock.c            | 80 ++++++++++++++++++++--------------------
2 files changed, 42 insertions(+), 39 deletions(-)
[PATCH -next v2] memblock: unify memblock dump and debugfs show
Posted by Kefeng Wang 10 months, 3 weeks ago 
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.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
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
Re: [PATCH -next v2] memblock: unify memblock dump and debugfs show
Posted by Mike Rapoport 10 months, 3 weeks ago 
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@huawei.com>
> ---
> 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.
Re: [PATCH -next v2] memblock: unify memblock dump and debugfs show
Posted by Kefeng Wang 10 months, 3 weeks ago 

On 2023/5/27 18:32, Mike Rapoport wrote:
> 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.

As the debugfs show is similar to memblock_dump, but not exactly the
same, eg,the node info and flag(with/without name), we may want to
support flagname in memblock_dump, also avoid more different changes
in the future. but keep single dump function is fine since they are
not complicated:)

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.