[PATCH v2 0/3] mm, vsprintf: dump full information of page flags in pGp

[PATCH v2 0/3] mm, vsprintf: dump full information of page flags in pGp

[Yafang Shao]

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.

This patchset also includes some code cleanup in mm/slub.c.

Below is the example of the output in mm/slub.c.
- Before the patchset
[ 6155.716018] INFO: Slab 0x000000004027dd4f objects=33 used=3 fp=0x000000008cd1579c flags=0x17ffffc0010200

- After the patchset
[ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)

The documentation and test cases of pGp are also updated. 

Below is the result of the test_printf,
[ 5091.307308] test_printf: loaded.
[ 5091.308285] test_printf: all 388 tests passed
[ 5091.309105] test_printf: unloaded.

v2:
- various coding improvement, per Joe, Miaohe, Vlastimil and Andy
- remove the prefix completely in patch #2, per Vlastimil
- Update the test cases, per Andy

Yafang Shao (3):
  mm, slub: use pGp to print page flags
  mm, slub: don't combine pr_err with INFO
  vsprintf: dump full information of page flags in pGp

 Documentation/core-api/printk-formats.rst |  2 +-
 lib/test_printf.c                         | 65 ++++++++++++++++++-----
 lib/vsprintf.c                            | 58 +++++++++++++++++++-
 mm/slub.c                                 | 13 ++---
 4 files changed, 116 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] mm, slub: use pGp to print page flags

[Yafang Shao]

As pGp has been already introduced in printk, we'd better use it to make
the output human readable.

Before this change, the output is,
[ 6155.716018] INFO: Slab 0x000000004027dd4f objects=33 used=3 fp=0x000000008cd1579c flags=0x17ffffc0010200

While after this change, the output is,
[ 8846.517809] INFO: Slab 0x00000000f42a2c60 objects=33 used=3 fp=0x0000000060d32ca8 flags=0x17ffffc0010200(slab|head)

Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: David Rientjes <rientjes@google.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 mm/slub.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 34dcc09e2ec9..87ff086e68a4 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -638,8 +638,9 @@ void print_tracking(struct kmem_cache *s, void *object)
 
 static void print_page_info(struct page *page)
 {
-	pr_err("INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=0x%04lx\n",
-	       page, page->objects, page->inuse, page->freelist, page->flags);
+	pr_err("INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n",
+	       page, page->objects, page->inuse, page->freelist,
+	       page->flags, &page->flags);
 
 }
 
-- 
2.17.1

[PATCH v2 2/3] mm, slub: don't combine pr_err with INFO

[Yafang Shao]

It is strange to combine "pr_err" with "INFO", so let's remove the
prefix completely.
This patch is motivated by David's comment[1].

- before the patch
[ 8846.517809] INFO: Slab 0x00000000f42a2c60 objects=33 used=3 fp=0x0000000060d32ca8 flags=0x17ffffc0010200(slab|head)

- after the patch
[ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)

[1]. https://lore.kernel.org/linux-mm/b9c0f2b6-e9b0-0c36-ebdd-2bc684c5a762@redhat.com/#t

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 mm/slub.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 87ff086e68a4..2514c37ab4e4 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -612,7 +612,7 @@ static void print_track(const char *s, struct track *t, unsigned long pr_time)
 	if (!t->addr)
 		return;
 
-	pr_err("INFO: %s in %pS age=%lu cpu=%u pid=%d\n",
+	pr_err("%s in %pS age=%lu cpu=%u pid=%d\n",
 	       s, (void *)t->addr, pr_time - t->when, t->cpu, t->pid);
 #ifdef CONFIG_STACKTRACE
 	{
@@ -638,7 +638,7 @@ void print_tracking(struct kmem_cache *s, void *object)
 
 static void print_page_info(struct page *page)
 {
-	pr_err("INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n",
+	pr_err("Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n",
 	       page, page->objects, page->inuse, page->freelist,
 	       page->flags, &page->flags);
 
@@ -695,7 +695,7 @@ static void print_trailer(struct kmem_cache *s, struct page *page, u8 *p)
 
 	print_page_info(page);
 
-	pr_err("INFO: Object 0x%p @offset=%tu fp=0x%p\n\n",
+	pr_err("Object 0x%p @offset=%tu fp=0x%p\n\n",
 	       p, p - addr, get_freepointer(s, p));
 
 	if (s->flags & SLAB_RED_ZONE)
@@ -788,7 +788,7 @@ static int check_bytes_and_report(struct kmem_cache *s, struct page *page,
 		end--;
 
 	slab_bug(s, "%s overwritten", what);
-	pr_err("INFO: 0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
+	pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
 					fault, end - 1, fault - addr,
 					fault[0], value);
 	print_trailer(s, page, object);
@@ -3854,7 +3854,7 @@ static void list_slab_objects(struct kmem_cache *s, struct page *page,
 	for_each_object(p, s, addr, page->objects) {
 
 		if (!test_bit(__obj_to_index(s, addr, p), map)) {
-			pr_err("INFO: Object 0x%p @offset=%tu\n", p, p - addr);
+			pr_err("Object 0x%p @offset=%tu\n", p, p - addr);
 			print_tracking(s, p);
 		}
 	}
-- 
2.17.1

[PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Yafang Shao]

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,
[ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)

- After the patch,
[ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)

The Documentation and test cases are also updated.

Cc: David Hildenbrand <david@redhat.com>
Cc: Joe Perches <joe@perches.com>
Cc: Miaohe Lin <linmiaohe@huawei.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 Documentation/core-api/printk-formats.rst |  2 +-
 lib/test_printf.c                         | 65 ++++++++++++++++++-----
 lib/vsprintf.c                            | 58 +++++++++++++++++++-
 3 files changed, 109 insertions(+), 16 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 6d26c5c6ac48..1374cdd04f0f 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -538,7 +538,7 @@ Flags bitfields such as page flags, gfp_flags
 
 ::
 
-	%pGp	referenced|uptodate|lru|active|private
+	%pGp	Node 0,Zone 2,referenced|uptodate|lru|active|private
 	%pGg	GFP_USER|GFP_DMA32|GFP_NOWARN
 	%pGv	read|exec|mayread|maywrite|mayexec|denywrite
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7ac87f18a10f..4c5e064cbe2e 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -569,6 +569,48 @@ netdev_features(void)
 {
 }
 
+static void __init
+page_flags_test(int section, int node, int zone, int last_cpupid,
+		int kasan_tag, int flags, const char *name, char *cmp_buf)
+{
+	unsigned long page_flags = 0;
+	unsigned long size = 0;
+
+#ifdef SECTION_IN_PAGE_FLAGS
+	page_flags |= (sec & SECTIONS_MASK) << SECTIONS_PGSHIFT;
+	snprintf(cmp_buf, BUF_SIZE, "Section %#x,", sec);
+	size = strlen(cmp_buf);
+#endif
+
+	page_flags |= (node & NODES_MASK) << NODES_PGSHIFT;
+	snprintf(cmp_buf + size, BUF_SIZE - size, "Node %d", node);
+	size = strlen(cmp_buf);
+
+	page_flags |= (zone & ZONES_MASK) << ZONES_PGSHIFT;
+	snprintf(cmp_buf + size, BUF_SIZE - size, ",Zone %d", zone);
+	size = strlen(cmp_buf);
+
+#ifndef LAST_CPUPID_NOT_IN_PAGE_FLAGS
+	page_flags |= (last_cpupid & LAST_CPUPID_MASK) << LAST_CPUPID_PGSHIFT;
+	snprintf(cmp_buf + size, BUF_SIZE - size, ",Lastcpupid %#x", last_cpupid);
+	size = strlen(cmp_buf);
+#endif
+
+#if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS)
+	page_flags |= (tag & KASAN_TAG_MASK) << KASAN_TAG_PGSHIFT;
+	snprintf(cmp_buf + size, BUF_SIZE - size, ",Kasantag %#x", tag);
+	size = strlen(cmp_buf);
+#endif
+
+	test(cmp_buf, "%pGp", &page_flags);
+
+	if (flags) {
+		page_flags |= flags;
+		snprintf(cmp_buf + size, BUF_SIZE - size, ",%s", name);
+		test(cmp_buf, "%pGp", &page_flags);
+	}
+}
+
 static void __init
 flags(void)
 {
@@ -576,17 +618,16 @@ flags(void)
 	gfp_t gfp;
 	char *cmp_buffer;
 
-	flags = 0;
-	test("", "%pGp", &flags);
-
-	/* Page flags should filter the zone id */
-	flags = 1UL << NR_PAGEFLAGS;
-	test("", "%pGp", &flags);
-
-	flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
-		| 1UL << PG_active | 1UL << PG_swapbacked;
-	test("uptodate|dirty|lru|active|swapbacked", "%pGp", &flags);
+	cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
+	if (!cmp_buffer)
+		return;
 
+	page_flags_test(0, 0, 0, 0, 0, 0, NULL, cmp_buffer);
+	page_flags_test(1, 1, 1, 0x1ffff, 1,
+			(1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
+			 | 1UL << PG_active | 1UL << PG_swapbacked),
+			"uptodate|dirty|lru|active|swapbacked",
+			cmp_buffer);
 
 	flags = VM_READ | VM_EXEC | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC
 			| VM_DENYWRITE;
@@ -601,10 +642,6 @@ flags(void)
 	gfp = __GFP_ATOMIC;
 	test("__GFP_ATOMIC", "%pGg", &gfp);
 
-	cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
-	if (!cmp_buffer)
-		return;
-
 	/* Any flags not translated by the table should remain numeric */
 	gfp = ~__GFP_BITS_MASK;
 	snprintf(cmp_buffer, BUF_SIZE, "%#lx", (unsigned long) gfp);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 14c9a6af1b23..5c2b02ad60f1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1916,6 +1916,62 @@ char *format_flags(char *buf, char *end, unsigned long flags,
 	return buf;
 }
 
+struct page_flags_layout {
+	int width;
+	int shift;
+	int mask;
+	const struct printf_spec *spec;
+	const char *name;
+};
+
+static const struct page_flags_layout pfl[] = {
+	{SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
+	 &default_dec_spec, "Section "},
+	{NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
+	 &default_dec_spec, "Node "},
+	{ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
+	 &default_dec_spec, "Zone "},
+	{LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
+	 &default_flag_spec, "Lastcpupid "},
+	{KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
+	 &default_flag_spec, "Kasantag "},
+};
+
+static
+char *format_page_flags(char *buf, char *end, unsigned long page_flags)
+{
+	unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1);
+	int size = ARRAY_SIZE(pfl);
+	bool separator = false;
+	int i;
+
+	for (i = 0; i < size; i++) {
+		if (pfl[i].width == 0)
+			continue;
+
+		if (separator) {
+			if (buf < end)
+				*buf = ',';
+			buf++;
+		}
+
+
+		buf = string(buf, end, pfl[i].name, *pfl[i].spec);
+
+		buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask,
+			     *pfl[i].spec);
+		separator = true;
+	}
+
+	if (flags) {
+		if (buf < end)
+			*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 +1985,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_page_flags(buf, end, flags);
 		flags &= (1UL << NR_PAGEFLAGS) - 1;
 		names = pageflag_names;
 		break;
-- 
2.17.1

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Joe Perches]

On Mon, 2021-02-01 at 19:56 +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,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> 
> - After the patch,
> [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)

While debugfs is not an ABI, this format is exported in debugfs to
userspace via mm/page_owner.c read_page_owner/print_page_owner.

Does changing the output format matter to anyone?

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[] 
> +static
> +char *format_page_flags(char *buf, char *end, unsigned long page_flags)
> +{
> +	unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1);
> +	int size = ARRAY_SIZE(pfl);

There's no real value in used-once variables.

> +	bool separator = false;
> +	int i;
> +
> +	for (i = 0; i < size; i++) {

Use ARRAY_SIZE here instead

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

> +		if (pfl[i].width == 0)
> +			continue;
> +
> +		if (separator) {
> +			if (buf < end)
> +				*buf = ',';
> +			buf++;
> +		}
> +
> +
> +		buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> +
> +		buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask,
> +			     *pfl[i].spec);
> +		separator = true;
> +	}

Style question:
Might this array be more intelligible with pointers instead of indexes?
Something like:

	struct page_flags_layout *p;

	for (p = pfl; p < pfl + ARRAY_SIZE(pfl); p++) {
		if (p->width == 0)
			continue;

		if (p > pfl) {
			if (buf < end)
				*buf = ',';
			buf++;
		}

		buf = string(buf, end, p->name, *p->spec);
		buf = number(buf, end, (page_flags >> p->shift) & p->mask, *p->spec);
	}

> +
> +	if (flags) {

Maybe:

	if (page_flags & (BIT(NR_PAGEFLAGS) - 1)) {

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

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[David Hildenbrand]

On 01.02.21 12:56, 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,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> 
> - After the patch,
> [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)

For debugging purposes, it might be helpful to have the actual zone name 
(and to know if the value is sane). You could obtain it (without other 
modifications) via

const char zname = "Invalid";

if (zone < MAX_NR_ZONES)
	zname = first_online_pgdat()->node_zones[zone].name;


Similarly, it might also be helpful to indicate if a node is 
online/offline/invalid/.

const char nstate = "Invalid";

if (node_online(nid))
	nstate = "Online";
else if (node_possible(nid))
	nstate = "Offline";


Printing that in addition to the raw value could be helpful. Just some 
thoughts.

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Yafang Shao]

On Mon, Feb 1, 2021 at 9:05 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2021-02-01 at 19:56 +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,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
>
> While debugfs is not an ABI, this format is exported in debugfs to
> userspace via mm/page_owner.c read_page_owner/print_page_owner.
>

Right, the page_owner will be affected by this  change.

> Does changing the output format matter to anyone?
>

The user tools which parse the page_owner may be affected.
If we don't want to affect the userspace tools, I think we can make a
little change in page_owner as follows,

    unsigned long masked_flags = page->flags &  (BIT(NR_PAGEFLAGS) - 1);
    snprintf("..., %#lx(%pGp)\n", page->flags, &masked_flags);

> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > +static
> > +char *format_page_flags(char *buf, char *end, unsigned long page_flags)
> > +{
> > +     unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1);
> > +     int size = ARRAY_SIZE(pfl);
>
> There's no real value in used-once variables.
>
> > +     bool separator = false;
> > +     int i;
> > +
> > +     for (i = 0; i < size; i++) {
>
> Use ARRAY_SIZE here instead
>

Sure

>         for (i = 0; i < ARRAY_SIZE(pfl); i++) {
>
> > +             if (pfl[i].width == 0)
> > +                     continue;
> > +
> > +             if (separator) {
> > +                     if (buf < end)
> > +                             *buf = ',';
> > +                     buf++;
> > +             }
> > +
> > +
> > +             buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > +
> > +             buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask,
> > +                          *pfl[i].spec);
> > +             separator = true;
> > +     }
>
> Style question:
> Might this array be more intelligible with pointers instead of indexes?

Good suggestion!
I will change it in the next version.

> Something like:
>
>         struct page_flags_layout *p;
>
>         for (p = pfl; p < pfl + ARRAY_SIZE(pfl); p++) {
>                 if (p->width == 0)
>                         continue;
>

>                 if (p > pfl) {
>                         if (buf < end)
>                                 *buf = ',';
>                         buf++;
>                 }

It doesn't work, because there's a 'continue' above, meaning that the
p may be greater than pfl without doing anything.

>
>                 buf = string(buf, end, p->name, *p->spec);
>                 buf = number(buf, end, (page_flags >> p->shift) & p->mask, *p->spec);
>         }
>
> > +
> > +     if (flags) {
>
> Maybe:
>
>         if (page_flags & (BIT(NR_PAGEFLAGS) - 1)) {
>

Sure.

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


--
Thanks
Yafang

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Andy Shevchenko]

On Mon, Feb 01, 2021 at 07:56:10PM +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,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> 
> - After the patch,
> [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> 
> The Documentation and test cases are also updated.

Thanks for an update, my comments below.

...

> -	%pGp	referenced|uptodate|lru|active|private
> +	%pGp	Node 0,Zone 2,referenced|uptodate|lru|active|private

Since of the nature of printf() buffer, I wonder if these should be at the end.
I.o.w. the question is is the added material more important to user to see than
the existed one?

...

> +static void __init
> +page_flags_test(int section, int node, int zone, int last_cpupid,
> +		int kasan_tag, int flags, const char *name, char *cmp_buf)
> +{
> +	unsigned long page_flags = 0;
> +	unsigned long size = 0;
> +
> +#ifdef SECTION_IN_PAGE_FLAGS
> +	page_flags |= (sec & SECTIONS_MASK) << SECTIONS_PGSHIFT;
> +	snprintf(cmp_buf, BUF_SIZE, "Section %#x,", sec);

I would keep it in the same form as latter ones, i.e.

	snprintf(cmp_buf + size, BUF_SIZE - size, "Section %#x,", sec);

In this case it will be easier if at some point we might need to reshuffle.

> +	size = strlen(cmp_buf);
> +#endif
> +
> +	page_flags |= (node & NODES_MASK) << NODES_PGSHIFT;
> +	snprintf(cmp_buf + size, BUF_SIZE - size, "Node %d", node);
> +	size = strlen(cmp_buf);
> +
> +	page_flags |= (zone & ZONES_MASK) << ZONES_PGSHIFT;
> +	snprintf(cmp_buf + size, BUF_SIZE - size, ",Zone %d", zone);
> +	size = strlen(cmp_buf);
> +
> +#ifndef LAST_CPUPID_NOT_IN_PAGE_FLAGS
> +	page_flags |= (last_cpupid & LAST_CPUPID_MASK) << LAST_CPUPID_PGSHIFT;
> +	snprintf(cmp_buf + size, BUF_SIZE - size, ",Lastcpupid %#x", last_cpupid);
> +	size = strlen(cmp_buf);
> +#endif
> +
> +#if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS)
> +	page_flags |= (tag & KASAN_TAG_MASK) << KASAN_TAG_PGSHIFT;
> +	snprintf(cmp_buf + size, BUF_SIZE - size, ",Kasantag %#x", tag);
> +	size = strlen(cmp_buf);
> +#endif
> +
> +	test(cmp_buf, "%pGp", &page_flags);
> +
> +	if (flags) {

If below will be always for flags set, I would rewrite this condition as

	if (!flags)
		return;

but it's up to you.

> +		page_flags |= flags;
> +		snprintf(cmp_buf + size, BUF_SIZE - size, ",%s", name);
> +		test(cmp_buf, "%pGp", &page_flags);
> +	}
> +}

...

> -	flags = 0;

> -	flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
> -		| 1UL << PG_active | 1UL << PG_swapbacked;

I would leave this untouched and reuse below...

> +	cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
> +	if (!cmp_buffer)
> +		return;

...as

> +	page_flags_test(0, 0, 0, 0, 0, 0, NULL, cmp_buffer);

	flags = 0;
	page_flags_test(0, 0, 0, 0, 0, flags, NULL, cmp_buffer);

> +	page_flags_test(1, 1, 1, 0x1ffff, 1,
> +			(1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
> +			 | 1UL << PG_active | 1UL << PG_swapbacked),
> +			"uptodate|dirty|lru|active|swapbacked",
> +			cmp_buffer);

	flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
		| 1UL << PG_active | 1UL << PG_swapbacked;
	page_flags_test(1, 1, 1, 0x1ffff, 1, flags,
			"uptodate|dirty|lru|active|swapbacked",
			cmp_buffer);

...

> +static const struct page_flags_layout pfl[] = {
> +	{SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
> +	 &default_dec_spec, "Section "},
> +	{NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
> +	 &default_dec_spec, "Node "},
> +	{ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
> +	 &default_dec_spec, "Zone "},
> +	{LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
> +	 &default_flag_spec, "Lastcpupid "},
> +	{KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
> +	 &default_flag_spec, "Kasantag "},
> +};

Please add trailing space only once where it's needed (below in the code).

...

> +static
> +char *format_page_flags(char *buf, char *end, unsigned long page_flags)
> +{
> +	unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1);
> +	int size = ARRAY_SIZE(pfl);
> +	bool separator = false;
> +	int i;
> +
> +	for (i = 0; i < size; i++) {
> +		if (pfl[i].width == 0)
> +			continue;
> +
> +		if (separator) {
> +			if (buf < end)
> +				*buf = ',';
> +			buf++;
> +		}

> +
> +

One blank line is enough.

> +		buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> +
> +		buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask,
> +			     *pfl[i].spec);
> +		separator = true;
> +	}
> +
> +	if (flags) {
> +		if (buf < end)
> +			*buf = ',';
> +		buf++;
> +	}

I think you may optimize above to avoid using the separator variable.

	DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
	unsigned long flags;
	unsigned long last;

	for (i = 0; i < ARRAY_SIZE(pfl); i++)
		__assign_bit(mask, pfl[i].width);

	last = find_last_bit(mask, ARRAY_SIZE(pfl));

	for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
		flags = (page_flags >> pfl[i].shift) & pfl[i].mask;

		/* Format: Flag Name + ' ' (space) + Number + ',' (separator) */
		buf = string(buf, end, pfl[i].name, *pfl[i].spec);

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

		buf = number(buf, end, flags, *pfl[i].spec);

		/* No separator for the last entry */
		if ((page_flags & (BIT(NR_PAGEFLAGS) - 1)) || (i != last)) {
			if (buf < end)
				*buf = ',';
			buf++;
		}
	}

> +	return buf;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[David Hildenbrand]

On 01.02.21 14:23, David Hildenbrand wrote:
> On 01.02.21 12:56, 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,
>> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
>>
>> - After the patch,
>> [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> 
> For debugging purposes, it might be helpful to have the actual zone name
> (and to know if the value is sane). You could obtain it (without other
> modifications) via
> 
> const char zname = "Invalid";
> 
> if (zone < MAX_NR_ZONES)
> 	zname = first_online_pgdat()->node_zones[zone].name;
> 
> 
> Similarly, it might also be helpful to indicate if a node is
> online/offline/invalid/.
> 
> const char nstate = "Invalid";
> 
> if (node_online(nid))
> 	nstate = "Online";
> else if (node_possible(nid))
> 	nstate = "Offline";

Just remembering that we have to take care of nid limits:

if (nid >= 0 && nid < MAX_NUMNODES) {
	if (node_online(nid))
		nstate = "Online";
	else if (node_possible(nid))
		nstate = "Offline";
}

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Andy Shevchenko]

On Mon, Feb 01, 2021 at 02:23:33PM +0100, David Hildenbrand wrote:
> On 01.02.21 12:56, 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,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > 
> > - After the patch,
> > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> 
> For debugging purposes, it might be helpful to have the actual zone name
> (and to know if the value is sane). You could obtain it (without other
> modifications) via
> 
> const char zname = "Invalid";
> 
> if (zone < MAX_NR_ZONES)
> 	zname = first_online_pgdat()->node_zones[zone].name;
> 
> 
> Similarly, it might also be helpful to indicate if a node is
> online/offline/invalid/.
> 
> const char nstate = "Invalid";
> 
> if (node_online(nid))
> 	nstate = "Online";
> else if (node_possible(nid))
> 	nstate = "Offline";
> 
> 
> Printing that in addition to the raw value could be helpful. Just some
> thoughts.

printf() buffer is not a black hole, esp. when you get it messed with critical
messages (Oops). I suggest to reduce a burden as much as possible. If you wish
to get this, make it caller-configurable, i.e. adding another letter to the
specifier.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Yafang Shao]

On Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 01, 2021 at 07:56:10PM +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,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> >
> > The Documentation and test cases are also updated.
>
> Thanks for an update, my comments below.
>
> ...
>
> > -     %pGp    referenced|uptodate|lru|active|private
> > +     %pGp    Node 0,Zone 2,referenced|uptodate|lru|active|private
>
> Since of the nature of printf() buffer, I wonder if these should be at the end.
> I.o.w. the question is is the added material more important to user to see than
> the existed one?
>

The existing one should be more important than the added one.
But the order of output will not match with the value for page->flags.
E.g.
    flags=0x17ffffc0010200(slab|head,Node 0,Zone 2,Lastcpupid 0x1fffff)
It may be strange to compare the value with the string.

> ...
>
> > +static void __init
> > +page_flags_test(int section, int node, int zone, int last_cpupid,
> > +             int kasan_tag, int flags, const char *name, char *cmp_buf)
> > +{
> > +     unsigned long page_flags = 0;
> > +     unsigned long size = 0;
> > +
> > +#ifdef SECTION_IN_PAGE_FLAGS
> > +     page_flags |= (sec & SECTIONS_MASK) << SECTIONS_PGSHIFT;
> > +     snprintf(cmp_buf, BUF_SIZE, "Section %#x,", sec);
>
> I would keep it in the same form as latter ones, i.e.
>
>         snprintf(cmp_buf + size, BUF_SIZE - size, "Section %#x,", sec);
>
> In this case it will be easier if at some point we might need to reshuffle.
>

Good suggestion.

> > +     size = strlen(cmp_buf);
> > +#endif
> > +
> > +     page_flags |= (node & NODES_MASK) << NODES_PGSHIFT;
> > +     snprintf(cmp_buf + size, BUF_SIZE - size, "Node %d", node);
> > +     size = strlen(cmp_buf);
> > +
> > +     page_flags |= (zone & ZONES_MASK) << ZONES_PGSHIFT;
> > +     snprintf(cmp_buf + size, BUF_SIZE - size, ",Zone %d", zone);
> > +     size = strlen(cmp_buf);
> > +
> > +#ifndef LAST_CPUPID_NOT_IN_PAGE_FLAGS
> > +     page_flags |= (last_cpupid & LAST_CPUPID_MASK) << LAST_CPUPID_PGSHIFT;
> > +     snprintf(cmp_buf + size, BUF_SIZE - size, ",Lastcpupid %#x", last_cpupid);
> > +     size = strlen(cmp_buf);
> > +#endif
> > +
> > +#if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS)
> > +     page_flags |= (tag & KASAN_TAG_MASK) << KASAN_TAG_PGSHIFT;
> > +     snprintf(cmp_buf + size, BUF_SIZE - size, ",Kasantag %#x", tag);
> > +     size = strlen(cmp_buf);
> > +#endif
> > +
> > +     test(cmp_buf, "%pGp", &page_flags);
> > +
> > +     if (flags) {
>
> If below will be always for flags set, I would rewrite this condition as
>
>         if (!flags)
>                 return;
>
> but it's up to you.
>
> > +             page_flags |= flags;
> > +             snprintf(cmp_buf + size, BUF_SIZE - size, ",%s", name);
> > +             test(cmp_buf, "%pGp", &page_flags);
> > +     }
> > +}
>
> ...
>
> > -     flags = 0;
>
> > -     flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
> > -             | 1UL << PG_active | 1UL << PG_swapbacked;
>
> I would leave this untouched and reuse below...
>
> > +     cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
> > +     if (!cmp_buffer)
> > +             return;
>
> ...as
>
> > +     page_flags_test(0, 0, 0, 0, 0, 0, NULL, cmp_buffer);
>
>         flags = 0;
>         page_flags_test(0, 0, 0, 0, 0, flags, NULL, cmp_buffer);
>
> > +     page_flags_test(1, 1, 1, 0x1ffff, 1,
> > +                     (1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
> > +                      | 1UL << PG_active | 1UL << PG_swapbacked),
> > +                     "uptodate|dirty|lru|active|swapbacked",
> > +                     cmp_buffer);
>
>         flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
>                 | 1UL << PG_active | 1UL << PG_swapbacked;
>         page_flags_test(1, 1, 1, 0x1ffff, 1, flags,
>                         "uptodate|dirty|lru|active|swapbacked",
>                         cmp_buffer);
>

Sure, I will change them.

> ...
>
> > +static const struct page_flags_layout pfl[] = {
> > +     {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
> > +      &default_dec_spec, "Section "},
> > +     {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
> > +      &default_dec_spec, "Node "},
> > +     {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
> > +      &default_dec_spec, "Zone "},
> > +     {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
> > +      &default_flag_spec, "Lastcpupid "},
> > +     {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
> > +      &default_flag_spec, "Kasantag "},
> > +};
>
> Please add trailing space only once where it's needed (below in the code).
>
> ...
>
> > +static
> > +char *format_page_flags(char *buf, char *end, unsigned long page_flags)
> > +{
> > +     unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1);
> > +     int size = ARRAY_SIZE(pfl);
> > +     bool separator = false;
> > +     int i;
> > +
> > +     for (i = 0; i < size; i++) {
> > +             if (pfl[i].width == 0)
> > +                     continue;
> > +
> > +             if (separator) {
> > +                     if (buf < end)
> > +                             *buf = ',';
> > +                     buf++;
> > +             }
>
> > +
> > +
>
> One blank line is enough.
>

Thanks for pointing it out.

BTW, it seems we need to improve the check_patch to catch this kind of
issue automatically.

> > +             buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > +
> > +             buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask,
> > +                          *pfl[i].spec);
> > +             separator = true;
> > +     }
> > +
> > +     if (flags) {
> > +             if (buf < end)
> > +                     *buf = ',';
> > +             buf++;
> > +     }
>
> I think you may optimize above to avoid using the separator variable.
>
>         DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
>         unsigned long flags;
>         unsigned long last;
>
>         for (i = 0; i < ARRAY_SIZE(pfl); i++)
>                 __assign_bit(mask, pfl[i].width);
>
>         last = find_last_bit(mask, ARRAY_SIZE(pfl));
>
>         for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
>                 flags = (page_flags >> pfl[i].shift) & pfl[i].mask;
>
>                 /* Format: Flag Name + ' ' (space) + Number + ',' (separator) */
>                 buf = string(buf, end, pfl[i].name, *pfl[i].spec);
>
>                 if (buf < end)
>                         *buf = ' ';
>                 buf++;
>
>                 buf = number(buf, end, flags, *pfl[i].spec);
>
>                 /* No separator for the last entry */
>                 if ((page_flags & (BIT(NR_PAGEFLAGS) - 1)) || (i != last)) {
>                         if (buf < end)
>                                 *buf = ',';
>                         buf++;
>                 }
>         }
>

Good suggestion. I will think about it.

-- 
Thanks
Yafang

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Yafang Shao]

On Mon, Feb 1, 2021 at 9:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 01, 2021 at 02:23:33PM +0100, David Hildenbrand wrote:
> > On 01.02.21 12:56, 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,
> > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > >
> > > - After the patch,
> > > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> >
> > For debugging purposes, it might be helpful to have the actual zone name
> > (and to know if the value is sane). You could obtain it (without other
> > modifications) via
> >
> > const char zname = "Invalid";
> >
> > if (zone < MAX_NR_ZONES)
> >       zname = first_online_pgdat()->node_zones[zone].name;
> >
> >
> > Similarly, it might also be helpful to indicate if a node is
> > online/offline/invalid/.
> >
> > const char nstate = "Invalid";
> >
> > if (node_online(nid))
> >       nstate = "Online";
> > else if (node_possible(nid))
> >       nstate = "Offline";
> >
> >
> > Printing that in addition to the raw value could be helpful. Just some
> > thoughts.
>
> printf() buffer is not a black hole, esp. when you get it messed with critical
> messages (Oops). I suggest to reduce a burden as much as possible. If you wish
> to get this, make it caller-configurable, i.e. adding another letter to the
> specifier.
>

I think David's suggestion will help us to identify issues.

You mean that we should use another one like "%pGpd" - means pGp debug
- to implement it ?

-- 
Thanks
Yafang

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Matthew Wilcox]

On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
> - Before the patch,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> 
> - After the patch,
> [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)

I would suggest it will be easier to parse as:

flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)

That should alleviate the concerns about debugfs format change -- we've
never guaranteed that flag names won't change, and they now look enough
like flags that parsers shouldn't fall over them.

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Yafang Shao]

On Mon, Feb 1, 2021 at 10:15 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
> > - Before the patch,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
>
> I would suggest it will be easier to parse as:
>
> flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
>
> That should alleviate the concerns about debugfs format change -- we've
> never guaranteed that flag names won't change, and they now look enough
> like flags that parsers shouldn't fall over them.

Good suggestion!
I will do it as you suggested in the next version.

-- 
Thanks
Yafang

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Andy Shevchenko]

On Mon, Feb 01, 2021 at 09:52:53PM +0800, Yafang Shao wrote:
> On Mon, Feb 1, 2021 at 9:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 01, 2021 at 02:23:33PM +0100, David Hildenbrand wrote:
> > > On 01.02.21 12:56, Yafang Shao wrote:

...

> > > Printing that in addition to the raw value could be helpful. Just some
> > > thoughts.
> >
> > printf() buffer is not a black hole, esp. when you get it messed with critical
> > messages (Oops). I suggest to reduce a burden as much as possible. If you wish
> > to get this, make it caller-configurable, i.e. adding another letter to the
> > specifier.
> >
> 
> I think David's suggestion will help us to identify issues.
> 
> You mean that we should use another one like "%pGpd" - means pGp debug
> - to implement it ?

Yes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Andy Shevchenko]

On Mon, Feb 01, 2021 at 09:49:59PM +0800, Yafang Shao wrote:
> On Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:

...

> > > - Before the patch,
> > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > >
> > > - After the patch,
> > > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> > >
> > > The Documentation and test cases are also updated.
> >
> > Thanks for an update, my comments below.
> >
> > ...
> >
> > > -     %pGp    referenced|uptodate|lru|active|private
> > > +     %pGp    Node 0,Zone 2,referenced|uptodate|lru|active|private
> >
> > Since of the nature of printf() buffer, I wonder if these should be at the end.
> > I.o.w. the question is is the added material more important to user to see than
> > the existed one?
> >
> 
> The existing one should be more important than the added one.
> But the order of output will not match with the value for page->flags.
> E.g.
>     flags=0x17ffffc0010200(slab|head,Node 0,Zone 2,Lastcpupid 0x1fffff)
> It may be strange to compare the value with the string.

More I'm looking at it, more I'm thinking it should have different specifiers
for each group of desired flags to be printed.

So, you leave %pGp as is and then add another letter to add more details, so
user will choose what and in which order they want.

For example, let's assume %pGp == %pGpf and P is a new specifier for what you
are initially adding here:

  %pGpfP => referenced|uptodate|lru|active|private,Node 0,Zone 2
  %pGpPf => Node 0,Zone 2,referenced|uptodate|lru|active|private

and so on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Andy Shevchenko]

On Mon, Feb 01, 2021 at 06:16:42PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 01, 2021 at 09:49:59PM +0800, Yafang Shao wrote:
> > On Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:

...

> > The existing one should be more important than the added one.
> > But the order of output will not match with the value for page->flags.
> > E.g.
> >     flags=0x17ffffc0010200(slab|head,Node 0,Zone 2,Lastcpupid 0x1fffff)
> > It may be strange to compare the value with the string.
> 
> More I'm looking at it, more I'm thinking it should have different specifiers
> for each group of desired flags to be printed.
> 
> So, you leave %pGp as is and then add another letter to add more details, so
> user will choose what and in which order they want.
> 
> For example, let's assume %pGp == %pGpf and P is a new specifier for what you
> are initially adding here:
> 
>   %pGpfP => referenced|uptodate|lru|active|private,Node 0,Zone 2
>   %pGpPf => Node 0,Zone 2,referenced|uptodate|lru|active|private
> 
> and so on.

And I agree with Matthew about format, but it doesn't oppose my suggestion
above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Joe Perches]

On Mon, 2021-02-01 at 14:15 +0000, Matthew Wilcox wrote:
> On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
> > - Before the patch,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > 
> > - After the patch,
> > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> 
> I would suggest it will be easier to parse as:
> 
> flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> 
> That should alleviate the concerns about debugfs format change -- we've
> never guaranteed that flag names won't change, and they now look enough
> like flags that parsers shouldn't fall over them.

Seems sensible and would make the generating code simpler too.

But is it worth the vsprintf code expansion for the 5 current uses?

mm/debug.c:     pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
mm/memory-failure.c:                    pr_info("soft offline: %#lx: %s migration failed %d, type %lx (%pGp)\n",
mm/memory-failure.c:            pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %lx (%pGp)\n",
mm/memory-failure.c:            pr_info("%s: %#lx: unknown page type: %lx (%pGp)\n",
mm/page_owner.c:                        "PFN %lu type %s Block %lu type %s Flags %#lx(%pGp)\n",

Wouldn't it be more sensible just to put this code in a new function
call in mm?

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Matthew Wilcox]

On Mon, Feb 01, 2021 at 10:51:03AM -0800, Joe Perches wrote:
> On Mon, 2021-02-01 at 14:15 +0000, Matthew Wilcox wrote:
> > On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
> > > - Before the patch,
> > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > > 
> > > - After the patch,
> > > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> > 
> > I would suggest it will be easier to parse as:
> > 
> > flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> > 
> > That should alleviate the concerns about debugfs format change -- we've
> > never guaranteed that flag names won't change, and they now look enough
> > like flags that parsers shouldn't fall over them.
> 
> Seems sensible and would make the generating code simpler too.
> 
> But is it worth the vsprintf code expansion for the 5 current uses?
> 
> mm/debug.c:     pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
> mm/memory-failure.c:                    pr_info("soft offline: %#lx: %s migration failed %d, type %lx (%pGp)\n",
> mm/memory-failure.c:            pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %lx (%pGp)\n",
> mm/memory-failure.c:            pr_info("%s: %#lx: unknown page type: %lx (%pGp)\n",
> mm/page_owner.c:                        "PFN %lu type %s Block %lu type %s Flags %#lx(%pGp)\n",
> 
> Wouldn't it be more sensible just to put this code in a new function
> call in mm?

Does it matter whether the code lives in vsprintf.c or mm/debug.c?  It's
built into the kernel core either way.  I'm not a huge fan of the current
way %pFoo is handled, but unless/until it's drastically revised, I don't
think this proposed patch makes anything worse.

Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp

[Yafang Shao]

On Tue, Feb 2, 2021 at 12:16 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 01, 2021 at 09:49:59PM +0800, Yafang Shao wrote:
> > On Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
>
> ...
>
> > > > - Before the patch,
> > > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > > >
> > > > - After the patch,
> > > > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> > > >
> > > > The Documentation and test cases are also updated.
> > >
> > > Thanks for an update, my comments below.
> > >
> > > ...
> > >
> > > > -     %pGp    referenced|uptodate|lru|active|private
> > > > +     %pGp    Node 0,Zone 2,referenced|uptodate|lru|active|private
> > >
> > > Since of the nature of printf() buffer, I wonder if these should be at the end.
> > > I.o.w. the question is is the added material more important to user to see than
> > > the existed one?
> > >
> >
> > The existing one should be more important than the added one.
> > But the order of output will not match with the value for page->flags.
> > E.g.
> >     flags=0x17ffffc0010200(slab|head,Node 0,Zone 2,Lastcpupid 0x1fffff)
> > It may be strange to compare the value with the string.
>
> More I'm looking at it, more I'm thinking it should have different specifiers
> for each group of desired flags to be printed.
>
> So, you leave %pGp as is and then add another letter to add more details, so
> user will choose what and in which order they want.
>
> For example, let's assume %pGp == %pGpf and P is a new specifier for what you
> are initially adding here:
>
>   %pGpfP => referenced|uptodate|lru|active|private,Node 0,Zone 2
>   %pGpPf => Node 0,Zone 2,referenced|uptodate|lru|active|private
>
> and so on.

Thanks for your suggestion. I will think about it.

-- 
Thanks
Yafang