diff mbox series

[3/3] printk: dump full information of page flags in pGp

Message ID 20210128021947.22877-4-laoar.shao@gmail.com (mailing list archive)
State New, archived
Headers show
Series mm, printk: dump full information of page flags in pGp | expand

Commit Message

Yafang Shao Jan. 28, 2021, 2:19 a.m. UTC
Currently the pGp only shows the names of page flags, rather than
the full information including section, node, zone, last cpupid and
kasan tag. While it is not easy to parse these information manually
because there're so many flavors. Let's interpret them in pGp as well.

- Before the patch,
[ 6312.639698] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(slab|head)

- After the patch,
[ 6315.235783] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(Node 0x0,Zone 0x2,Lastcpupid 0x1fffff,slab|head)

Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 lib/vsprintf.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

Comments

Joe Perches Jan. 28, 2021, 2:35 a.m. UTC | #1
On Thu, 2021-01-28 at 10:19 +0800, Yafang Shao wrote:
> Currently the pGp only shows the names of page flags, rather than
> the full information including section, node, zone, last cpupid and
> kasan tag. While it is not easy to parse these information manually
> because there're so many flavors. Let's interpret them in pGp as well.
[]
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -1916,6 +1916,46 @@ char *format_flags(char *buf, char *end, unsigned long flags,
>  	return buf;
>  }
> 
> +struct page_flags_layout {
> +	int width;
> +	int shift;
> +	int mask;
> +	char *name;
> +};
> +
> +struct page_flags_layout pfl[] = {

static const struct page_flags_layout pfl[] = {

> +	{SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, "Section "},
> +	{NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, "Node "},
> +	{ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, "Zone "},
> +	{LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, "Lastcpupid "},
> +	{KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, "Kasantag "},
> +};
> +
> +static
> +char *format_layout(char *buf, char *end, unsigned long flags)

poor name.  perhaps format_page_flags

> +{
> +	int i;
> +
> +	for (i = 0; i < sizeof(pfl) / sizeof(struct page_flags_layout) && buf < end; i++) {

	for (i = 0; i < ARRAY_SIZE(pfl) && buf < end; i++) {


> @@ -1929,7 +1969,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
>  	switch (fmt[1]) {
>  	case 'p':
>  		flags = *(unsigned long *)flags_ptr;
> -		/* Remove zone id */
> +		buf = format_layout(buf, end, flags & ~((1UL << NR_PAGEFLAGS) - 1));
>  		flags &= (1UL << NR_PAGEFLAGS) - 1;

Perhaps store the bitshift into a temp and use the temp twice

		foo = BIT(NR_PAGEFLAGS) - 1;

		buf = format_layout(buf, end, flags & ~foo);
		flags &= foo;
Miaohe Lin Jan. 28, 2021, 2:52 a.m. UTC | #2
Hi:
On 2021/1/28 10:19, Yafang Shao wrote:
> Currently the pGp only shows the names of page flags, rather than
> the full information including section, node, zone, last cpupid and
> kasan tag. While it is not easy to parse these information manually
> because there're so many flavors. Let's interpret them in pGp as well.
> 
> - Before the patch,
> [ 6312.639698] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(slab|head)
> 
> - After the patch,
> [ 6315.235783] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(Node 0x0,Zone 0x2,Lastcpupid 0x1fffff,slab|head)
> 

Thanks. This really helps!

> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  lib/vsprintf.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..bd809f4f1b82 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1916,6 +1916,46 @@ char *format_flags(char *buf, char *end, unsigned long flags,
>  	return buf;
>  }
>  
> +struct page_flags_layout {
> +	int width;
> +	int shift;
> +	int mask;
> +	char *name;

Should we add const for name ?

> +};
> +
> +struct page_flags_layout pfl[] = {

Should we add static const for pfl[] as we won't change its value and use it outside this file ?

> +	{SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, "Section "},
> +	{NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, "Node "},
> +	{ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, "Zone "},
> +	{LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, "Lastcpupid "},
> +	{KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, "Kasantag "},
> +};
> +
> +static
> +char *format_layout(char *buf, char *end, unsigned long flags)
> +{
> +	int i;
> +
> +	for (i = 0; i < sizeof(pfl) / sizeof(struct page_flags_layout) && buf < end; i++) {

I think we can use ARRAY_SIZE here.

> +		if (pfl[i].width == 0)
> +			continue;
> +
> +		buf = string(buf, end, pfl[i].name, default_str_spec);
> +
> +		if (buf >= end)
> +			break;
> +		buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> +			     default_flag_spec);
> +
> +		if (buf >= end)
> +			break;
> +		*buf = ',';
> +		buf++;
> +	}
> +
> +	return buf;
> +}
> +
>  static noinline_for_stack
>  char *flags_string(char *buf, char *end, void *flags_ptr,
>  		   struct printf_spec spec, const char *fmt)
> @@ -1929,7 +1969,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
>  	switch (fmt[1]) {
>  	case 'p':
>  		flags = *(unsigned long *)flags_ptr;
> -		/* Remove zone id */
> +		buf = format_layout(buf, end, flags & ~((1UL << NR_PAGEFLAGS) - 1));
>  		flags &= (1UL << NR_PAGEFLAGS) - 1;
>  		names = pageflag_names;
>  		break;
>
Many thanks.
Yafang Shao Jan. 28, 2021, 7:42 a.m. UTC | #3
On Thu, Jan 28, 2021 at 10:35 AM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2021-01-28 at 10:19 +0800, Yafang Shao wrote:
> > Currently the pGp only shows the names of page flags, rather than
> > the full information including section, node, zone, last cpupid and
> > kasan tag. While it is not easy to parse these information manually
> > because there're so many flavors. Let's interpret them in pGp as well.
> []
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > @@ -1916,6 +1916,46 @@ char *format_flags(char *buf, char *end, unsigned long flags,
> >       return buf;
> >  }
> >
> > +struct page_flags_layout {
> > +     int width;
> > +     int shift;
> > +     int mask;
> > +     char *name;
> > +};
> > +
> > +struct page_flags_layout pfl[] = {
>
> static const struct page_flags_layout pfl[] = {

Sure.

>
> > +     {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, "Section "},
> > +     {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, "Node "},
> > +     {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, "Zone "},
> > +     {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, "Lastcpupid "},
> > +     {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, "Kasantag "},
> > +};
> > +
> > +static
> > +char *format_layout(char *buf, char *end, unsigned long flags)
>
> poor name.  perhaps format_page_flags
>

Thanks for the suggestion.

> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < sizeof(pfl) / sizeof(struct page_flags_layout) && buf < end; i++) {
>
>         for (i = 0; i < ARRAY_SIZE(pfl) && buf < end; i++) {
>

Sure.


>
> > @@ -1929,7 +1969,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
> >       switch (fmt[1]) {
> >       case 'p':
> >               flags = *(unsigned long *)flags_ptr;
> > -             /* Remove zone id */
> > +             buf = format_layout(buf, end, flags & ~((1UL << NR_PAGEFLAGS) - 1));
> >               flags &= (1UL << NR_PAGEFLAGS) - 1;
>
> Perhaps store the bitshift into a temp and use the temp twice
>
>                 foo = BIT(NR_PAGEFLAGS) - 1;
>
>                 buf = format_layout(buf, end, flags & ~foo);
>                 flags &= foo;
>
>

Thanks for the suggestion. I will change them all.
Yafang Shao Jan. 28, 2021, 7:44 a.m. UTC | #4
On Thu, Jan 28, 2021 at 10:52 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> Hi:
> On 2021/1/28 10:19, Yafang Shao wrote:
> > Currently the pGp only shows the names of page flags, rather than
> > the full information including section, node, zone, last cpupid and
> > kasan tag. While it is not easy to parse these information manually
> > because there're so many flavors. Let's interpret them in pGp as well.
> >
> > - Before the patch,
> > [ 6312.639698] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6315.235783] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(Node 0x0,Zone 0x2,Lastcpupid 0x1fffff,slab|head)
> >
>
> Thanks. This really helps!
>
> > Cc: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  lib/vsprintf.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 3b53c73580c5..bd809f4f1b82 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1916,6 +1916,46 @@ char *format_flags(char *buf, char *end, unsigned long flags,
> >       return buf;
> >  }
> >
> > +struct page_flags_layout {
> > +     int width;
> > +     int shift;
> > +     int mask;
> > +     char *name;
>
> Should we add const for name ?
>

Good suggestion.

> > +};
> > +
> > +struct page_flags_layout pfl[] = {
>
> Should we add static const for pfl[] as we won't change its value and use it outside this file ?
>

Sure.

> > +     {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, "Section "},
> > +     {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, "Node "},
> > +     {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, "Zone "},
> > +     {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, "Lastcpupid "},
> > +     {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, "Kasantag "},
> > +};
> > +
> > +static
> > +char *format_layout(char *buf, char *end, unsigned long flags)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < sizeof(pfl) / sizeof(struct page_flags_layout) && buf < end; i++) {
>
> I think we can use ARRAY_SIZE here.
>

Sure.

> > +             if (pfl[i].width == 0)
> > +                     continue;
> > +
> > +             buf = string(buf, end, pfl[i].name, default_str_spec);
> > +
> > +             if (buf >= end)
> > +                     break;
> > +             buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> > +                          default_flag_spec);
> > +
> > +             if (buf >= end)
> > +                     break;
> > +             *buf = ',';
> > +             buf++;
> > +     }
> > +
> > +     return buf;
> > +}
> > +
> >  static noinline_for_stack
> >  char *flags_string(char *buf, char *end, void *flags_ptr,
> >                  struct printf_spec spec, const char *fmt)
> > @@ -1929,7 +1969,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
> >       switch (fmt[1]) {
> >       case 'p':
> >               flags = *(unsigned long *)flags_ptr;
> > -             /* Remove zone id */
> > +             buf = format_layout(buf, end, flags & ~((1UL << NR_PAGEFLAGS) - 1));
> >               flags &= (1UL << NR_PAGEFLAGS) - 1;
> >               names = pageflag_names;
> >               break;
> >
> Many thanks.
Vlastimil Babka Jan. 28, 2021, 10:42 a.m. UTC | #5
On 1/28/21 3:19 AM, Yafang Shao wrote:
> Currently the pGp only shows the names of page flags, rather than
> the full information including section, node, zone, last cpupid and
> kasan tag. While it is not easy to parse these information manually
> because there're so many flavors. Let's interpret them in pGp as well.
> 
> - Before the patch,
> [ 6312.639698] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(slab|head)
> 
> - After the patch,
> [ 6315.235783] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(Node 0x0,Zone 0x2,Lastcpupid 0x1fffff,slab|head)

node, zone could be decimal?
Thanks

> 
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  lib/vsprintf.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..bd809f4f1b82 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1916,6 +1916,46 @@ char *format_flags(char *buf, char *end, unsigned long flags,
>  	return buf;
>  }
>  
> +struct page_flags_layout {
> +	int width;
> +	int shift;
> +	int mask;
> +	char *name;
> +};
> +
> +struct page_flags_layout pfl[] = {
> +	{SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, "Section "},
> +	{NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, "Node "},
> +	{ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, "Zone "},
> +	{LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, "Lastcpupid "},
> +	{KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, "Kasantag "},
> +};
> +
> +static
> +char *format_layout(char *buf, char *end, unsigned long flags)
> +{
> +	int i;
> +
> +	for (i = 0; i < sizeof(pfl) / sizeof(struct page_flags_layout) && buf < end; i++) {
> +		if (pfl[i].width == 0)
> +			continue;
> +
> +		buf = string(buf, end, pfl[i].name, default_str_spec);
> +
> +		if (buf >= end)
> +			break;
> +		buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> +			     default_flag_spec);
> +
> +		if (buf >= end)
> +			break;
> +		*buf = ',';
> +		buf++;
> +	}
> +
> +	return buf;
> +}
> +
>  static noinline_for_stack
>  char *flags_string(char *buf, char *end, void *flags_ptr,
>  		   struct printf_spec spec, const char *fmt)
> @@ -1929,7 +1969,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
>  	switch (fmt[1]) {
>  	case 'p':
>  		flags = *(unsigned long *)flags_ptr;
> -		/* Remove zone id */
> +		buf = format_layout(buf, end, flags & ~((1UL << NR_PAGEFLAGS) - 1));
>  		flags &= (1UL << NR_PAGEFLAGS) - 1;
>  		names = pageflag_names;
>  		break;
>
Andy Shevchenko Jan. 28, 2021, 12:11 p.m. UTC | #6
On Thu, Jan 28, 2021 at 10:19:47AM +0800, Yafang Shao wrote:
> Currently the pGp only shows the names of page flags, rather than
> the full information including section, node, zone, last cpupid and
> kasan tag. While it is not easy to parse these information manually
> because there're so many flavors. Let's interpret them in pGp as well.
> 
> - Before the patch,
> [ 6312.639698] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(slab|head)
> 
> - After the patch,
> [ 6315.235783] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(Node 0x0,Zone 0x2,Lastcpupid 0x1fffff,slab|head)

> +	int i;
> +
> +	for (i = 0; i < sizeof(pfl) / sizeof(struct page_flags_layout) && buf < end; i++) {

'buf < end' is redundant.

> +		if (pfl[i].width == 0)
> +			continue;
> +
> +		buf = string(buf, end, pfl[i].name, default_str_spec);

> +		if (buf >= end)
> +			break;

Can you rather use usual patter, i.e.

	if (buf < end) {
		...do something...
	}
	buf++; // or whatever increase should be done

Moreover, number() and string() IIRC have the proper checks embedded into them.

> +		buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> +			     default_flag_spec);

> +		if (buf >= end)
> +			break;
> +		*buf = ',';
> +		buf++;

Here is a very standard pattern can be used, see code around

		if (buf < end)
			*buf = ',';
		buf++;

> +	}
Yafang Shao Jan. 28, 2021, 1:07 p.m. UTC | #7
On Thu, Jan 28, 2021 at 6:42 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 1/28/21 3:19 AM, Yafang Shao wrote:
> > Currently the pGp only shows the names of page flags, rather than
> > the full information including section, node, zone, last cpupid and
> > kasan tag. While it is not easy to parse these information manually
> > because there're so many flavors. Let's interpret them in pGp as well.
> >
> > - Before the patch,
> > [ 6312.639698] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6315.235783] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(Node 0x0,Zone 0x2,Lastcpupid 0x1fffff,slab|head)
>
> node, zone could be decimal?

Make sense to me. Will change it.

>
> >
> > Cc: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  lib/vsprintf.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 3b53c73580c5..bd809f4f1b82 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1916,6 +1916,46 @@ char *format_flags(char *buf, char *end, unsigned long flags,
> >       return buf;
> >  }
> >
> > +struct page_flags_layout {
> > +     int width;
> > +     int shift;
> > +     int mask;
> > +     char *name;
> > +};
> > +
> > +struct page_flags_layout pfl[] = {
> > +     {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, "Section "},
> > +     {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, "Node "},
> > +     {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, "Zone "},
> > +     {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, "Lastcpupid "},
> > +     {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, "Kasantag "},
> > +};
> > +
> > +static
> > +char *format_layout(char *buf, char *end, unsigned long flags)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < sizeof(pfl) / sizeof(struct page_flags_layout) && buf < end; i++) {
> > +             if (pfl[i].width == 0)
> > +                     continue;
> > +
> > +             buf = string(buf, end, pfl[i].name, default_str_spec);
> > +
> > +             if (buf >= end)
> > +                     break;
> > +             buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> > +                          default_flag_spec);
> > +
> > +             if (buf >= end)
> > +                     break;
> > +             *buf = ',';
> > +             buf++;
> > +     }
> > +
> > +     return buf;
> > +}
> > +
> >  static noinline_for_stack
> >  char *flags_string(char *buf, char *end, void *flags_ptr,
> >                  struct printf_spec spec, const char *fmt)
> > @@ -1929,7 +1969,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
> >       switch (fmt[1]) {
> >       case 'p':
> >               flags = *(unsigned long *)flags_ptr;
> > -             /* Remove zone id */
> > +             buf = format_layout(buf, end, flags & ~((1UL << NR_PAGEFLAGS) - 1));
> >               flags &= (1UL << NR_PAGEFLAGS) - 1;
> >               names = pageflag_names;
> >               break;
> >
>
Yafang Shao Jan. 28, 2021, 1:18 p.m. UTC | #8
On Thu, Jan 28, 2021 at 8:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jan 28, 2021 at 10:19:47AM +0800, Yafang Shao wrote:
> > Currently the pGp only shows the names of page flags, rather than
> > the full information including section, node, zone, last cpupid and
> > kasan tag. While it is not easy to parse these information manually
> > because there're so many flavors. Let's interpret them in pGp as well.
> >
> > - Before the patch,
> > [ 6312.639698] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6315.235783] ERR: Slab 0x000000006d1133b9 objects=33 used=3 fp=0x000000006d0779d1 flags=0x17ffffc0010200(Node 0x0,Zone 0x2,Lastcpupid 0x1fffff,slab|head)
>
> > +     int i;
> > +
> > +     for (i = 0; i < sizeof(pfl) / sizeof(struct page_flags_layout) && buf < end; i++) {
>
> 'buf < end' is redundant.
>

Thanks for pointing this out.

> > +             if (pfl[i].width == 0)
> > +                     continue;
> > +
> > +             buf = string(buf, end, pfl[i].name, default_str_spec);
>
> > +             if (buf >= end)
> > +                     break;
>
> Can you rather use usual patter, i.e.
>
>         if (buf < end) {
>                 ...do something...
>         }
>         buf++; // or whatever increase should be done
>
> Moreover, number() and string() IIRC have the proper checks embedded into them.
>

I will take a look at the detail in these two functions.

> > +             buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> > +                          default_flag_spec);
>
> > +             if (buf >= end)
> > +                     break;
> > +             *buf = ',';
> > +             buf++;
>
> Here is a very standard pattern can be used, see code around
>
>                 if (buf < end)
>                         *buf = ',';
>                 buf++;
>
> > +     }
>

Thanks for the explanation.
I will change it as you suggested.
Andy Shevchenko Jan. 28, 2021, 2:50 p.m. UTC | #9
On Thu, Jan 28, 2021 at 09:18:24PM +0800, Yafang Shao wrote:
> On Thu, Jan 28, 2021 at 8:11 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> Thanks for the explanation.
> I will change it as you suggested.

You are welcome!

Just note, that kasprintf() should work for this as well as for the rest
printf() cases. That's why it's very important to return proper buffer pointer.

Also read `man snprintf(3)` RETURN VALUE section to understand it.
Yafang Shao Jan. 29, 2021, 1:21 a.m. UTC | #10
On Thu, Jan 28, 2021 at 10:50 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jan 28, 2021 at 09:18:24PM +0800, Yafang Shao wrote:
> > On Thu, Jan 28, 2021 at 8:11 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
>
> ...
>
> > Thanks for the explanation.
> > I will change it as you suggested.
>
> You are welcome!
>
> Just note, that kasprintf() should work for this as well as for the rest
> printf() cases. That's why it's very important to return proper buffer pointer.
>
> Also read `man snprintf(3)` RETURN VALUE section to understand it.
>

Thanks for the information. I will take a look at it.
diff mbox series

Patch

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..bd809f4f1b82 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1916,6 +1916,46 @@  char *format_flags(char *buf, char *end, unsigned long flags,
 	return buf;
 }
 
+struct page_flags_layout {
+	int width;
+	int shift;
+	int mask;
+	char *name;
+};
+
+struct page_flags_layout pfl[] = {
+	{SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, "Section "},
+	{NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, "Node "},
+	{ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, "Zone "},
+	{LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, "Lastcpupid "},
+	{KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, "Kasantag "},
+};
+
+static
+char *format_layout(char *buf, char *end, unsigned long flags)
+{
+	int i;
+
+	for (i = 0; i < sizeof(pfl) / sizeof(struct page_flags_layout) && buf < end; i++) {
+		if (pfl[i].width == 0)
+			continue;
+
+		buf = string(buf, end, pfl[i].name, default_str_spec);
+
+		if (buf >= end)
+			break;
+		buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
+			     default_flag_spec);
+
+		if (buf >= end)
+			break;
+		*buf = ',';
+		buf++;
+	}
+
+	return buf;
+}
+
 static noinline_for_stack
 char *flags_string(char *buf, char *end, void *flags_ptr,
 		   struct printf_spec spec, const char *fmt)
@@ -1929,7 +1969,7 @@  char *flags_string(char *buf, char *end, void *flags_ptr,
 	switch (fmt[1]) {
 	case 'p':
 		flags = *(unsigned long *)flags_ptr;
-		/* Remove zone id */
+		buf = format_layout(buf, end, flags & ~((1UL << NR_PAGEFLAGS) - 1));
 		flags &= (1UL << NR_PAGEFLAGS) - 1;
 		names = pageflag_names;
 		break;