[PATCH 0/5] Improvements to %pGp

[PATCH 0/5] Improvements to %pGp

[Matthew Wilcox (Oracle)]

The first four patches are improvements to page_flags_test().
The fifth is the patch I originally sent out which exposed some of
the weaknesses in page_flags_test().

Matthew Wilcox (Oracle) (5):
  test_printf: Make pft array const
  test_printf: Assign all flags to page_flags
  test_printf: Remove custom appending of '|'
  test_printf: Append '|' more efficiently
  vsprintf: Make %pGp print the hex value

 lib/test_printf.c   | 48 +++++++++++++++++++++------------------------
 lib/vsprintf.c      |  8 ++++++++
 mm/debug.c          |  2 +-
 mm/memory-failure.c |  8 ++++----
 mm/page_owner.c     |  4 ++--
 mm/slub.c           |  4 ++--
 6 files changed, 39 insertions(+), 35 deletions(-)

-- 
2.32.0

[PATCH 1/5] test_printf: Make pft array const

[Matthew Wilcox (Oracle)]

Instead of assigning ptf[i].value, leave the values in the on-stack
array and then we can make the array const.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 lib/test_printf.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 55082432f37e..a52c1c3a55ba 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -586,22 +586,21 @@ struct page_flags_test {
 	int width;
 	int shift;
 	int mask;
-	unsigned long value;
 	const char *fmt;
 	const char *name;
 };
 
-static struct page_flags_test pft[] = {
+static const struct page_flags_test pft[] = {
 	{SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
-	 0, "%d", "section"},
+	 "%d", "section"},
 	{NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
-	 0, "%d", "node"},
+	 "%d", "node"},
 	{ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
-	 0, "%d", "zone"},
+	 "%d", "zone"},
 	{LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
-	 0, "%#x", "lastcpupid"},
+	 "%#x", "lastcpupid"},
 	{KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
-	 0, "%#x", "kasantag"},
+	 "%#x", "kasantag"},
 };
 
 static void __init
@@ -627,10 +626,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
 #endif
 	}
 
-	/* Set the test value */
-	for (i = 0; i < ARRAY_SIZE(pft); i++)
-		pft[i].value = values[i];
-
 	for (i = 0; i < ARRAY_SIZE(pft); i++) {
 		if (!pft[i].width)
 			continue;
@@ -640,11 +635,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
 			size = strlen(cmp_buf);
 		}
 
-		page_flags |= (pft[i].value & pft[i].mask) << pft[i].shift;
+		page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
 		snprintf(cmp_buf + size, BUF_SIZE - size, "%s=", pft[i].name);
 		size = strlen(cmp_buf);
 		snprintf(cmp_buf + size, BUF_SIZE - size, pft[i].fmt,
-			 pft[i].value & pft[i].mask);
+			 values[i] & pft[i].mask);
 		size = strlen(cmp_buf);
 		append = true;
 	}
-- 
2.32.0

[PATCH 2/5] test_printf: Assign all flags to page_flags

[Matthew Wilcox (Oracle)]

Even unknown flags should be passed to %pGp so that we can test what it
does with them (today: nothing).

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 lib/test_printf.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index a52c1c3a55ba..f744b0498672 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -608,14 +608,12 @@ 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 values[] = {section, node, zone, last_cpupid, kasan_tag};
-	unsigned long page_flags = 0;
+	unsigned long page_flags = flags;
 	unsigned long size = 0;
 	bool append = false;
 	int i;
 
-	flags &= PAGEFLAGS_MASK;
-	if (flags) {
-		page_flags |= flags;
+	if (flags & PAGEFLAGS_MASK) {
 		snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
 		size = strlen(cmp_buf);
 #if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \
-- 
2.32.0

[PATCH 3/5] test_printf: Remove custom appending of '|'

[Matthew Wilcox (Oracle)]

Instead of having an ifdef to decide whether to print a |, use the
'append' functionality of the main loop to print it.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 lib/test_printf.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index f744b0498672..60cdf4ba991e 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -616,12 +616,7 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
 	if (flags & PAGEFLAGS_MASK) {
 		snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
 		size = strlen(cmp_buf);
-#if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \
-	LAST_CPUPID_WIDTH || KASAN_TAG_WIDTH
-		/* Other information also included in page flags */
-		snprintf(cmp_buf + size, BUF_SIZE - size, "|");
-		size = strlen(cmp_buf);
-#endif
+		append = true;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(pft); i++) {
-- 
2.32.0

[PATCH 4/5] test_printf: Append '|' more efficiently

[Matthew Wilcox (Oracle)]

Instead of calling snprintf(), just append '|' by hand.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 lib/test_printf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 60cdf4ba991e..662c3785aa57 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
 		if (!pft[i].width)
 			continue;
 
-		if (append) {
-			snprintf(cmp_buf + size, BUF_SIZE - size, "|");
-			size = strlen(cmp_buf);
+		if (append && size < BUF_SIZE) {
+			cmp_buf[size++] = '|';
+			cmp_buf[size] = '\0';
 		}
 
 		page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
-- 
2.32.0

[PATCH 5/5] vsprintf: Make %pGp print the hex value

[Matthew Wilcox (Oracle)]

All existing users of %pGp want the hex value as well as the decoded
flag names.  This looks awkward (passing the same parameter to printf
twice), so move that functionality into the core.  If we want, we
can make that optional with flag arguments to %pGp in the future.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 lib/test_printf.c   | 10 +++++++++-
 lib/vsprintf.c      |  8 ++++++++
 mm/debug.c          |  2 +-
 mm/memory-failure.c |  8 ++++----
 mm/page_owner.c     |  4 ++--
 mm/slub.c           |  4 ++--
 6 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 662c3785aa57..a60b1a749e87 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -613,6 +613,10 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
 	bool append = false;
 	int i;
 
+	for (i = 0; i < ARRAY_SIZE(values); i++)
+		page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
+	snprintf(cmp_buf + size, BUF_SIZE - size, "%#lx(", page_flags);
+	size = strlen(cmp_buf);
 	if (flags & PAGEFLAGS_MASK) {
 		snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
 		size = strlen(cmp_buf);
@@ -628,7 +632,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
 			cmp_buf[size] = '\0';
 		}
 
-		page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
 		snprintf(cmp_buf + size, BUF_SIZE - size, "%s=", pft[i].name);
 		size = strlen(cmp_buf);
 		snprintf(cmp_buf + size, BUF_SIZE - size, pft[i].fmt,
@@ -637,6 +640,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
 		append = true;
 	}
 
+	if (size < BUF_SIZE) {
+		cmp_buf[size++] = ')';
+		cmp_buf[size] = '\0';
+	}
+
 	test(cmp_buf, "%pGp", &page_flags);
 }
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7ad44f2c8f5..214098248610 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2023,6 +2023,11 @@ char *format_page_flags(char *buf, char *end, unsigned long flags)
 	bool append = false;
 	int i;
 
+	buf = number(buf, end, flags, default_flag_spec);
+	if (buf < end)
+		*buf = '(';
+	buf++;
+
 	/* Page flags from the main area. */
 	if (main_flags) {
 		buf = format_flags(buf, end, main_flags, pageflag_names);
@@ -2051,6 +2056,9 @@ char *format_page_flags(char *buf, char *end, unsigned long flags)
 
 		append = true;
 	}
+	if (buf < end)
+		*buf = ')';
+	buf++;
 
 	return buf;
 }
diff --git a/mm/debug.c b/mm/debug.c
index fae0f81ad831..714be101dec9 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -162,7 +162,7 @@ static void __dump_page(struct page *page)
 out_mapping:
 	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
 
-	pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
+	pr_warn("%sflags: %pGp%s\n", type, &head->flags,
 		page_cma ? " CMA" : "");
 	print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
 			sizeof(unsigned long), page,
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 3e6449f2102a..e4e122a2e9b1 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2109,14 +2109,14 @@ static int __soft_offline_page(struct page *page)
 			if (!list_empty(&pagelist))
 				putback_movable_pages(&pagelist);
 
-			pr_info("soft offline: %#lx: %s migration failed %d, type %lx (%pGp)\n",
-				pfn, msg_page[huge], ret, page->flags, &page->flags);
+			pr_info("soft offline: %#lx: %s migration failed %d, type %pGp\n",
+				pfn, msg_page[huge], ret, &page->flags);
 			if (ret > 0)
 				ret = -EBUSY;
 		}
 	} else {
-		pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %lx (%pGp)\n",
-			pfn, msg_page[huge], page_count(page), page->flags, &page->flags);
+		pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %pGp\n",
+			pfn, msg_page[huge], page_count(page), &page->flags);
 		ret = -EBUSY;
 	}
 	return ret;
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 62402d22539b..4afc713ca525 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -351,12 +351,12 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	pageblock_mt = get_pageblock_migratetype(page);
 	page_mt  = gfp_migratetype(page_owner->gfp_mask);
 	ret += snprintf(kbuf + ret, count - ret,
-			"PFN %lu type %s Block %lu type %s Flags %#lx(%pGp)\n",
+			"PFN %lu type %s Block %lu type %s Flags %pGp\n",
 			pfn,
 			migratetype_names[page_mt],
 			pfn >> pageblock_order,
 			migratetype_names[pageblock_mt],
-			page->flags, &page->flags);
+			&page->flags);
 
 	if (ret >= count)
 		goto err;
diff --git a/mm/slub.c b/mm/slub.c
index 3d2025f7163b..f7ac28646580 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -763,9 +763,9 @@ void print_tracking(struct kmem_cache *s, void *object)
 
 static void print_page_info(struct page *page)
 {
-	pr_err("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=%pGp\n",
 	       page, page->objects, page->inuse, page->freelist,
-	       page->flags, &page->flags);
+	       &page->flags);
 
 }
 
-- 
2.32.0

Re: [PATCH 1/5] test_printf: Make pft array const

[Yafang Shao]

On Wed, Oct 13, 2021 at 2:29 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> Instead of assigning ptf[i].value, leave the values in the on-stack
> array and then we can make the array const.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Yafang Shao <laoar.shao@gmail.com>

> ---
>  lib/test_printf.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 55082432f37e..a52c1c3a55ba 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -586,22 +586,21 @@ struct page_flags_test {
>         int width;
>         int shift;
>         int mask;
> -       unsigned long value;
>         const char *fmt;
>         const char *name;
>  };
>
> -static struct page_flags_test pft[] = {
> +static const struct page_flags_test pft[] = {
>         {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
> -        0, "%d", "section"},
> +        "%d", "section"},
>         {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
> -        0, "%d", "node"},
> +        "%d", "node"},
>         {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
> -        0, "%d", "zone"},
> +        "%d", "zone"},
>         {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
> -        0, "%#x", "lastcpupid"},
> +        "%#x", "lastcpupid"},
>         {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
> -        0, "%#x", "kasantag"},
> +        "%#x", "kasantag"},
>  };
>
>  static void __init
> @@ -627,10 +626,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>  #endif
>         }
>
> -       /* Set the test value */
> -       for (i = 0; i < ARRAY_SIZE(pft); i++)
> -               pft[i].value = values[i];
> -
>         for (i = 0; i < ARRAY_SIZE(pft); i++) {
>                 if (!pft[i].width)
>                         continue;
> @@ -640,11 +635,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>                         size = strlen(cmp_buf);
>                 }
>
> -               page_flags |= (pft[i].value & pft[i].mask) << pft[i].shift;
> +               page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
>                 snprintf(cmp_buf + size, BUF_SIZE - size, "%s=", pft[i].name);
>                 size = strlen(cmp_buf);
>                 snprintf(cmp_buf + size, BUF_SIZE - size, pft[i].fmt,
> -                        pft[i].value & pft[i].mask);
> +                        values[i] & pft[i].mask);
>                 size = strlen(cmp_buf);
>                 append = true;
>         }
> --
> 2.32.0
>


-- 
Thanks
Yafang

Re: [PATCH 2/5] test_printf: Assign all flags to page_flags

[Yafang Shao]

On Wed, Oct 13, 2021 at 2:31 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> Even unknown flags should be passed to %pGp so that we can test what it
> does with them (today: nothing).
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Yafang Shao <laoar.shao@gmail.com>

> ---
>  lib/test_printf.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index a52c1c3a55ba..f744b0498672 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -608,14 +608,12 @@ 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 values[] = {section, node, zone, last_cpupid, kasan_tag};
> -       unsigned long page_flags = 0;
> +       unsigned long page_flags = flags;
>         unsigned long size = 0;
>         bool append = false;
>         int i;
>
> -       flags &= PAGEFLAGS_MASK;
> -       if (flags) {
> -               page_flags |= flags;
> +       if (flags & PAGEFLAGS_MASK) {
>                 snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
>                 size = strlen(cmp_buf);
>  #if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \
> --
> 2.32.0
>


-- 
Thanks
Yafang

Re: [PATCH 3/5] test_printf: Remove custom appending of '|'

[Yafang Shao]

On Wed, Oct 13, 2021 at 2:32 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> Instead of having an ifdef to decide whether to print a |, use the
> 'append' functionality of the main loop to print it.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Yafang Shao <laoar.shao@gmail.com>

> ---
>  lib/test_printf.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index f744b0498672..60cdf4ba991e 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -616,12 +616,7 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>         if (flags & PAGEFLAGS_MASK) {
>                 snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
>                 size = strlen(cmp_buf);
> -#if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \
> -       LAST_CPUPID_WIDTH || KASAN_TAG_WIDTH
> -               /* Other information also included in page flags */
> -               snprintf(cmp_buf + size, BUF_SIZE - size, "|");
> -               size = strlen(cmp_buf);
> -#endif
> +               append = true;
>         }
>
>         for (i = 0; i < ARRAY_SIZE(pft); i++) {
> --
> 2.32.0
>


-- 
Thanks
Yafang

Re: [PATCH 4/5] test_printf: Append '|' more efficiently

[Yafang Shao]

On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> Instead of calling snprintf(), just append '|' by hand.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  lib/test_printf.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 60cdf4ba991e..662c3785aa57 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>                 if (!pft[i].width)
>                         continue;
>
> -               if (append) {
> -                       snprintf(cmp_buf + size, BUF_SIZE - size, "|");
> -                       size = strlen(cmp_buf);
> +               if (append && size < BUF_SIZE) {

Should it be:
                    if (append && size < BUF_SIZE - 1)
?


> +                       cmp_buf[size++] = '|';
> +                       cmp_buf[size] = '\0';
>                 }
>
>                 page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
> --
> 2.32.0
>


-- 
Thanks
Yafang

Re: [PATCH 5/5] vsprintf: Make %pGp print the hex value

[Yafang Shao]

On Wed, Oct 13, 2021 at 2:34 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> All existing users of %pGp want the hex value as well as the decoded
> flag names.  This looks awkward (passing the same parameter to printf
> twice), so move that functionality into the core.  If we want, we
> can make that optional with flag arguments to %pGp in the future.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  lib/test_printf.c   | 10 +++++++++-
>  lib/vsprintf.c      |  8 ++++++++
>  mm/debug.c          |  2 +-
>  mm/memory-failure.c |  8 ++++----
>  mm/page_owner.c     |  4 ++--
>  mm/slub.c           |  4 ++--
>  6 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 662c3785aa57..a60b1a749e87 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -613,6 +613,10 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>         bool append = false;
>         int i;
>
> +       for (i = 0; i < ARRAY_SIZE(values); i++)
> +               page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
> +       snprintf(cmp_buf + size, BUF_SIZE - size, "%#lx(", page_flags);
> +       size = strlen(cmp_buf);
>         if (flags & PAGEFLAGS_MASK) {
>                 snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
>                 size = strlen(cmp_buf);
> @@ -628,7 +632,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>                         cmp_buf[size] = '\0';
>                 }
>
> -               page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
>                 snprintf(cmp_buf + size, BUF_SIZE - size, "%s=", pft[i].name);
>                 size = strlen(cmp_buf);
>                 snprintf(cmp_buf + size, BUF_SIZE - size, pft[i].fmt,
> @@ -637,6 +640,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>                 append = true;
>         }
>
> +       if (size < BUF_SIZE) {

Same with the prev patch. Should it be:
              if (size < BUF_SIZE - 1) {
?

Except that, the other parts look good to me.

> +               cmp_buf[size++] = ')';
> +               cmp_buf[size] = '\0';
> +       }
> +
>         test(cmp_buf, "%pGp", &page_flags);
>  }
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7ad44f2c8f5..214098248610 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2023,6 +2023,11 @@ char *format_page_flags(char *buf, char *end, unsigned long flags)
>         bool append = false;
>         int i;
>
> +       buf = number(buf, end, flags, default_flag_spec);
> +       if (buf < end)
> +               *buf = '(';
> +       buf++;
> +
>         /* Page flags from the main area. */
>         if (main_flags) {
>                 buf = format_flags(buf, end, main_flags, pageflag_names);
> @@ -2051,6 +2056,9 @@ char *format_page_flags(char *buf, char *end, unsigned long flags)
>
>                 append = true;
>         }
> +       if (buf < end)
> +               *buf = ')';
> +       buf++;
>
>         return buf;
>  }
> diff --git a/mm/debug.c b/mm/debug.c
> index fae0f81ad831..714be101dec9 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -162,7 +162,7 @@ static void __dump_page(struct page *page)
>  out_mapping:
>         BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>
> -       pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
> +       pr_warn("%sflags: %pGp%s\n", type, &head->flags,
>                 page_cma ? " CMA" : "");
>         print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
>                         sizeof(unsigned long), page,
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 3e6449f2102a..e4e122a2e9b1 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2109,14 +2109,14 @@ static int __soft_offline_page(struct page *page)
>                         if (!list_empty(&pagelist))
>                                 putback_movable_pages(&pagelist);
>
> -                       pr_info("soft offline: %#lx: %s migration failed %d, type %lx (%pGp)\n",
> -                               pfn, msg_page[huge], ret, page->flags, &page->flags);
> +                       pr_info("soft offline: %#lx: %s migration failed %d, type %pGp\n",
> +                               pfn, msg_page[huge], ret, &page->flags);
>                         if (ret > 0)
>                                 ret = -EBUSY;
>                 }
>         } else {
> -               pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %lx (%pGp)\n",
> -                       pfn, msg_page[huge], page_count(page), page->flags, &page->flags);
> +               pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %pGp\n",
> +                       pfn, msg_page[huge], page_count(page), &page->flags);
>                 ret = -EBUSY;
>         }
>         return ret;
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 62402d22539b..4afc713ca525 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -351,12 +351,12 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>         pageblock_mt = get_pageblock_migratetype(page);
>         page_mt  = gfp_migratetype(page_owner->gfp_mask);
>         ret += snprintf(kbuf + ret, count - ret,
> -                       "PFN %lu type %s Block %lu type %s Flags %#lx(%pGp)\n",
> +                       "PFN %lu type %s Block %lu type %s Flags %pGp\n",
>                         pfn,
>                         migratetype_names[page_mt],
>                         pfn >> pageblock_order,
>                         migratetype_names[pageblock_mt],
> -                       page->flags, &page->flags);
> +                       &page->flags);
>
>         if (ret >= count)
>                 goto err;
> diff --git a/mm/slub.c b/mm/slub.c
> index 3d2025f7163b..f7ac28646580 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -763,9 +763,9 @@ void print_tracking(struct kmem_cache *s, void *object)
>
>  static void print_page_info(struct page *page)
>  {
> -       pr_err("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=%pGp\n",
>                page, page->objects, page->inuse, page->freelist,
> -              page->flags, &page->flags);
> +              &page->flags);
>
>  }
>
> --
> 2.32.0
>


-- 
Thanks
Yafang

Re: [PATCH 2/5] test_printf: Assign all flags to page_flags

[Kirill A. Shutemov]

On Tue, Oct 12, 2021 at 07:26:44PM +0100, Matthew Wilcox (Oracle) wrote:
> Even unknown flags should be passed to %pGp so that we can test what it
> does with them (today: nothing).
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  lib/test_printf.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index a52c1c3a55ba..f744b0498672 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -608,14 +608,12 @@ 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 values[] = {section, node, zone, last_cpupid, kasan_tag};
> -	unsigned long page_flags = 0;
> +	unsigned long page_flags = flags;
>  	unsigned long size = 0;
>  	bool append = false;
>  	int i;
>  
> -	flags &= PAGEFLAGS_MASK;
> -	if (flags) {
> -		page_flags |= flags;
> +	if (flags & PAGEFLAGS_MASK) {
>  		snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
>  		size = strlen(cmp_buf);
>  #if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \

Do we even need 'page_flags'? Can't we do everything on 'flags' directly?

-- 
 Kirill A. Shutemov

Re: [PATCH 4/5] test_printf: Append '|' more efficiently

[Kirill A. Shutemov]

On Wed, Oct 13, 2021 at 01:22:10PM +0800, Yafang Shao wrote:
> On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle)
> <willy@infradead.org> wrote:
> >
> > Instead of calling snprintf(), just append '|' by hand.
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  lib/test_printf.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index 60cdf4ba991e..662c3785aa57 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/test_printf.c
> > @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
> >                 if (!pft[i].width)
> >                         continue;
> >
> > -               if (append) {
> > -                       snprintf(cmp_buf + size, BUF_SIZE - size, "|");
> > -                       size = strlen(cmp_buf);
> > +               if (append && size < BUF_SIZE) {
> 
> Should it be:
>                     if (append && size < BUF_SIZE - 1)
> ?

Yeah, looks like off-by-one to me.

-- 
 Kirill A. Shutemov

Re: [PATCH 4/5] test_printf: Append '|' more efficiently

[Rasmus Villemoes]

On 13/10/2021 07.22, Yafang Shao wrote:
> On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle)
> <willy@infradead.org> wrote:
>>
>> Instead of calling snprintf(), just append '|' by hand.
>>
>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> ---
>>  lib/test_printf.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/test_printf.c b/lib/test_printf.c
>> index 60cdf4ba991e..662c3785aa57 100644
>> --- a/lib/test_printf.c
>> +++ b/lib/test_printf.c
>> @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>>                 if (!pft[i].width)
>>                         continue;
>>
>> -               if (append) {
>> -                       snprintf(cmp_buf + size, BUF_SIZE - size, "|");
>> -                       size = strlen(cmp_buf);
>> +               if (append && size < BUF_SIZE) {
> 
> Should it be:
>                     if (append && size < BUF_SIZE - 1)
> ?
> 

Perhaps, but the whole thing screams "don't do it like this". We have
seq_buf to abstract a "buffer with known length you can do a bunch of
snprintfs to". That's what should be used. Then the test can also error
out if seq_buf_has_overflowed(), because that's a bug in the test.

Alternatively, the right pattern is "size += scnprintf(cmp_buf + size,
BUF_SIZE - size)", since that will eventually saturate size at BUF_SIZE-1.

Rasmus

Re: [PATCH 1/5] test_printf: Make pft array const

[Petr Mladek]

On Tue 2021-10-12 19:26:43, Matthew Wilcox (Oracle) wrote:
> Instead of assigning ptf[i].value, leave the values in the on-stack
> array and then we can make the array const.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 2/5] test_printf: Assign all flags to page_flags

[Petr Mladek]

On Tue 2021-10-12 19:26:44, Matthew Wilcox (Oracle) wrote:
> Even unknown flags should be passed to %pGp so that we can test what it
> does with them (today: nothing).
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Makes sense.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 2/5] test_printf: Assign all flags to page_flags

[Petr Mladek]

On Wed 2021-10-13 12:25:30, Kirill A. Shutemov wrote:
> On Tue, Oct 12, 2021 at 07:26:44PM +0100, Matthew Wilcox (Oracle) wrote:
> > Even unknown flags should be passed to %pGp so that we can test what it
> > does with them (today: nothing).
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  lib/test_printf.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index a52c1c3a55ba..f744b0498672 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/test_printf.c
> > @@ -608,14 +608,12 @@ 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 values[] = {section, node, zone, last_cpupid, kasan_tag};
> > -	unsigned long page_flags = 0;
> > +	unsigned long page_flags = flags;
> >  	unsigned long size = 0;
> >  	bool append = false;
> >  	int i;
> >  
> > -	flags &= PAGEFLAGS_MASK;
> > -	if (flags) {
> > -		page_flags |= flags;
> > +	if (flags & PAGEFLAGS_MASK) {
> >  		snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
> >  		size = strlen(cmp_buf);
> >  #if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \
> 
> Do we even need 'page_flags'? Can't we do everything on 'flags' directly?

I have to say that the meaning of "flags" and "page_flags" variable
is far from clear [*] It would be worth documentation how they are
supposed to be used.

I am going to comment more on this in the 5th patch.

Best Regards,
Petr

[*] Shame on me. I did not pay much attention to the test code
    when reviewing the original code.

Re: [PATCH 3/5] test_printf: Remove custom appending of '|'

[Petr Mladek]

On Wed 2021-10-13 13:21:15, Yafang Shao wrote:
> On Wed, Oct 13, 2021 at 2:32 AM Matthew Wilcox (Oracle)
> <willy@infradead.org> wrote:
> >
> > Instead of having an ifdef to decide whether to print a |, use the
> > 'append' functionality of the main loop to print it.
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Reviewed-by: Yafang Shao <laoar.shao@gmail.com>

Makes sense.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 4/5] test_printf: Append '|' more efficiently

[Petr Mladek]

On Wed 2021-10-13 11:59:38, Rasmus Villemoes wrote:
> On 13/10/2021 07.22, Yafang Shao wrote:
> > On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle)
> > <willy@infradead.org> wrote:
> >>
> >> Instead of calling snprintf(), just append '|' by hand.
> >>
> >> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> >> ---
> >>  lib/test_printf.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/test_printf.c b/lib/test_printf.c
> >> index 60cdf4ba991e..662c3785aa57 100644
> >> --- a/lib/test_printf.c
> >> +++ b/lib/test_printf.c
> >> @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
> >>                 if (!pft[i].width)
> >>                         continue;
> >>
> >> -               if (append) {
> >> -                       snprintf(cmp_buf + size, BUF_SIZE - size, "|");
> >> -                       size = strlen(cmp_buf);
> >> +               if (append && size < BUF_SIZE) {
> > 
> > Should it be:
> >                     if (append && size < BUF_SIZE - 1)
>
> Perhaps, but the whole thing screams "don't do it like this". We have
> seq_buf to abstract a "buffer with known length you can do a bunch of
> snprintfs to". That's what should be used. Then the test can also error
> out if seq_buf_has_overflowed(), because that's a bug in the test.

Interesting idea.

> Alternatively, the right pattern is "size += scnprintf(cmp_buf + size,
> BUF_SIZE - size)", since that will eventually saturate size at BUF_SIZE-1.

Yup, this is well known pattern so that it is much easier to make
it correctly and review.

The performance is not important here.

Best Regards,
Petr

Re: [PATCH 5/5] vsprintf: Make %pGp print the hex value

[Petr Mladek]

On Tue 2021-10-12 19:26:47, Matthew Wilcox (Oracle) wrote:
> All existing users of %pGp want the hex value as well as the decoded
> flag names.  This looks awkward (passing the same parameter to printf
> twice), so move that functionality into the core.  If we want, we
> can make that optional with flag arguments to %pGp in the future.

It makes sense. Just the selftest code is pain, see below ;-)

> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 662c3785aa57..a60b1a749e87 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -613,6 +613,10 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>  	bool append = false;
>  	int i;
>  
> +	for (i = 0; i < ARRAY_SIZE(values); i++)
> +		page_flags |= (values[i] & pft[i].mask) << pft[i].shift;

I can't get the idea behind this. IMHO, the value might be zero even
when the related flag is set.

And the %pGp code seems to always print page flags from
page_flags_fields[] when the field width is not zero.

Or do I misread the code?

Best Regards,
Petr

Re: [PATCH 1/5] test_printf: Make pft array const

[Anshuman Khandual]

On 10/12/21 11:56 PM, Matthew Wilcox (Oracle) wrote:
> Instead of assigning ptf[i].value, leave the values in the on-stack
> array and then we can make the array const.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  lib/test_printf.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 55082432f37e..a52c1c3a55ba 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -586,22 +586,21 @@ struct page_flags_test {
>  	int width;
>  	int shift;
>  	int mask;
> -	unsigned long value;
>  	const char *fmt;
>  	const char *name;
>  };
>  
> -static struct page_flags_test pft[] = {
> +static const struct page_flags_test pft[] = {
>  	{SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
> -	 0, "%d", "section"},
> +	 "%d", "section"},
>  	{NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
> -	 0, "%d", "node"},
> +	 "%d", "node"},
>  	{ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
> -	 0, "%d", "zone"},
> +	 "%d", "zone"},
>  	{LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
> -	 0, "%#x", "lastcpupid"},
> +	 "%#x", "lastcpupid"},
>  	{KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
> -	 0, "%#x", "kasantag"},
> +	 "%#x", "kasantag"},
>  };
>  
>  static void __init
> @@ -627,10 +626,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>  #endif
>  	}
>  
> -	/* Set the test value */
> -	for (i = 0; i < ARRAY_SIZE(pft); i++)
> -		pft[i].value = values[i];
> -
>  	for (i = 0; i < ARRAY_SIZE(pft); i++) {
>  		if (!pft[i].width)
>  			continue;
> @@ -640,11 +635,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>  			size = strlen(cmp_buf);
>  		}
>  
> -		page_flags |= (pft[i].value & pft[i].mask) << pft[i].shift;
> +		page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
>  		snprintf(cmp_buf + size, BUF_SIZE - size, "%s=", pft[i].name);
>  		size = strlen(cmp_buf);
>  		snprintf(cmp_buf + size, BUF_SIZE - size, pft[i].fmt,
> -			 pft[i].value & pft[i].mask);
> +			 values[i] & pft[i].mask);
>  		size = strlen(cmp_buf);
>  		append = true;
>  	}
> 

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH 2/5] test_printf: Assign all flags to page_flags

[Anshuman Khandual]

On 10/13/21 2:55 PM, Kirill A. Shutemov wrote:
> On Tue, Oct 12, 2021 at 07:26:44PM +0100, Matthew Wilcox (Oracle) wrote:
>> Even unknown flags should be passed to %pGp so that we can test what it
>> does with them (today: nothing).
>>
>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> ---
>>  lib/test_printf.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/test_printf.c b/lib/test_printf.c
>> index a52c1c3a55ba..f744b0498672 100644
>> --- a/lib/test_printf.c
>> +++ b/lib/test_printf.c
>> @@ -608,14 +608,12 @@ 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 values[] = {section, node, zone, last_cpupid, kasan_tag};
>> -	unsigned long page_flags = 0;
>> +	unsigned long page_flags = flags;
>>  	unsigned long size = 0;
>>  	bool append = false;
>>  	int i;
>>  
>> -	flags &= PAGEFLAGS_MASK;
>> -	if (flags) {
>> -		page_flags |= flags;
>> +	if (flags & PAGEFLAGS_MASK) {
>>  		snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
>>  		size = strlen(cmp_buf);
>>  #if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \
> 
> Do we even need 'page_flags'? Can't we do everything on 'flags' directly?

Agreed, +1. Otherwise the change LGTM.

Re: [PATCH 3/5] test_printf: Remove custom appending of '|'

[Anshuman Khandual]

On 10/12/21 11:56 PM, Matthew Wilcox (Oracle) wrote:
> Instead of having an ifdef to decide whether to print a |, use the
> 'append' functionality of the main loop to print it.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  lib/test_printf.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index f744b0498672..60cdf4ba991e 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -616,12 +616,7 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>  	if (flags & PAGEFLAGS_MASK) {
>  		snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
>  		size = strlen(cmp_buf);
> -#if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \
> -	LAST_CPUPID_WIDTH || KASAN_TAG_WIDTH
> -		/* Other information also included in page flags */
> -		snprintf(cmp_buf + size, BUF_SIZE - size, "|");
> -		size = strlen(cmp_buf);
> -#endif
> +		append = true;
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(pft); i++) {
> 

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH 4/5] test_printf: Append '|' more efficiently

[Anshuman Khandual]

On 10/13/21 2:57 PM, Kirill A. Shutemov wrote:
> On Wed, Oct 13, 2021 at 01:22:10PM +0800, Yafang Shao wrote:
>> On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle)
>> <willy@infradead.org> wrote:
>>>
>>> Instead of calling snprintf(), just append '|' by hand.
>>>
>>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>>> ---
>>>  lib/test_printf.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/test_printf.c b/lib/test_printf.c
>>> index 60cdf4ba991e..662c3785aa57 100644
>>> --- a/lib/test_printf.c
>>> +++ b/lib/test_printf.c
>>> @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>>>                 if (!pft[i].width)
>>>                         continue;
>>>
>>> -               if (append) {
>>> -                       snprintf(cmp_buf + size, BUF_SIZE - size, "|");
>>> -                       size = strlen(cmp_buf);
>>> +               if (append && size < BUF_SIZE) {
>>
>> Should it be:
>>                     if (append && size < BUF_SIZE - 1)
>> ?
> 
> Yeah, looks like off-by-one to me.

Agreed.

Re: [PATCH 5/5] vsprintf: Make %pGp print the hex value

[Anshuman Khandual]

On 10/12/21 11:56 PM, Matthew Wilcox (Oracle) wrote:
> All existing users of %pGp want the hex value as well as the decoded
> flag names.  This looks awkward (passing the same parameter to printf

Agreed, it is awkward. Always thought about this, passing the exact same
parameter twice for the print functions.

> twice), so move that functionality into the core.  If we want, we
> can make that optional with flag arguments to %pGp in the future.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  lib/test_printf.c   | 10 +++++++++-
>  lib/vsprintf.c      |  8 ++++++++
>  mm/debug.c          |  2 +-
>  mm/memory-failure.c |  8 ++++----
>  mm/page_owner.c     |  4 ++--
>  mm/slub.c           |  4 ++--
>  6 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 662c3785aa57..a60b1a749e87 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -613,6 +613,10 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>  	bool append = false;
>  	int i;
>  
> +	for (i = 0; i < ARRAY_SIZE(values); i++)
> +		page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
> +	snprintf(cmp_buf + size, BUF_SIZE - size, "%#lx(", page_flags);
> +	size = strlen(cmp_buf);
>  	if (flags & PAGEFLAGS_MASK) {
>  		snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name);
>  		size = strlen(cmp_buf);
> @@ -628,7 +632,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>  			cmp_buf[size] = '\0';
>  		}
>  
> -		page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
>  		snprintf(cmp_buf + size, BUF_SIZE - size, "%s=", pft[i].name);
>  		size = strlen(cmp_buf);
>  		snprintf(cmp_buf + size, BUF_SIZE - size, pft[i].fmt,
> @@ -637,6 +640,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
>  		append = true;
>  	}
>  
> +	if (size < BUF_SIZE) {

Should be BUF_SIZE - 1 instead ? But seems like it is still inconclusive
whether writing these directly into the buffer, instead of snprintf() is
better or not.

> +		cmp_buf[size++] = ')';
> +		cmp_buf[size] = '\0';
> +	}
> +
>  	test(cmp_buf, "%pGp", &page_flags);
>  }
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7ad44f2c8f5..214098248610 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2023,6 +2023,11 @@ char *format_page_flags(char *buf, char *end, unsigned long flags)
>  	bool append = false;
>  	int i;
>  
> +	buf = number(buf, end, flags, default_flag_spec);
> +	if (buf < end)
> +		*buf = '(';
> +	buf++;
> +
>  	/* Page flags from the main area. */
>  	if (main_flags) {
>  		buf = format_flags(buf, end, main_flags, pageflag_names);
> @@ -2051,6 +2056,9 @@ char *format_page_flags(char *buf, char *end, unsigned long flags)
>  
>  		append = true;
>  	}
> +	if (buf < end)
> +		*buf = ')';
> +	buf++;
>  
>  	return buf;
>  }
> diff --git a/mm/debug.c b/mm/debug.c
> index fae0f81ad831..714be101dec9 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -162,7 +162,7 @@ static void __dump_page(struct page *page)
>  out_mapping:
>  	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>  
> -	pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
> +	pr_warn("%sflags: %pGp%s\n", type, &head->flags,
>  		page_cma ? " CMA" : "");
>  	print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
>  			sizeof(unsigned long), page,
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 3e6449f2102a..e4e122a2e9b1 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2109,14 +2109,14 @@ static int __soft_offline_page(struct page *page)
>  			if (!list_empty(&pagelist))
>  				putback_movable_pages(&pagelist);
>  
> -			pr_info("soft offline: %#lx: %s migration failed %d, type %lx (%pGp)\n",
> -				pfn, msg_page[huge], ret, page->flags, &page->flags);
> +			pr_info("soft offline: %#lx: %s migration failed %d, type %pGp\n",
> +				pfn, msg_page[huge], ret, &page->flags);
>  			if (ret > 0)
>  				ret = -EBUSY;
>  		}
>  	} else {
> -		pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %lx (%pGp)\n",
> -			pfn, msg_page[huge], page_count(page), page->flags, &page->flags);
> +		pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %pGp\n",
> +			pfn, msg_page[huge], page_count(page), &page->flags);
>  		ret = -EBUSY;
>  	}
>  	return ret;
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 62402d22539b..4afc713ca525 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -351,12 +351,12 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>  	pageblock_mt = get_pageblock_migratetype(page);
>  	page_mt  = gfp_migratetype(page_owner->gfp_mask);
>  	ret += snprintf(kbuf + ret, count - ret,
> -			"PFN %lu type %s Block %lu type %s Flags %#lx(%pGp)\n",
> +			"PFN %lu type %s Block %lu type %s Flags %pGp\n",
>  			pfn,
>  			migratetype_names[page_mt],
>  			pfn >> pageblock_order,
>  			migratetype_names[pageblock_mt],
> -			page->flags, &page->flags);
> +			&page->flags);
>  
>  	if (ret >= count)
>  		goto err;
> diff --git a/mm/slub.c b/mm/slub.c
> index 3d2025f7163b..f7ac28646580 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -763,9 +763,9 @@ void print_tracking(struct kmem_cache *s, void *object)
>  
>  static void print_page_info(struct page *page)
>  {
> -	pr_err("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=%pGp\n",
>  	       page, page->objects, page->inuse, page->freelist,
> -	       page->flags, &page->flags);
> +	       &page->flags);
>  
>  }

Otherwise this change LGTM.

Re: [PATCH 5/5] vsprintf: Make %pGp print the hex value

[Petr Mladek]

On Fri 2021-10-15 12:55:48, Petr Mladek wrote:
> On Tue 2021-10-12 19:26:47, Matthew Wilcox (Oracle) wrote:
> > All existing users of %pGp want the hex value as well as the decoded
> > flag names.  This looks awkward (passing the same parameter to printf
> > twice), so move that functionality into the core.  If we want, we
> > can make that optional with flag arguments to %pGp in the future.
> 
> It makes sense. Just the selftest code is pain, see below ;-)
> 
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index 662c3785aa57..a60b1a749e87 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/test_printf.c
> > @@ -613,6 +613,10 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
> >  	bool append = false;
> >  	int i;
> >  
> > +	for (i = 0; i < ARRAY_SIZE(values); i++)
> > +		page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
> 
> I can't get the idea behind this. IMHO, the value might be zero even
> when the related flag is set.
> 
> And the %pGp code seems to always print page flags from
> page_flags_fields[] when the field width is not zero.
> 
> Or do I misread the code?

Just for record. I really did misread the code. It constructs flags
from the given values. It does reverse operation to
format_page_flags().

Best Regards,
Petr