diff mbox series

[v2] memblock: Add flags and nid info in memblock debugfs

Message ID	20230517025747.230-1-ssawgyw@gmail.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Yuwei Guan <ssawgyw@gmail.com> To: rppt@kernel.org, akpm@linux-foundation.org, tsahu@linux.ibm.com Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Yuwei Guan <ssawgyw@gmail.com> Subject: [PATCH v2] memblock: Add flags and nid info in memblock debugfs Date: Wed, 17 May 2023 10:57:47 +0800 Message-Id: <20230517025747.230-1-ssawgyw@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	[v2] memblock: Add flags and nid info in memblock debugfs \| [v2] memblock: Add flags and nid info in memblock debugfs

Commit Message

Yuwei Guan May 17, 2023, 2:57 a.m. UTC

  Currently, the memblock debugfs can display the count of memblock_type and
the base and end of the reg. However, when the following scenario occurs,
the information in the existing debugfs cannot make it clear why the
address is not consecutive.

For example,
cat /sys/kernel/debug/memblock/memory
   0: 0x0000000080000000..0x00000000901fffff
   1: 0x0000000090200000..0x00000000905fffff
   2: 0x0000000090600000..0x0000000092ffffff
   3: 0x0000000093000000..0x00000000973fffff
   4: 0x0000000097400000..0x00000000b71fffff
   5: 0x00000000c0000000..0x00000000dfffffff
   6: 0x00000000e2500000..0x00000000f87fffff
   7: 0x00000000f8800000..0x00000000fa7fffff
   8: 0x00000000fa800000..0x00000000fd3effff
   9: 0x00000000fd3f0000..0x00000000fd3fefff
  10: 0x00000000fd3ff000..0x00000000fd7fffff
  11: 0x00000000fd800000..0x00000000fd901fff
  12: 0x00000000fd902000..0x00000000fd909fff
  13: 0x00000000fd90a000..0x00000000fd90bfff
  14: 0x00000000fd90c000..0x00000000ffffffff
  15: 0x0000000880000000..0x0000000affffffff

So we can add flags and nid to this debugfs.

For example,
cat /sys/kernel/debug/memblock/memory
cnt     base..end       flags   nid
0:      0x0000000080000000..0x00000000901fffff  0x0     0x0
1:      0x0000000090200000..0x00000000905fffff  0x4     0x0
2:      0x0000000090600000..0x0000000092ffffff  0x0     0x0
3:      0x0000000093000000..0x00000000973fffff  0x4     0x0
4:      0x0000000097400000..0x00000000b71fffff  0x0     0x0
5:      0x00000000c0000000..0x00000000dfffffff  0x0     0x0
6:      0x00000000e2500000..0x00000000f87fffff  0x0     0x0
7:      0x00000000f8800000..0x00000000fa7fffff  0x4     0x0
8:      0x00000000fa800000..0x00000000fd3effff  0x0     0x0
9:      0x00000000fd3f0000..0x00000000fd3fefff  0x4     0x0
10:     0x00000000fd3ff000..0x00000000fd7fffff  0x0     0x0
11:     0x00000000fd800000..0x00000000fd901fff  0x4     0x0
12:     0x00000000fd902000..0x00000000fd909fff  0x0     0x0
13:     0x00000000fd90a000..0x00000000fd90bfff  0x4     0x0
14:     0x00000000fd90c000..0x00000000ffffffff  0x0     0x0
15:     0x0000000880000000..0x0000000affffffff  0x0     0x0

Signed-off-by: Yuwei Guan <ssawgyw@gmail.com>
Reviewed-by: Tarun Sahu <tsahu@linux.ibm.com>
---
 mm/memblock.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Anshuman Khandual May 17, 2023, 6:07 a.m. UTC | #1

On 5/17/23 08:27, Yuwei Guan wrote:
> Currently, the memblock debugfs can display the count of memblock_type and
> the base and end of the reg. However, when the following scenario occurs,

scenarios where the memblock flags or nid varies inside a single PA range ?
I guess the commit message description here can be improved to accommodate
such details.

> the information in the existing debugfs cannot make it clear why the
> address is not consecutive.
> 
> For example,
> cat /sys/kernel/debug/memblock/memory
>    0: 0x0000000080000000..0x00000000901fffff
>    1: 0x0000000090200000..0x00000000905fffff
>    2: 0x0000000090600000..0x0000000092ffffff
>    3: 0x0000000093000000..0x00000000973fffff
>    4: 0x0000000097400000..0x00000000b71fffff
>    5: 0x00000000c0000000..0x00000000dfffffff
>    6: 0x00000000e2500000..0x00000000f87fffff
>    7: 0x00000000f8800000..0x00000000fa7fffff
>    8: 0x00000000fa800000..0x00000000fd3effff
>    9: 0x00000000fd3f0000..0x00000000fd3fefff
>   10: 0x00000000fd3ff000..0x00000000fd7fffff
>   11: 0x00000000fd800000..0x00000000fd901fff
>   12: 0x00000000fd902000..0x00000000fd909fff
>   13: 0x00000000fd90a000..0x00000000fd90bfff
>   14: 0x00000000fd90c000..0x00000000ffffffff
>   15: 0x0000000880000000..0x0000000affffffff
> 
> So we can add flags and nid to this debugfs.
> 
> For example,
> cat /sys/kernel/debug/memblock/memory
> cnt     base..end       flags   nid

These markers ^^^ are not aligned properly, and also might not be
required as well.

> 0:      0x0000000080000000..0x00000000901fffff  0x0     0x0
> 1:      0x0000000090200000..0x00000000905fffff  0x4     0x0
> 2:      0x0000000090600000..0x0000000092ffffff  0x0     0x0
> 3:      0x0000000093000000..0x00000000973fffff  0x4     0x0
> 4:      0x0000000097400000..0x00000000b71fffff  0x0     0x0
> 5:      0x00000000c0000000..0x00000000dfffffff  0x0     0x0
> 6:      0x00000000e2500000..0x00000000f87fffff  0x0     0x0
> 7:      0x00000000f8800000..0x00000000fa7fffff  0x4     0x0
> 8:      0x00000000fa800000..0x00000000fd3effff  0x0     0x0
> 9:      0x00000000fd3f0000..0x00000000fd3fefff  0x4     0x0
> 10:     0x00000000fd3ff000..0x00000000fd7fffff  0x0     0x0
> 11:     0x00000000fd800000..0x00000000fd901fff  0x4     0x0
> 12:     0x00000000fd902000..0x00000000fd909fff  0x0     0x0
> 13:     0x00000000fd90a000..0x00000000fd90bfff  0x4     0x0
> 14:     0x00000000fd90c000..0x00000000ffffffff  0x0     0x0
> 15:     0x0000000880000000..0x0000000affffffff  0x0     0x0
> 
> Signed-off-by: Yuwei Guan <ssawgyw@gmail.com>
> Reviewed-by: Tarun Sahu <tsahu@linux.ibm.com>
> ---
>  mm/memblock.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 511d4783dcf1..b36fb6b31e0f 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -2144,12 +2144,16 @@ static int memblock_debug_show(struct seq_file *m, void *private)
>  	int i;
>  	phys_addr_t end;
>  
> +	seq_puts(m, "cnt\tbase..end\tflags\tnid\n");

Please drop this.

> +
>  	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\n", &reg->base, &end);
> +		seq_printf(m, "%d:\t", i);

Why drop the existing %4d formatting qualifier ?

> +		seq_printf(m, "%pa..%pa\t", &reg->base, &end);
> +		seq_printf(m, "0x%x\t", reg->flags);

Should there be ORed string values for each memblock flag ?

enum memblock_flags {
        MEMBLOCK_NONE           = 0x0,  /* No special request */
        MEMBLOCK_HOTPLUG        = 0x1,  /* hotpluggable region */
        MEMBLOCK_MIRROR         = 0x2,  /* mirrored region */
        MEMBLOCK_NOMAP          = 0x4,  /* don't add to kernel direct mapping */
        MEMBLOCK_DRIVER_MANAGED = 0x8,  /* always detected via a driver */
};

Something like NN | HT | MR | NM | DM ?

> +		seq_printf(m, "0x%x\n", memblock_get_region_node(reg));
>  	}
>  	return 0;
>  }

Mike Rapoport May 17, 2023, 1:42 p.m. UTC | #2

On Wed, May 17, 2023 at 11:37:25AM +0530, Anshuman Khandual wrote:
> 
> On 5/17/23 08:27, Yuwei Guan wrote:
> > Currently, the memblock debugfs can display the count of memblock_type and
> > the base and end of the reg. However, when the following scenario occurs,
> 
> scenarios where the memblock flags or nid varies inside a single PA range ?
> I guess the commit message description here can be improved to accommodate
> such details.
> 
> > the information in the existing debugfs cannot make it clear why the
> > address is not consecutive.
> > 
> > For example,
> > cat /sys/kernel/debug/memblock/memory
> >    0: 0x0000000080000000..0x00000000901fffff
> >    1: 0x0000000090200000..0x00000000905fffff
> >    2: 0x0000000090600000..0x0000000092ffffff
> >    3: 0x0000000093000000..0x00000000973fffff
> >    4: 0x0000000097400000..0x00000000b71fffff
> >    5: 0x00000000c0000000..0x00000000dfffffff
> >    6: 0x00000000e2500000..0x00000000f87fffff
> >    7: 0x00000000f8800000..0x00000000fa7fffff
> >    8: 0x00000000fa800000..0x00000000fd3effff
> >    9: 0x00000000fd3f0000..0x00000000fd3fefff
> >   10: 0x00000000fd3ff000..0x00000000fd7fffff
> >   11: 0x00000000fd800000..0x00000000fd901fff
> >   12: 0x00000000fd902000..0x00000000fd909fff
> >   13: 0x00000000fd90a000..0x00000000fd90bfff
> >   14: 0x00000000fd90c000..0x00000000ffffffff
> >   15: 0x0000000880000000..0x0000000affffffff
> > 
> > So we can add flags and nid to this debugfs.
> > 
> > For example,
> > cat /sys/kernel/debug/memblock/memory
> > cnt     base..end       flags   nid
> 
> These markers ^^^ are not aligned properly, and also might not be
> required as well.
> 
> > 0:      0x0000000080000000..0x00000000901fffff  0x0     0x0
> > 1:      0x0000000090200000..0x00000000905fffff  0x4     0x0
> > 2:      0x0000000090600000..0x0000000092ffffff  0x0     0x0
> > 3:      0x0000000093000000..0x00000000973fffff  0x4     0x0
> > 4:      0x0000000097400000..0x00000000b71fffff  0x0     0x0
> > 5:      0x00000000c0000000..0x00000000dfffffff  0x0     0x0
> > 6:      0x00000000e2500000..0x00000000f87fffff  0x0     0x0
> > 7:      0x00000000f8800000..0x00000000fa7fffff  0x4     0x0
> > 8:      0x00000000fa800000..0x00000000fd3effff  0x0     0x0
> > 9:      0x00000000fd3f0000..0x00000000fd3fefff  0x4     0x0
> > 10:     0x00000000fd3ff000..0x00000000fd7fffff  0x0     0x0
> > 11:     0x00000000fd800000..0x00000000fd901fff  0x4     0x0
> > 12:     0x00000000fd902000..0x00000000fd909fff  0x0     0x0
> > 13:     0x00000000fd90a000..0x00000000fd90bfff  0x4     0x0
> > 14:     0x00000000fd90c000..0x00000000ffffffff  0x0     0x0
> > 15:     0x0000000880000000..0x0000000affffffff  0x0     0x0
> > 
> > Signed-off-by: Yuwei Guan <ssawgyw@gmail.com>
> > Reviewed-by: Tarun Sahu <tsahu@linux.ibm.com>
> > ---
> >  mm/memblock.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index 511d4783dcf1..b36fb6b31e0f 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -2144,12 +2144,16 @@ static int memblock_debug_show(struct seq_file *m, void *private)
> >  	int i;
> >  	phys_addr_t end;
> >  
> > +	seq_puts(m, "cnt\tbase..end\tflags\tnid\n");
> 
> Please drop this.
> 
> > +
> >  	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\n", &reg->base, &end);
> > +		seq_printf(m, "%d:\t", i);
> 
> Why drop the existing %4d formatting qualifier ?
> 
> > +		seq_printf(m, "%pa..%pa\t", &reg->base, &end);
> > +		seq_printf(m, "0x%x\t", reg->flags);
> 
> Should there be ORed string values for each memblock flag ?
> 
> enum memblock_flags {
>         MEMBLOCK_NONE           = 0x0,  /* No special request */
>         MEMBLOCK_HOTPLUG        = 0x1,  /* hotpluggable region */
>         MEMBLOCK_MIRROR         = 0x2,  /* mirrored region */
>         MEMBLOCK_NOMAP          = 0x4,  /* don't add to kernel direct mapping */
>         MEMBLOCK_DRIVER_MANAGED = 0x8,  /* always detected via a driver */
> };
> 
> Something like NN | HT | MR | NM | DM ?

These are not less cryptic than numbers :)
Most of them are mutually exclusive, so maybe just spell them out fully,
just shorten DRIVER_MANAGED to DRV_MNG?
And make the flags dump the last to keep columns nicely aligned.
 
> > +		seq_printf(m, "0x%x\n", memblock_get_region_node(reg));
> >  	}
> >  	return 0;
> >  }

Mike Rapoport May 17, 2023, 1:43 p.m. UTC | #3

On Wed, May 17, 2023 at 10:57:47AM +0800, Yuwei Guan wrote:
> Currently, the memblock debugfs can display the count of memblock_type and
> the base and end of the reg. However, when the following scenario occurs,
> the information in the existing debugfs cannot make it clear why the
> address is not consecutive.

...
 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 511d4783dcf1..b36fb6b31e0f 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -2144,12 +2144,16 @@ static int memblock_debug_show(struct seq_file *m, void *private)
>  	int i;
>  	phys_addr_t end;
>  
> +	seq_puts(m, "cnt\tbase..end\tflags\tnid\n");
> +
>  	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\n", &reg->base, &end);
> +		seq_printf(m, "%d:\t", i);
> +		seq_printf(m, "%pa..%pa\t", &reg->base, &end);
> +		seq_printf(m, "0x%x\t", reg->flags);
> +		seq_printf(m, "0x%x\n", memblock_get_region_node(reg));

Please use "%4d" for nid.

>  	}
>  	return 0;
>  }
> -- 
> 2.34.1
>

Yuwei Guan May 18, 2023, 5:58 a.m. UTC | #4

Mike Rapoport <rppt@kernel.org> 于2023年5月17日周三 21:42写道：
>
> On Wed, May 17, 2023 at 11:37:25AM +0530, Anshuman Khandual wrote:
> >
> > On 5/17/23 08:27, Yuwei Guan wrote:
> > > Currently, the memblock debugfs can display the count of memblock_type and
> > > the base and end of the reg. However, when the following scenario occurs,
> >
> > scenarios where the memblock flags or nid varies inside a single PA range ?
> > I guess the commit message description here can be improved to accommodate
> > such details.
> >
> > > the information in the existing debugfs cannot make it clear why the
> > > address is not consecutive.
> > >
> > > For example,
> > > cat /sys/kernel/debug/memblock/memory
> > >    0: 0x0000000080000000..0x00000000901fffff
> > >    1: 0x0000000090200000..0x00000000905fffff
> > >    2: 0x0000000090600000..0x0000000092ffffff
> > >    3: 0x0000000093000000..0x00000000973fffff
> > >    4: 0x0000000097400000..0x00000000b71fffff
> > >    5: 0x00000000c0000000..0x00000000dfffffff
> > >    6: 0x00000000e2500000..0x00000000f87fffff
> > >    7: 0x00000000f8800000..0x00000000fa7fffff
> > >    8: 0x00000000fa800000..0x00000000fd3effff
> > >    9: 0x00000000fd3f0000..0x00000000fd3fefff
> > >   10: 0x00000000fd3ff000..0x00000000fd7fffff
> > >   11: 0x00000000fd800000..0x00000000fd901fff
> > >   12: 0x00000000fd902000..0x00000000fd909fff
> > >   13: 0x00000000fd90a000..0x00000000fd90bfff
> > >   14: 0x00000000fd90c000..0x00000000ffffffff
> > >   15: 0x0000000880000000..0x0000000affffffff
> > >
> > > So we can add flags and nid to this debugfs.
> > >
> > > For example,
> > > cat /sys/kernel/debug/memblock/memory
> > > cnt     base..end       flags   nid
> >
> > These markers ^^^ are not aligned properly, and also might not be
> > required as well.
> >
> > > 0:      0x0000000080000000..0x00000000901fffff  0x0     0x0
> > > 1:      0x0000000090200000..0x00000000905fffff  0x4     0x0
> > > 2:      0x0000000090600000..0x0000000092ffffff  0x0     0x0
> > > 3:      0x0000000093000000..0x00000000973fffff  0x4     0x0
> > > 4:      0x0000000097400000..0x00000000b71fffff  0x0     0x0
> > > 5:      0x00000000c0000000..0x00000000dfffffff  0x0     0x0
> > > 6:      0x00000000e2500000..0x00000000f87fffff  0x0     0x0
> > > 7:      0x00000000f8800000..0x00000000fa7fffff  0x4     0x0
> > > 8:      0x00000000fa800000..0x00000000fd3effff  0x0     0x0
> > > 9:      0x00000000fd3f0000..0x00000000fd3fefff  0x4     0x0
> > > 10:     0x00000000fd3ff000..0x00000000fd7fffff  0x0     0x0
> > > 11:     0x00000000fd800000..0x00000000fd901fff  0x4     0x0
> > > 12:     0x00000000fd902000..0x00000000fd909fff  0x0     0x0
> > > 13:     0x00000000fd90a000..0x00000000fd90bfff  0x4     0x0
> > > 14:     0x00000000fd90c000..0x00000000ffffffff  0x0     0x0
> > > 15:     0x0000000880000000..0x0000000affffffff  0x0     0x0
> > >
> > > Signed-off-by: Yuwei Guan <ssawgyw@gmail.com>
> > > Reviewed-by: Tarun Sahu <tsahu@linux.ibm.com>
> > > ---
> > >  mm/memblock.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/mm/memblock.c b/mm/memblock.c
> > > index 511d4783dcf1..b36fb6b31e0f 100644
> > > --- a/mm/memblock.c
> > > +++ b/mm/memblock.c
> > > @@ -2144,12 +2144,16 @@ static int memblock_debug_show(struct seq_file *m, void *private)
> > >     int i;
> > >     phys_addr_t end;
> > >
> > > +   seq_puts(m, "cnt\tbase..end\tflags\tnid\n");
> >
> > Please drop this.
> >
> > > +
> > >     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\n", &reg->base, &end);
> > > +           seq_printf(m, "%d:\t", i);
> >
> > Why drop the existing %4d formatting qualifier ?
> >
> > > +           seq_printf(m, "%pa..%pa\t", &reg->base, &end);
> > > +           seq_printf(m, "0x%x\t", reg->flags);
> >
> > Should there be ORed string values for each memblock flag ?
> >
> > enum memblock_flags {
> >         MEMBLOCK_NONE           = 0x0,  /* No special request */
> >         MEMBLOCK_HOTPLUG        = 0x1,  /* hotpluggable region */
> >         MEMBLOCK_MIRROR         = 0x2,  /* mirrored region */
> >         MEMBLOCK_NOMAP          = 0x4,  /* don't add to kernel direct mapping */
> >         MEMBLOCK_DRIVER_MANAGED = 0x8,  /* always detected via a driver */
> > };
> >
> > Something like NN | HT | MR | NM | DM ?
>
> These are not less cryptic than numbers :)
> Most of them are mutually exclusive, so maybe just spell them out fully,
> just shorten DRIVER_MANAGED to DRV_MNG?
> And make the flags dump the last to keep columns nicely aligned.
>

Ok, I will update them in the v3 patch soon.

Thanks
> > > +           seq_printf(m, "0x%x\n", memblock_get_region_node(reg));
> > >     }
> > >     return 0;
> > >  }
>
> --
> Sincerely yours,
> Mike.

diff mbox series

Patch

diff --git a/mm/memblock.c b/mm/memblock.c
index 511d4783dcf1..b36fb6b31e0f 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -2144,12 +2144,16 @@  static int memblock_debug_show(struct seq_file *m, void *private)
 	int i;
 	phys_addr_t end;
 
+	seq_puts(m, "cnt\tbase..end\tflags\tnid\n");
+
 	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\n", &reg->base, &end);
+		seq_printf(m, "%d:\t", i);
+		seq_printf(m, "%pa..%pa\t", &reg->base, &end);
+		seq_printf(m, "0x%x\t", reg->flags);
+		seq_printf(m, "0x%x\n", memblock_get_region_node(reg));
 	}
 	return 0;
 }