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

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

[Yafang Shao]

The existed pGp shows the names of page flags only, rather than the full
information including section, node, zone, last cpuipid and kasan tag.
While it is not easy to parse these information manually because there
are so many flavors. We'd better interpret them in printf.

To be compitable with the existed format of pGp, the new introduced ones
also use '|' as the separator, then the user tools parsing pGp won't
need to make change, suggested by Matthew. The new added information is
tracked onto the end of the existed one, e.g.
[ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)

The documentation and test cases are also updated. The result of the
test cases as follows,
[  501.485081] test_printf: loaded.
[  501.485768] test_printf: all 388 tests passed
[  501.488762] test_printf: unloaded.

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

v4:
- extend %pGp instead of introducing new format, per Matthew

v3:
- coding improvement, per Joe and Andy
- the possible impact on debugfs and the fix of it, per Joe and Matthew
- introduce new format instead of changing pGp, per Andy

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                         | 60 +++++++++++++++++----
 lib/vsprintf.c                            | 66 +++++++++++++++++++++--
 mm/slub.c                                 | 13 ++---
 4 files changed, 121 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH v4 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)

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: David Rientjes <rientjes@google.com>
Acked-by: Christoph Lameter <cl@linux.com>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.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

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

[David Hildenbrand]

On 09.02.21 11:56, Yafang Shao wrote:
> 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)
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: David Rientjes <rientjes@google.com>
> Acked-by: Christoph Lameter <cl@linux.com>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reviewed-by: Miaohe Lin <linmiaohe@huawei.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);
>   
>   }
>   
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v4 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>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Matthew Wilcox <willy@infradead.org>
---
 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

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

[David Hildenbrand]

On 09.02.21 11:56, Yafang Shao wrote:
> 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>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> ---
>   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);
>   		}
>   	}
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v4 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.

To be compitable with the existed format of pGp, the new introduced ones
also use '|' as the separator, then the user tools parsing pGp won't
need to make change, suggested by Matthew. The new information is
tracked onto the end of the existed one.

On example of the output in mm/slub.c as follows,
- Before the patch,
[ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)

- After the patch,
[ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)

The documentation and test cases are also updated. The output of the
test cases as follows,
[  501.485081] test_printf: loaded.
[  501.485768] test_printf: all 388 tests passed
[  501.488762] test_printf: unloaded.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
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>
Cc: Matthew Wilcox <willy@infradead.org>
---
 Documentation/core-api/printk-formats.rst |  2 +-
 lib/test_printf.c                         | 60 +++++++++++++++++----
 lib/vsprintf.c                            | 66 +++++++++++++++++++++--
 3 files changed, 114 insertions(+), 14 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 6d26c5c6ac48..93c3e48ff30d 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	referenced|uptodate|lru|active|private|node=0|zone=2|lastcpupid=0x1fffff
 	%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..148773dfe97a 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -569,24 +569,68 @@ 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;
+
+	flags &= BIT(NR_PAGEFLAGS) - 1;
+	if (flags) {
+		page_flags |= flags;
+		snprintf(cmp_buf + size, BUF_SIZE - size, "%s|", name);
+		size = strlen(cmp_buf);
+	}
+
+#ifdef SECTION_IN_PAGE_FLAGS
+	page_flags |= (sec & SECTIONS_MASK) << SECTIONS_PGSHIFT;
+	snprintf(cmp_buf + size, BUF_SIZE - size, "section=%#x|", sec);
+	size = strlen(cmp_buf);
+#endif
+
+	page_flags |= ((node & NODES_MASK) << NODES_PGSHIFT) |
+			((zone & ZONES_MASK) << ZONES_PGSHIFT);
+	snprintf(cmp_buf + size, BUF_SIZE - size, "node=%d|zone=%d", node, 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);
+}
+
 static void __init
 flags(void)
 {
 	unsigned long flags;
-	gfp_t gfp;
 	char *cmp_buffer;
+	gfp_t gfp;
+
+	cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
+	if (!cmp_buffer)
+		return;
 
 	flags = 0;
-	test("", "%pGp", &flags);
+	page_flags_test(0, 0, 0, 0, 0, flags, "", cmp_buffer);
 
-	/* Page flags should filter the zone id */
 	flags = 1UL << NR_PAGEFLAGS;
-	test("", "%pGp", &flags);
+	page_flags_test(0, 0, 0, 0, 0, flags, "", cmp_buffer);
 
 	flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
 		| 1UL << PG_active | 1UL << PG_swapbacked;
-	test("uptodate|dirty|lru|active|swapbacked", "%pGp", &flags);
-
+	page_flags_test(1, 1, 1, 0x1fffff, 1, flags,
+			"uptodate|dirty|lru|active|swapbacked",
+			cmp_buffer);
 
 	flags = VM_READ | VM_EXEC | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC
 			| VM_DENYWRITE;
@@ -601,10 +645,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..3f26611adb34 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1916,6 +1916,66 @@ 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 flags)
+{
+	DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
+	unsigned long last;
+	int i;
+
+	if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
+		if (buf < end)
+			*buf = '|';
+		buf++;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(pfl); i++)
+		__assign_bit(i, mask, pfl[i].width);
+
+	last = find_last_bit(mask, ARRAY_SIZE(pfl));
+
+	for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
+		/* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
+		buf = string(buf, end, pfl[i].name, *pfl[i].spec);
+
+		if (buf < end)
+			*buf = '=';
+		buf++;
+		buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
+			     *pfl[i].spec);
+
+		/* No separator for the last entry */
+		if (i != last) {
+			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,10 +1989,10 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
 	switch (fmt[1]) {
 	case 'p':
 		flags = *(unsigned long *)flags_ptr;
-		/* Remove zone id */
-		flags &= (1UL << NR_PAGEFLAGS) - 1;
 		names = pageflag_names;
-		break;
+		buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) - 1), names);
+		buf = format_page_flags(buf, end, flags);
+		return buf;
 	case 'v':
 		flags = *(unsigned long *)flags_ptr;
 		names = vmaflag_names;
-- 
2.17.1

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

[David Hildenbrand]

On 09.02.21 11: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.
> 
> To be compitable with the existed format of pGp, the new introduced ones
> also use '|' as the separator, then the user tools parsing pGp won't
> need to make change, suggested by Matthew. The new information is
> tracked onto the end of the existed one.
> 
> On example of the output in mm/slub.c as follows,
> - Before the patch,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> 
> - After the patch,
> [ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)

Did not review in detail, but LGTM.

Thanks, this will be very helpful!


-- 
Thanks,

David / dhildenb

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

[Vlastimil Babka]

On 2/9/21 11:56 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.
> 
> To be compitable with the existed format of pGp, the new introduced ones
> also use '|' as the separator, then the user tools parsing pGp won't
> need to make change, suggested by Matthew. The new information is
> tracked onto the end of the existed one.
> 
> On example of the output in mm/slub.c as follows,
> - Before the patch,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> 
> - After the patch,
> [ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> 
> The documentation and test cases are also updated. The output of the
> test cases as follows,
> [  501.485081] test_printf: loaded.
> [  501.485768] test_printf: all 388 tests passed
> [  501.488762] test_printf: unloaded.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> 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>
> Cc: Matthew Wilcox <willy@infradead.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

The 'pfl' array should be even useful in kernel crash dump tools!

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

[Petr Mladek]

On Tue 2021-02-09 18:56:13, 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.
> 
> To be compitable with the existed format of pGp, the new introduced ones
> also use '|' as the separator, then the user tools parsing pGp won't
> need to make change, suggested by Matthew. The new information is
> tracked onto the end of the existed one.
> 
> On example of the output in mm/slub.c as follows,
> - Before the patch,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> 
> - After the patch,
> [ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> 
> The documentation and test cases are also updated. The output of the
> test cases as follows,
> [  501.485081] test_printf: loaded.
> [  501.485768] test_printf: all 388 tests passed
> [  501.488762] test_printf: unloaded.
> 

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 14c9a6af1b23..3f26611adb34 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1916,6 +1916,66 @@ 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 flags)
> +{
> +	DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> +	unsigned long last;
> +	int i;
> +
> +	if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> +		if (buf < end)
> +			*buf = '|';
> +		buf++;
> +	}

This is far from obvious. You print '|' here because you printed
something somewhere else. See below.

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

The bitmap looks like an overkill. If I get it correctly, it is a
tricky way to handle only flags defined by the used build
configuration. See below.

> +	last = find_last_bit(mask, ARRAY_SIZE(pfl));
> +
> +	for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> +		/* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
> +		buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> +
> +		if (buf < end)
> +			*buf = '=';
> +		buf++;
> +		buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> +			     *pfl[i].spec);
> +
> +		/* No separator for the last entry */
> +		if (i != last) {
> +			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,10 +1989,10 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
>  	switch (fmt[1]) {
>  	case 'p':
>  		flags = *(unsigned long *)flags_ptr;
> -		/* Remove zone id */
> -		flags &= (1UL << NR_PAGEFLAGS) - 1;
>  		names = pageflag_names;

The "names" variable is needed only with "break;" when using the final
format_flags(buf, end, flags, names);

> -		break;
> +		buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) - 1), names);
> +		buf = format_page_flags(buf, end, flags);

I am sorry for my ignorance. I am not familiar with MM.
But it is pretty hard to understand what call does what.

I have found the following comment in include/linux/page_flags.h:

 * The page flags field is split into two parts, the main flags area
 * which extends from the low bits upwards, and the fields area which
 * extends from the high bits downwards.

Sigh, I know that you already reworked this several times because
people "nitpicked" about the code style. But it seems that it
rather diverged instead of converged.

What about the following?

Note: It is inpired by the names "main area" and "fields area"
      mentioned in the above comment from page_flags.h.
      I have later realized that "page_flags_layout" actually made
      sense as well. Feel free to rename page_flags_fileds
      back to page_flags_layout.

Anyway, this is my proposal:

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..cf67b39d72ae 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1916,6 +1916,67 @@ char *format_flags(char *buf, char *end, unsigned long flags,
 	return buf;
 }
 
+/* Meta information for page flags in the fields area */
+struct page_flags_fields {
+	int width;
+	int shift;
+	int mask;
+	const struct printf_spec *spec;
+	const char *name;
+};
+
+static const struct page_flags_fields pff[] = {
+	{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 flags)
+{
+	unsigned long main_flags = flags & (BIT(NR_PAGEFLAGS) - 1);
+	bool append = false;
+	int i;
+
+	/* Page flags from the main area. */
+	if (main_flags) {
+		buf = format_flags(buf, end, main_flags, pageflag_names);
+		append = true;
+	}
+
+	/* Page flags from the fields area */
+	for (i = 0; i < ARRAY_SIZE(pff); i++) {
+		/* Skip undefined fields. */
+		if (!pff[i].width)
+			continue;
+
+		/* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
+		if (append) {
+			if (buf < end)
+				*buf = '|';
+			buf++;
+		}
+
+		buf = string(buf, end, pff[i].name, *pff[i].spec);
+		if (buf < end)
+			*buf = '=';
+		buf++;
+		buf = number(buf, end, (flags >> pff[i].shift) & pff[i].mask,
+			     *pff[i].spec);
+
+		append = true;
+	}
+
+	return buf;
+}
+
 static noinline_for_stack
 char *flags_string(char *buf, char *end, void *flags_ptr,
 		   struct printf_spec spec, const char *fmt)
@@ -1929,10 +1990,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
 	switch (fmt[1]) {
 	case 'p':
 		flags = *(unsigned long *)flags_ptr;
-		/* Remove zone id */
-		flags &= (1UL << NR_PAGEFLAGS) - 1;
-		names = pageflag_names;
-		break;
+		return format_page_flags(buf, end, flags);
 	case 'v':
 		flags = *(unsigned long *)flags_ptr;
 		names = vmaflag_names;
-- 
2.26.2

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

[Andy Shevchenko]

On Tue, Feb 09, 2021 at 02:53:53PM +0100, Petr Mladek wrote:
> On Tue 2021-02-09 18:56:13, Yafang Shao wrote:

...

> I am sorry for my ignorance. I am not familiar with MM.
> But it is pretty hard to understand what call does what.
> 
> I have found the following comment in include/linux/page_flags.h:
> 
>  * The page flags field is split into two parts, the main flags area
>  * which extends from the low bits upwards, and the fields area which
>  * extends from the high bits downwards.
> 
> Sigh, I know that you already reworked this several times because
> people "nitpicked" about the code style. But it seems that it
> rather diverged instead of converged.
> 
> What about the following?

Isn't is some like v1 or v2?

> Note: It is inpired by the names "main area" and "fields area"
>       mentioned in the above comment from page_flags.h.
>       I have later realized that "page_flags_layout" actually made
>       sense as well. Feel free to rename page_flags_fileds
>       back to page_flags_layout.
> 
> Anyway, this is my proposal:

What about to create a one format_flags() function which accepts new data
structure and do something like

buf = format_flags(main_area);
buf = format_flags(fields_area);
return buf;

?

-- 
With Best Regards,
Andy Shevchenko

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

[Petr Mladek]

On Tue 2021-02-09 16:16:01, Andy Shevchenko wrote:
> On Tue, Feb 09, 2021 at 02:53:53PM +0100, Petr Mladek wrote:
> > On Tue 2021-02-09 18:56:13, Yafang Shao wrote:
> 
> ...
> 
> > I am sorry for my ignorance. I am not familiar with MM.
> > But it is pretty hard to understand what call does what.
> > 
> > I have found the following comment in include/linux/page_flags.h:
> > 
> >  * The page flags field is split into two parts, the main flags area
> >  * which extends from the low bits upwards, and the fields area which
> >  * extends from the high bits downwards.
> > 
> > Sigh, I know that you already reworked this several times because
> > people "nitpicked" about the code style. But it seems that it
> > rather diverged instead of converged.
> > 
> > What about the following?
> 
> Isn't is some like v1 or v2?

Yes. And people suggested only some micro-optimizations and reported
few small bugs there.

But the code was heavily reworked in v3 to support the new
%pGp[bl] variants. It also added the trick with the bitmap
which invalidated all the previous suggestions.

v3 and v4 review focused on behavior changes. We finally
agreed on it. Let's give it more cycle and clean up the code
after so many shuffles.


> > Note: It is inpired by the names "main area" and "fields area"
> >       mentioned in the above comment from page_flags.h.
> >       I have later realized that "page_flags_layout" actually made
> >       sense as well. Feel free to rename page_flags_fileds
> >       back to page_flags_layout.
> > 
> > Anyway, this is my proposal:
> 
> What about to create a one format_flags() function which accepts new data
> structure and do something like
> 
> buf = format_flags(main_area);
> buf = format_flags(fields_area);
> return buf;

I am not sure that it would make things easier. The handling of
the main area is trivial and reuses existing structures. The handling
of the fields area seems to be much more complicated.

Best Regards,
Petr

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

[Yafang Shao]

On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2021-02-09 18:56:13, 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.
> >
> > To be compitable with the existed format of pGp, the new introduced ones
> > also use '|' as the separator, then the user tools parsing pGp won't
> > need to make change, suggested by Matthew. The new information is
> > tracked onto the end of the existed one.
> >
> > On example of the output in mm/slub.c as follows,
> > - Before the patch,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> >
> > The documentation and test cases are also updated. The output of the
> > test cases as follows,
> > [  501.485081] test_printf: loaded.
> > [  501.485768] test_printf: all 388 tests passed
> > [  501.488762] test_printf: unloaded.
> >
>
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 14c9a6af1b23..3f26611adb34 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1916,6 +1916,66 @@ 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 flags)
> > +{
> > +     DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> > +     unsigned long last;
> > +     int i;
> > +
> > +     if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> > +             if (buf < end)
> > +                     *buf = '|';
> > +             buf++;
> > +     }
>
> This is far from obvious. You print '|' here because you printed
> something somewhere else. See below.
>
> > +
> > +     for (i = 0; i < ARRAY_SIZE(pfl); i++)
> > +             __assign_bit(i, mask, pfl[i].width);
>
> The bitmap looks like an overkill. If I get it correctly, it is a
> tricky way to handle only flags defined by the used build
> configuration. See below.
>
> > +     last = find_last_bit(mask, ARRAY_SIZE(pfl));
> > +
> > +     for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> > +             /* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
> > +             buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > +
> > +             if (buf < end)
> > +                     *buf = '=';
> > +             buf++;
> > +             buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> > +                          *pfl[i].spec);
> > +
> > +             /* No separator for the last entry */
> > +             if (i != last) {
> > +                     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,10 +1989,10 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
> >       switch (fmt[1]) {
> >       case 'p':
> >               flags = *(unsigned long *)flags_ptr;
> > -             /* Remove zone id */
> > -             flags &= (1UL << NR_PAGEFLAGS) - 1;
> >               names = pageflag_names;
>
> The "names" variable is needed only with "break;" when using the final
> format_flags(buf, end, flags, names);
>
> > -             break;
> > +             buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) - 1), names);
> > +             buf = format_page_flags(buf, end, flags);
>
> I am sorry for my ignorance. I am not familiar with MM.
> But it is pretty hard to understand what call does what.
>
> I have found the following comment in include/linux/page_flags.h:
>
>  * The page flags field is split into two parts, the main flags area
>  * which extends from the low bits upwards, and the fields area which
>  * extends from the high bits downwards.
>
> Sigh, I know that you already reworked this several times because
> people "nitpicked" about the code style. But it seems that it
> rather diverged instead of converged.
>
> What about the following?
>
> Note: It is inpired by the names "main area" and "fields area"
>       mentioned in the above comment from page_flags.h.
>       I have later realized that "page_flags_layout" actually made
>       sense as well. Feel free to rename page_flags_fileds
>       back to page_flags_layout.
>
> Anyway, this is my proposal:
>

This proposal is similar to v2.
I don't mind changing it back with your additional better naming.
By the way, it will be better to make a little change per Joe's
suggestion on v2 that using a pointer instead of the index, for
example,

 for (p = pff; p < pff + ARRAY_SIZE(pff); p++) {

I will leave it for a moment in case others have a different opinion.

Anyway I like your proposal.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..cf67b39d72ae 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1916,6 +1916,67 @@ char *format_flags(char *buf, char *end, unsigned long flags,
>         return buf;
>  }
>
> +/* Meta information for page flags in the fields area */
> +struct page_flags_fields {
> +       int width;
> +       int shift;
> +       int mask;
> +       const struct printf_spec *spec;
> +       const char *name;
> +};
> +
> +static const struct page_flags_fields pff[] = {
> +       {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 flags)
> +{
> +       unsigned long main_flags = flags & (BIT(NR_PAGEFLAGS) - 1);
> +       bool append = false;
> +       int i;
> +
> +       /* Page flags from the main area. */
> +       if (main_flags) {
> +               buf = format_flags(buf, end, main_flags, pageflag_names);
> +               append = true;
> +       }
> +
> +       /* Page flags from the fields area */
> +       for (i = 0; i < ARRAY_SIZE(pff); i++) {
> +               /* Skip undefined fields. */
> +               if (!pff[i].width)
> +                       continue;
> +
> +               /* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
> +               if (append) {
> +                       if (buf < end)
> +                               *buf = '|';
> +                       buf++;
> +               }
> +
> +               buf = string(buf, end, pff[i].name, *pff[i].spec);
> +               if (buf < end)
> +                       *buf = '=';
> +               buf++;
> +               buf = number(buf, end, (flags >> pff[i].shift) & pff[i].mask,
> +                            *pff[i].spec);
> +
> +               append = true;
> +       }
> +
> +       return buf;
> +}
> +
>  static noinline_for_stack
>  char *flags_string(char *buf, char *end, void *flags_ptr,
>                    struct printf_spec spec, const char *fmt)
> @@ -1929,10 +1990,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
>         switch (fmt[1]) {
>         case 'p':
>                 flags = *(unsigned long *)flags_ptr;
> -               /* Remove zone id */
> -               flags &= (1UL << NR_PAGEFLAGS) - 1;
> -               names = pageflag_names;
> -               break;
> +               return format_page_flags(buf, end, flags);
>         case 'v':
>                 flags = *(unsigned long *)flags_ptr;
>                 names = vmaflag_names;
> --
> 2.26.2
>


-- 
Thanks
Yafang

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

[Yafang Shao]

On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2021-02-09 18:56:13, 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.
> >
> > To be compitable with the existed format of pGp, the new introduced ones
> > also use '|' as the separator, then the user tools parsing pGp won't
> > need to make change, suggested by Matthew. The new information is
> > tracked onto the end of the existed one.
> >
> > On example of the output in mm/slub.c as follows,
> > - Before the patch,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> >
> > The documentation and test cases are also updated. The output of the
> > test cases as follows,
> > [  501.485081] test_printf: loaded.
> > [  501.485768] test_printf: all 388 tests passed
> > [  501.488762] test_printf: unloaded.
> >
>
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 14c9a6af1b23..3f26611adb34 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1916,6 +1916,66 @@ 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 flags)
> > +{
> > +     DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> > +     unsigned long last;
> > +     int i;
> > +
> > +     if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> > +             if (buf < end)
> > +                     *buf = '|';
> > +             buf++;
> > +     }
>
> This is far from obvious. You print '|' here because you printed
> something somewhere else. See below.
>
> > +
> > +     for (i = 0; i < ARRAY_SIZE(pfl); i++)
> > +             __assign_bit(i, mask, pfl[i].width);
>
> The bitmap looks like an overkill. If I get it correctly, it is a
> tricky way to handle only flags defined by the used build
> configuration. See below.
>
> > +     last = find_last_bit(mask, ARRAY_SIZE(pfl));
> > +
> > +     for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> > +             /* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
> > +             buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > +
> > +             if (buf < end)
> > +                     *buf = '=';
> > +             buf++;
> > +             buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> > +                          *pfl[i].spec);
> > +
> > +             /* No separator for the last entry */
> > +             if (i != last) {
> > +                     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,10 +1989,10 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
> >       switch (fmt[1]) {
> >       case 'p':
> >               flags = *(unsigned long *)flags_ptr;
> > -             /* Remove zone id */
> > -             flags &= (1UL << NR_PAGEFLAGS) - 1;
> >               names = pageflag_names;
>
> The "names" variable is needed only with "break;" when using the final
> format_flags(buf, end, flags, names);
>
> > -             break;
> > +             buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) - 1), names);
> > +             buf = format_page_flags(buf, end, flags);
>
> I am sorry for my ignorance. I am not familiar with MM.
> But it is pretty hard to understand what call does what.
>
> I have found the following comment in include/linux/page_flags.h:
>
>  * The page flags field is split into two parts, the main flags area
>  * which extends from the low bits upwards, and the fields area which
>  * extends from the high bits downwards.
>
> Sigh, I know that you already reworked this several times because
> people "nitpicked" about the code style. But it seems that it
> rather diverged instead of converged.
>
> What about the following?
>
> Note: It is inpired by the names "main area" and "fields area"
>       mentioned in the above comment from page_flags.h.
>       I have later realized that "page_flags_layout" actually made
>       sense as well. Feel free to rename page_flags_fileds
>       back to page_flags_layout.
>
> Anyway, this is my proposal:
>

This proposal is similar to v2.
I don't mind changing it back with your additional better naming.
By the way, it will be better to make a little change per Joe's
suggestion on v2 that using a pointer instead of the index, for
example,

 for (p = pff; p < pff + ARRAY_SIZE(pff); p++) {

I will leave it for a moment in case others have a different opinion.

Anyway I like your proposal.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..cf67b39d72ae 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1916,6 +1916,67 @@ char *format_flags(char *buf, char *end, unsigned long flags,
>         return buf;
>  }
>
> +/* Meta information for page flags in the fields area */
> +struct page_flags_fields {
> +       int width;
> +       int shift;
> +       int mask;
> +       const struct printf_spec *spec;
> +       const char *name;
> +};
> +
> +static const struct page_flags_fields pff[] = {
> +       {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 flags)
> +{
> +       unsigned long main_flags = flags & (BIT(NR_PAGEFLAGS) - 1);
> +       bool append = false;
> +       int i;
> +
> +       /* Page flags from the main area. */
> +       if (main_flags) {
> +               buf = format_flags(buf, end, main_flags, pageflag_names);
> +               append = true;
> +       }
> +
> +       /* Page flags from the fields area */
> +       for (i = 0; i < ARRAY_SIZE(pff); i++) {
> +               /* Skip undefined fields. */
> +               if (!pff[i].width)
> +                       continue;
> +
> +               /* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
> +               if (append) {
> +                       if (buf < end)
> +                               *buf = '|';
> +                       buf++;
> +               }
> +
> +               buf = string(buf, end, pff[i].name, *pff[i].spec);
> +               if (buf < end)
> +                       *buf = '=';
> +               buf++;
> +               buf = number(buf, end, (flags >> pff[i].shift) & pff[i].mask,
> +                            *pff[i].spec);
> +
> +               append = true;
> +       }
> +
> +       return buf;
> +}
> +
>  static noinline_for_stack
>  char *flags_string(char *buf, char *end, void *flags_ptr,
>                    struct printf_spec spec, const char *fmt)
> @@ -1929,10 +1990,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
>         switch (fmt[1]) {
>         case 'p':
>                 flags = *(unsigned long *)flags_ptr;
> -               /* Remove zone id */
> -               flags &= (1UL << NR_PAGEFLAGS) - 1;
> -               names = pageflag_names;
> -               break;
> +               return format_page_flags(buf, end, flags);
>         case 'v':
>                 flags = *(unsigned long *)flags_ptr;
>                 names = vmaflag_names;
> --
> 2.26.2
>


-- 
Thanks
Yafang

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

[Petr Mladek]

On Wed 2021-02-10 00:21:37, Yafang Shao wrote:
> On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Tue 2021-02-09 18:56:13, 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.
> > >
> > > To be compitable with the existed format of pGp, the new introduced ones
> > > also use '|' as the separator, then the user tools parsing pGp won't
> > > need to make change, suggested by Matthew. The new information is
> > > tracked onto the end of the existed one.
> > >
> > > On example of the output in mm/slub.c as follows,
> > > - Before the patch,
> > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > >
> > > - After the patch,
> > > [ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> > >
> > > The documentation and test cases are also updated. The output of the
> > > test cases as follows,
> > > [  501.485081] test_printf: loaded.
> > > [  501.485768] test_printf: all 388 tests passed
> > > [  501.488762] test_printf: unloaded.
> > >
> >
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index 14c9a6af1b23..3f26611adb34 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -1916,6 +1916,66 @@ 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 flags)
> > > +{
> > > +     DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> > > +     unsigned long last;
> > > +     int i;
> > > +
> > > +     if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> > > +             if (buf < end)
> > > +                     *buf = '|';
> > > +             buf++;
> > > +     }
> >
> > This is far from obvious. You print '|' here because you printed
> > something somewhere else. See below.
> >
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(pfl); i++)
> > > +             __assign_bit(i, mask, pfl[i].width);
> >
> > The bitmap looks like an overkill. If I get it correctly, it is a
> > tricky way to handle only flags defined by the used build
> > configuration. See below.
> >
> > > +     last = find_last_bit(mask, ARRAY_SIZE(pfl));
> > > +
> > > +     for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> > > +             /* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
> > > +             buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > > +
> > > +             if (buf < end)
> > > +                     *buf = '=';
> > > +             buf++;
> > > +             buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> > > +                          *pfl[i].spec);
> > > +
> > > +             /* No separator for the last entry */
> > > +             if (i != last) {
> > > +                     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,10 +1989,10 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
> > >       switch (fmt[1]) {
> > >       case 'p':
> > >               flags = *(unsigned long *)flags_ptr;
> > > -             /* Remove zone id */
> > > -             flags &= (1UL << NR_PAGEFLAGS) - 1;
> > >               names = pageflag_names;
> >
> > The "names" variable is needed only with "break;" when using the final
> > format_flags(buf, end, flags, names);
> >
> > > -             break;
> > > +             buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) - 1), names);
> > > +             buf = format_page_flags(buf, end, flags);
> >
> > I am sorry for my ignorance. I am not familiar with MM.
> > But it is pretty hard to understand what call does what.
> >
> > I have found the following comment in include/linux/page_flags.h:
> >
> >  * The page flags field is split into two parts, the main flags area
> >  * which extends from the low bits upwards, and the fields area which
> >  * extends from the high bits downwards.
> >
> > Sigh, I know that you already reworked this several times because
> > people "nitpicked" about the code style. But it seems that it
> > rather diverged instead of converged.
> >
> > What about the following?
> >
> > Note: It is inpired by the names "main area" and "fields area"
> >       mentioned in the above comment from page_flags.h.
> >       I have later realized that "page_flags_layout" actually made
> >       sense as well. Feel free to rename page_flags_fileds
> >       back to page_flags_layout.
> >
> > Anyway, this is my proposal:
> >
> 
> This proposal is similar to v2.
> I don't mind changing it back with your additional better naming.

Great.

> By the way, it will be better to make a little change per Joe's
> suggestion on v2 that using a pointer instead of the index, for
> example,
> 
>  for (p = pff; p < pff + ARRAY_SIZE(pff); p++) {

This looks a bit non-standard. IMHO, Joe was not against using index.
He proposed:

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

, see
https://lore.kernel.org/lkml/e5ea9e8b1190c2a397a1b84dd55bb9c706dc7058.camel@perches.com/

I am not sure about the (buf < end) check. It might be some
optimization or it did fit the the old code.

Anyway, I like the currently used:

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

It is standard, easy to understand, and thus more safe. I am sure that
compiler will optimize it very well.

Best Regards,
Petr

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

[Joe Perches]

On Wed, 2021-02-10 at 13:51 +0100, Petr Mladek wrote:
> On Wed 2021-02-10 00:21:37, Yafang Shao wrote:
> > On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek <pmladek@suse.com> wrote:
[]
> >  for (p = pff; p < pff + ARRAY_SIZE(pff); p++) {
> 
> This looks a bit non-standard. IMHO, Joe was not against using index.
> He proposed:
> 
> 	for (i = 0; i < ARRAY_SIZE(pfl) && buf < end; i++) {
> 
> , see
> https://lore.kernel.org/lkml/e5ea9e8b1190c2a397a1b84dd55bb9c706dc7058.camel@perches.com/
> 
> I am not sure about the (buf < end) check. It might be some
> optimization or it did fit the the old code.

I believe the buf < end bit was broken anyway.

I believe vsprintf is supposed to return the maximum possible length
of the output and the function should not restrict that.  The
function should not write beyond the specified end.
 
> Anyway, I like the currently used:
> 
> 	for (i = 0; i < ARRAY_SIZE(pff); i++) {
> 
> It is standard, easy to understand, and thus more safe. I am sure that
> compiler will optimize it very well.

true.

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

[Joe Perches]

On Wed, 2021-02-10 at 13:51 +0100, Petr Mladek wrote:
> On Wed 2021-02-10 00:21:37, Yafang Shao wrote:
> > On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek <pmladek@suse.com> wrote:
[]
> >  for (p = pff; p < pff + ARRAY_SIZE(pff); p++) {
> 
> This looks a bit non-standard. IMHO, Joe was not against using index.
> He proposed:
> 
> 	for (i = 0; i < ARRAY_SIZE(pfl) && buf < end; i++) {
> 
> , see
> https://lore.kernel.org/lkml/e5ea9e8b1190c2a397a1b84dd55bb9c706dc7058.camel@perches.com/
> 
> I am not sure about the (buf < end) check. It might be some
> optimization or it did fit the the old code.

I believe the buf < end bit was broken anyway.

I believe vsprintf is supposed to return the maximum possible length
of the output and the function should not restrict that.  The
function should not write beyond the specified end.
 
> Anyway, I like the currently used:
> 
> 	for (i = 0; i < ARRAY_SIZE(pff); i++) {
> 
> It is standard, easy to understand, and thus more safe. I am sure that
> compiler will optimize it very well.

true.

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

[Yafang Shao]

On Wed, Feb 10, 2021 at 8:51 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2021-02-10 00:21:37, Yafang Shao wrote:
> > On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > On Tue 2021-02-09 18:56:13, 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.
> > > >
> > > > To be compitable with the existed format of pGp, the new introduced ones
> > > > also use '|' as the separator, then the user tools parsing pGp won't
> > > > need to make change, suggested by Matthew. The new information is
> > > > tracked onto the end of the existed one.
> > > >
> > > > On example of the output in mm/slub.c as follows,
> > > > - Before the patch,
> > > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > > >
> > > > - After the patch,
> > > > [ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> > > >
> > > > The documentation and test cases are also updated. The output of the
> > > > test cases as follows,
> > > > [  501.485081] test_printf: loaded.
> > > > [  501.485768] test_printf: all 388 tests passed
> > > > [  501.488762] test_printf: unloaded.
> > > >
> > >
> > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > > index 14c9a6af1b23..3f26611adb34 100644
> > > > --- a/lib/vsprintf.c
> > > > +++ b/lib/vsprintf.c
> > > > @@ -1916,6 +1916,66 @@ 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 flags)
> > > > +{
> > > > +     DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> > > > +     unsigned long last;
> > > > +     int i;
> > > > +
> > > > +     if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> > > > +             if (buf < end)
> > > > +                     *buf = '|';
> > > > +             buf++;
> > > > +     }
> > >
> > > This is far from obvious. You print '|' here because you printed
> > > something somewhere else. See below.
> > >
> > > > +
> > > > +     for (i = 0; i < ARRAY_SIZE(pfl); i++)
> > > > +             __assign_bit(i, mask, pfl[i].width);
> > >
> > > The bitmap looks like an overkill. If I get it correctly, it is a
> > > tricky way to handle only flags defined by the used build
> > > configuration. See below.
> > >
> > > > +     last = find_last_bit(mask, ARRAY_SIZE(pfl));
> > > > +
> > > > +     for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> > > > +             /* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
> > > > +             buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > > > +
> > > > +             if (buf < end)
> > > > +                     *buf = '=';
> > > > +             buf++;
> > > > +             buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> > > > +                          *pfl[i].spec);
> > > > +
> > > > +             /* No separator for the last entry */
> > > > +             if (i != last) {
> > > > +                     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,10 +1989,10 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
> > > >       switch (fmt[1]) {
> > > >       case 'p':
> > > >               flags = *(unsigned long *)flags_ptr;
> > > > -             /* Remove zone id */
> > > > -             flags &= (1UL << NR_PAGEFLAGS) - 1;
> > > >               names = pageflag_names;
> > >
> > > The "names" variable is needed only with "break;" when using the final
> > > format_flags(buf, end, flags, names);
> > >
> > > > -             break;
> > > > +             buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) - 1), names);
> > > > +             buf = format_page_flags(buf, end, flags);
> > >
> > > I am sorry for my ignorance. I am not familiar with MM.
> > > But it is pretty hard to understand what call does what.
> > >
> > > I have found the following comment in include/linux/page_flags.h:
> > >
> > >  * The page flags field is split into two parts, the main flags area
> > >  * which extends from the low bits upwards, and the fields area which
> > >  * extends from the high bits downwards.
> > >
> > > Sigh, I know that you already reworked this several times because
> > > people "nitpicked" about the code style. But it seems that it
> > > rather diverged instead of converged.
> > >
> > > What about the following?
> > >
> > > Note: It is inpired by the names "main area" and "fields area"
> > >       mentioned in the above comment from page_flags.h.
> > >       I have later realized that "page_flags_layout" actually made
> > >       sense as well. Feel free to rename page_flags_fileds
> > >       back to page_flags_layout.
> > >
> > > Anyway, this is my proposal:
> > >
> >
> > This proposal is similar to v2.
> > I don't mind changing it back with your additional better naming.
>
> Great.
>
> > By the way, it will be better to make a little change per Joe's
> > suggestion on v2 that using a pointer instead of the index, for
> > example,
> >
> >  for (p = pff; p < pff + ARRAY_SIZE(pff); p++) {
>
> This looks a bit non-standard. IMHO, Joe was not against using index.
> He proposed:
>
>         for (i = 0; i < ARRAY_SIZE(pfl) && buf < end; i++) {
>
> , see
> https://lore.kernel.org/lkml/e5ea9e8b1190c2a397a1b84dd55bb9c706dc7058.camel@perches.com/
>
> I am not sure about the (buf < end) check. It might be some
> optimization or it did fit the the old code.
>
> Anyway, I like the currently used:
>
>         for (i = 0; i < ARRAY_SIZE(pff); i++) {
>
> It is standard, easy to understand, and thus more safe. I am sure that
> compiler will optimize it very well.
>

Fair enough, I will do it as you suggested in the next version.

-- 
Thanks
Yafang

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

[Yafang Shao]

On Wed, Feb 10, 2021 at 8:51 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2021-02-10 00:21:37, Yafang Shao wrote:
> > On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > On Tue 2021-02-09 18:56:13, 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.
> > > >
> > > > To be compitable with the existed format of pGp, the new introduced ones
> > > > also use '|' as the separator, then the user tools parsing pGp won't
> > > > need to make change, suggested by Matthew. The new information is
> > > > tracked onto the end of the existed one.
> > > >
> > > > On example of the output in mm/slub.c as follows,
> > > > - Before the patch,
> > > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > > >
> > > > - After the patch,
> > > > [ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> > > >
> > > > The documentation and test cases are also updated. The output of the
> > > > test cases as follows,
> > > > [  501.485081] test_printf: loaded.
> > > > [  501.485768] test_printf: all 388 tests passed
> > > > [  501.488762] test_printf: unloaded.
> > > >
> > >
> > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > > index 14c9a6af1b23..3f26611adb34 100644
> > > > --- a/lib/vsprintf.c
> > > > +++ b/lib/vsprintf.c
> > > > @@ -1916,6 +1916,66 @@ 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 flags)
> > > > +{
> > > > +     DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> > > > +     unsigned long last;
> > > > +     int i;
> > > > +
> > > > +     if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> > > > +             if (buf < end)
> > > > +                     *buf = '|';
> > > > +             buf++;
> > > > +     }
> > >
> > > This is far from obvious. You print '|' here because you printed
> > > something somewhere else. See below.
> > >
> > > > +
> > > > +     for (i = 0; i < ARRAY_SIZE(pfl); i++)
> > > > +             __assign_bit(i, mask, pfl[i].width);
> > >
> > > The bitmap looks like an overkill. If I get it correctly, it is a
> > > tricky way to handle only flags defined by the used build
> > > configuration. See below.
> > >
> > > > +     last = find_last_bit(mask, ARRAY_SIZE(pfl));
> > > > +
> > > > +     for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> > > > +             /* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
> > > > +             buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > > > +
> > > > +             if (buf < end)
> > > > +                     *buf = '=';
> > > > +             buf++;
> > > > +             buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> > > > +                          *pfl[i].spec);
> > > > +
> > > > +             /* No separator for the last entry */
> > > > +             if (i != last) {
> > > > +                     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,10 +1989,10 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
> > > >       switch (fmt[1]) {
> > > >       case 'p':
> > > >               flags = *(unsigned long *)flags_ptr;
> > > > -             /* Remove zone id */
> > > > -             flags &= (1UL << NR_PAGEFLAGS) - 1;
> > > >               names = pageflag_names;
> > >
> > > The "names" variable is needed only with "break;" when using the final
> > > format_flags(buf, end, flags, names);
> > >
> > > > -             break;
> > > > +             buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) - 1), names);
> > > > +             buf = format_page_flags(buf, end, flags);
> > >
> > > I am sorry for my ignorance. I am not familiar with MM.
> > > But it is pretty hard to understand what call does what.
> > >
> > > I have found the following comment in include/linux/page_flags.h:
> > >
> > >  * The page flags field is split into two parts, the main flags area
> > >  * which extends from the low bits upwards, and the fields area which
> > >  * extends from the high bits downwards.
> > >
> > > Sigh, I know that you already reworked this several times because
> > > people "nitpicked" about the code style. But it seems that it
> > > rather diverged instead of converged.
> > >
> > > What about the following?
> > >
> > > Note: It is inpired by the names "main area" and "fields area"
> > >       mentioned in the above comment from page_flags.h.
> > >       I have later realized that "page_flags_layout" actually made
> > >       sense as well. Feel free to rename page_flags_fileds
> > >       back to page_flags_layout.
> > >
> > > Anyway, this is my proposal:
> > >
> >
> > This proposal is similar to v2.
> > I don't mind changing it back with your additional better naming.
>
> Great.
>
> > By the way, it will be better to make a little change per Joe's
> > suggestion on v2 that using a pointer instead of the index, for
> > example,
> >
> >  for (p = pff; p < pff + ARRAY_SIZE(pff); p++) {
>
> This looks a bit non-standard. IMHO, Joe was not against using index.
> He proposed:
>
>         for (i = 0; i < ARRAY_SIZE(pfl) && buf < end; i++) {
>
> , see
> https://lore.kernel.org/lkml/e5ea9e8b1190c2a397a1b84dd55bb9c706dc7058.camel@perches.com/
>
> I am not sure about the (buf < end) check. It might be some
> optimization or it did fit the the old code.
>
> Anyway, I like the currently used:
>
>         for (i = 0; i < ARRAY_SIZE(pff); i++) {
>
> It is standard, easy to understand, and thus more safe. I am sure that
> compiler will optimize it very well.
>

Fair enough, I will do it as you suggested in the next version.

-- 
Thanks
Yafang

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

[Andy Shevchenko]

On Tue, Feb 09, 2021 at 06:56:10PM +0800, Yafang Shao wrote:
> The existed pGp shows the names of page flags only, rather than the full
> information including section, node, zone, last cpuipid and kasan tag.
> While it is not easy to parse these information manually because there
> are so many flavors. We'd better interpret them in printf.
> 
> To be compitable with the existed format of pGp, the new introduced ones
> also use '|' as the separator, then the user tools parsing pGp won't
> need to make change, suggested by Matthew. The new added information is
> tracked onto the end of the existed one, e.g.
> [ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> 
> The documentation and test cases are also updated. The result of the
> test cases as follows,
> [  501.485081] test_printf: loaded.
> [  501.485768] test_printf: all 388 tests passed
> [  501.488762] test_printf: unloaded.
> 
> This patchset also includes some code cleanup in mm/slub.c.

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> v4:
> - extend %pGp instead of introducing new format, per Matthew
> 
> v3:
> - coding improvement, per Joe and Andy
> - the possible impact on debugfs and the fix of it, per Joe and Matthew
> - introduce new format instead of changing pGp, per Andy
> 
> 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                         | 60 +++++++++++++++++----
>  lib/vsprintf.c                            | 66 +++++++++++++++++++++--
>  mm/slub.c                                 | 13 ++---
>  4 files changed, 121 insertions(+), 20 deletions(-)
> 
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko