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

[Anshuman Khandual <anshuman.khandual@arm.com>]

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.