| Message ID | 20230526120505.123693-1-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-next,v2] memblock: unify memblock dump and debugfs show | expand |
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 ", ®->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 >
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:)
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 ", ®->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);
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(-)