[PATCH v3 0/3] mm, vsprintf: introduce new format to dump full information of page flags

[PATCH v3 0/3] mm, vsprintf: introduce new format to dump full information of page flags

[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 avoid breaking some tools which parsing pGp via debugfs or affecting
the printf buffer, other new formats are introduced, so the user can choose
what and in which order they want, suggested by Andy. These new introduced
format as follows,
    pGpb: print other information first and then the names of page flags
    pGpl: print the names of page flags first and then the other info

The differece between them looks like the difference between big-endian and
little-endian, that's why they are named like that. The examples of the
output as follows,
    %pGpb 0x17ffffc0010200(node=0|zone=2|lastcpupid=0x1fffff|slab|head)
    %pGpl 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)

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 doc and test cases are also updated. Below is the output of the
test cases,
[ 4299.847655] test_printf: loaded.
[ 4299.848301] test_printf: all 404 tests passed
[ 4299.850371] test_printf: unloaded.

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

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: introduce new format to dump full information of page flags

 Documentation/core-api/printk-formats.rst |   2 +
 lib/test_printf.c                         | 126 +++++++++++++++++++---
 lib/vsprintf.c                            | 115 +++++++++++++++++++-
 mm/slub.c                                 |  13 +--
 4 files changed, 233 insertions(+), 23 deletions(-)

-- 
2.17.1

[PATCH v3 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>
Acked-by: Christoph Lameter <cl@linux.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 v3 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 v3 3/3] vsprintf: introduce new format to dump full information of page flags

[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 avoid breaking some tools which parsing pGp via debugfs or affecting
the printf buffer, other new formats are introduced, so the user can choose
what and in which order they want, suggested by Andy. These new introduced
format as follows,
    pGpb: print other information first and then the names of page flags
    pGpl: print the names of page flags first and then the other info

The differece between them looks like the difference between big-endian and
little-endian, that's why they are named like that. The examples of the
output as follows,
    %pGpb 0x17ffffc0010200(node=0|zone=2|lastcpupid=0x1fffff|slab|head)
    %pGpl 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)

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 doc and test cases are also updated. Below is the output of the
test cases,
[ 4299.847655] test_printf: loaded.
[ 4299.848301] test_printf: all 404 tests passed
[ 4299.850371] test_printf: unloaded.

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>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 Documentation/core-api/printk-formats.rst |   2 +
 lib/test_printf.c                         | 126 +++++++++++++++++++---
 lib/vsprintf.c                            | 115 +++++++++++++++++++-
 3 files changed, 226 insertions(+), 17 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 6d26c5c6ac48..56f8e0fc3963 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -539,6 +539,8 @@ Flags bitfields such as page flags, gfp_flags
 ::
 
 	%pGp	referenced|uptodate|lru|active|private
+	%pGpb	node=0|zone=2|referenced|uptodate|lru|active|private
+	%pGpl	referenced|uptodate|lru|active|private|node=0|zone=2
 	%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..af2945bb730a 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -569,24 +569,130 @@ netdev_features(void)
 {
 }
 
+static void  __init
+page_flags_build_info(char *cmp_buf, int prefix, int sec, int node, int zone,
+		      int last_cpupid, int kasan_tag, unsigned long *flags)
+{
+	unsigned long size = prefix;
+
+#ifdef SECTION_IN_PAGE_FLAGS
+	*flags |= (sec & SECTIONS_MASK) << SECTIONS_PGSHIFT;
+	snprintf(cmp_buf + size, BUF_SIZE - size, "section=%#x|", sec);
+	size = strlen(cmp_buf);
+#endif
+
+	*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
+	*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)
+	*flags |= (tag & KASAN_TAG_MASK) << KASAN_TAG_PGSHIFT;
+	snprintf(cmp_buf + size, BUF_SIZE - size, "|kasantag=%#x", tag);
+	size = strlen(cmp_buf);
+#endif
+}
+
 static void __init
-flags(void)
+page_flags_build_names(char *cmp_buf, int prefix, const char *expect,
+		       unsigned long flags, unsigned long *page_flags)
 {
+	*page_flags |= flags;
+	snprintf(cmp_buf + prefix, BUF_SIZE - prefix, "%s", expect);
+}
+
+static void __init
+__page_flags_test(const char *expect, unsigned long flags)
+{
+	test(expect, "%pGp", &flags);
+}
+
+static void __init
+page_flags_test_be(char *cmp_buf, int sec, int node, int zone,
+		   int last_cpupid, int kasan_tag, const char *name,
+		   unsigned long flags)
+{
+	unsigned long page_flags = 0;
+	int size;
+
+	page_flags_build_info(cmp_buf, 0, sec, node, zone, last_cpupid,
+			      kasan_tag, &page_flags);
+
+	if (*name) {
+		size = strlen(cmp_buf);
+		if (size < BUF_SIZE - 2) {
+			*(cmp_buf + size) = '|';
+			*(cmp_buf + size + 1) = '\0';
+		}
+		page_flags_build_names(cmp_buf, strlen(cmp_buf), name, flags, &page_flags);
+	}
+
+	test(cmp_buf, "%pGpb", &page_flags);
+}
+
+static void __init
+page_flags_test_le(char *cmp_buf, int sec, int node, int zone,
+		   int last_cpupid, int kasan_tag, const char *name,
+		   unsigned long flags)
+{
+	unsigned long page_flags = 0;
+	int size = 0;
+
+	if (*name) {
+		page_flags_build_names(cmp_buf, 0, name, flags, &page_flags);
+		size = strlen(cmp_buf);
+		if (size < BUF_SIZE - 2) {
+			*(cmp_buf + size) = '|';
+			*(cmp_buf + size + 1) = '\0';
+		}
+		size = strlen(cmp_buf);
+	}
+
+	page_flags_build_info(cmp_buf, size, sec, node, zone, last_cpupid,
+			      kasan_tag, &page_flags);
+
+	test(cmp_buf, "%pGpl", &page_flags);
+}
+
+static void __init
+page_flags_test(char *cmp_buf)
+{
+	char *name = "uptodate|dirty|lru|active|swapbacked";
 	unsigned long flags;
-	gfp_t gfp;
-	char *cmp_buffer;
 
 	flags = 0;
-	test("", "%pGp", &flags);
+	__page_flags_test("", flags);
+	page_flags_test_be(cmp_buf, 0, 0, 0, 0, 0, "", flags);
+	page_flags_test_le(cmp_buf, 0, 0, 0, 0, 0, "", flags);
 
-	/* Page flags should filter the zone id */
 	flags = 1UL << NR_PAGEFLAGS;
-	test("", "%pGp", &flags);
+	__page_flags_test("", 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);
+		 | 1UL << PG_active | 1UL << PG_swapbacked;
+	__page_flags_test(name, flags);
+	page_flags_test_be(cmp_buf, 1, 1, 1, 0x1fffff, 1, name, flags);
+	page_flags_test_le(cmp_buf, 1, 1, 1, 0x1fffff, 1, name, flags);
+}
+
+static void __init
+flags(void)
+{
+	unsigned long flags;
+	gfp_t gfp;
+	char *cmp_buffer;
 
+	cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
+	if (!cmp_buffer)
+		return;
+
+	page_flags_test(cmp_buffer);
 
 	flags = VM_READ | VM_EXEC | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC
 			| VM_DENYWRITE;
@@ -601,10 +707,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..c912cc9bddb0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1916,6 +1916,115 @@ 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;
+
+	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
+char *format_page_flags_be(char *buf, char *end, unsigned long flags)
+{
+	unsigned long mask = BIT(NR_PAGEFLAGS) - 1;
+
+	buf = __format_page_flags(buf, end, flags);
+
+	if (flags & mask) {
+		if (buf < end)
+			*buf = '|';
+		buf++;
+	}
+
+	return format_flags(buf, end, flags & mask, pageflag_names);
+}
+
+static
+char *format_page_flags_le(char *buf, char *end, unsigned long flags)
+{
+	unsigned long mask = BIT(NR_PAGEFLAGS) - 1;
+
+	buf = format_flags(buf, end, flags & mask, pageflag_names);
+
+	if (flags & mask) {
+		if (buf < end)
+			*buf = '|';
+		buf++;
+	}
+
+	return __format_page_flags(buf, end, flags & ~mask);
+}
+
+static
+char *format_page_flags(char *buf, char *end, void *flags_ptr,
+			struct printf_spec spec, const char *fmt)
+{
+	unsigned long flags = *(unsigned long *)flags_ptr;
+	unsigned long mask = BIT(NR_PAGEFLAGS) - 1;
+
+	if (strlen(fmt) == 2) {
+		flags &= mask;
+		return format_flags(buf, end, flags, pageflag_names);
+	}
+
+	switch (fmt[2]) {
+	case 'b':
+		return format_page_flags_be(buf, end, flags);
+	case 'l':
+		return format_page_flags_le(buf, end, flags);
+	default:
+		flags &= mask;
+		return format_flags(buf, end, flags, pageflag_names);
+	}
+}
+
 static noinline_for_stack
 char *flags_string(char *buf, char *end, void *flags_ptr,
 		   struct printf_spec spec, const char *fmt)
@@ -1928,11 +2037,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_ptr, spec, fmt);
 	case 'v':
 		flags = *(unsigned long *)flags_ptr;
 		names = vmaflag_names;
-- 
2.17.1

Re: [PATCH v3 0/3] mm, vsprintf: introduce new format to dump full information of page flags

[Matthew Wilcox]

On Mon, Feb 08, 2021 at 06:14:36PM +0800, Yafang Shao wrote:
> To avoid breaking some tools which parsing pGp via debugfs or affecting
> the printf buffer, other new formats are introduced, so the user can choose
> what and in which order they want, suggested by Andy. These new introduced
> format as follows,
>     pGpb: print other information first and then the names of page flags
>     pGpl: print the names of page flags first and then the other info

This is overengineering things.  We already print in little-endian order,
and the other information should be tacked onto the end.  Just extend
%pGp.  Andy's suggestion to add another flag was a bad one.

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

[Matthew Wilcox]

On Mon, Feb 08, 2021 at 06:14:37PM +0800, 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)
> 
> 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>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Re: [PATCH v3 0/3] mm, vsprintf: introduce new format to dump full information of page flags

[Andy Shevchenko]

On Mon, Feb 8, 2021 at 2:37 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Feb 08, 2021 at 06:14:36PM +0800, Yafang Shao wrote:
> > To avoid breaking some tools which parsing pGp via debugfs or affecting
> > the printf buffer, other new formats are introduced, so the user can choose
> > what and in which order they want, suggested by Andy. These new introduced
> > format as follows,
> >     pGpb: print other information first and then the names of page flags
> >     pGpl: print the names of page flags first and then the other info
>
> This is overengineering things.  We already print in little-endian order,
> and the other information should be tacked onto the end.  Just extend
> %pGp.  Andy's suggestion to add another flag was a bad one.

Fair enough, I can admit this because I don't know the mm specifics.
However, my initial point was about the long message which might be
cut into pieces during Oops or whatever and the user will get the
first parts only.  Depends on what you consider as a higher priority
to print the order might be different. Initial patch suggested to
print the new fields first.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/3] mm, vsprintf: introduce new format to dump full information of page flags

[Yafang Shao]

On Mon, Feb 8, 2021 at 8:20 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Feb 08, 2021 at 06:14:36PM +0800, Yafang Shao wrote:
> > To avoid breaking some tools which parsing pGp via debugfs or affecting
> > the printf buffer, other new formats are introduced, so the user can choose
> > what and in which order they want, suggested by Andy. These new introduced
> > format as follows,
> >     pGpb: print other information first and then the names of page flags
> >     pGpl: print the names of page flags first and then the other info
>
> This is overengineering things.  We already print in little-endian order,
> and the other information should be tacked onto the end.  Just extend
> %pGp.  Andy's suggestion to add another flag was a bad one.
>

All right. I will do it in the next version.

-- 
Thanks
Yafang

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

[Vlastimil Babka]

On 2/8/21 11:14 AM, 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>
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

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

> ---
>  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);
>  		}
>  	}
> 

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

[Matthew Wilcox]

On Mon, Feb 08, 2021 at 06:14:38PM +0800, Yafang Shao wrote:
> It is strange to combine "pr_err" with "INFO", so let's remove the
> prefix completely.

So is this the right thing to do?  Should it be pr_info() instead?
Many of these messages do not appear to be error messages, but
rather informational messages.

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

[Vlastimil Babka]

On 2/8/21 6:26 PM, Matthew Wilcox wrote:
> On Mon, Feb 08, 2021 at 06:14:38PM +0800, Yafang Shao wrote:
>> It is strange to combine "pr_err" with "INFO", so let's remove the
>> prefix completely.
> 
> So is this the right thing to do?  Should it be pr_info() instead?
> Many of these messages do not appear to be error messages, but
> rather informational messages.

They are all part of longer error reports. See
https://lore.kernel.org/linux-mm/0b2f4419-06a9-0b6c-067b-8d0848e78c33@suse.cz/

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

[Miaohe Lin]

On 2021/2/8 18:14, 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)
> 
> 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>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

LGTM. Thanks.
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);
>  
>  }
>  
> 

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

[Miaohe Lin]

On 2021/2/8 18:14, 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>
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Looks good,thanks.
Reviewed-by: Miaohe Lin <linmiaohe@huawei.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);
>  		}
>  	}
>