linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: Yafang Shao <laoar.shao@gmail.com>
Cc: willy@infradead.org, andriy.shevchenko@linux.intel.com,
	david@redhat.com, linmiaohe@huawei.com, vbabka@suse.cz,
	cl@linux.com, penberg@kernel.org, rientjes@google.com,
	iamjoonsoo.kim@lge.com, akpm@linux-foundation.org,
	rostedt@goodmis.org, sergey.senozhatsky@gmail.com,
	joe@perches.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp
Date: Tue, 9 Feb 2021 14:53:53 +0100	[thread overview]
Message-ID: <YCKT8WCPGU+HBY91@alley> (raw)
In-Reply-To: <20210209105613.42747-4-laoar.shao@gmail.com>

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



  parent reply	other threads:[~2021-02-09 13:53 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-09 10:56 [PATCH v4 0/3] mm, vsprintf: dump full information of page flags in pGp Yafang Shao
2021-02-09 10:56 ` [PATCH v4 1/3] mm, slub: use pGp to print page flags Yafang Shao
2021-02-09 10:57   ` David Hildenbrand
2021-02-09 10:56 ` [PATCH v4 2/3] mm, slub: don't combine pr_err with INFO Yafang Shao
2021-02-09 10:57   ` David Hildenbrand
2021-02-09 10:56 ` [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp Yafang Shao
2021-02-09 11:00   ` David Hildenbrand
2021-02-09 12:00   ` Vlastimil Babka
2021-02-09 13:53   ` Petr Mladek [this message]
2021-02-09 14:16     ` Andy Shevchenko
2021-02-09 14:57       ` Petr Mladek
2021-02-09 16:21     ` Yafang Shao
2021-02-10 12:51       ` Petr Mladek
2021-02-10 13:04         ` Joe Perches
2021-02-10 15:39         ` Yafang Shao
2021-02-09 12:41 ` [PATCH v4 0/3] mm, " Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YCKT8WCPGU+HBY91@alley \
    --to=pmladek@suse.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=cl@linux.com \
    --cc=david@redhat.com \
    --cc=iamjoonsoo.kim@lge.com \
    --cc=joe@perches.com \
    --cc=laoar.shao@gmail.com \
    --cc=linmiaohe@huawei.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=penberg@kernel.org \
    --cc=rientjes@google.com \
    --cc=rostedt@goodmis.org \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=vbabka@suse.cz \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).