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@xxxxxxxxxxxxx>
> ---
> 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.
