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.
This patchset also includes some code cleanup in mm/slub.c.
Below is the example of the output in mm/slub.c.
- Before the patchset
[ 6155.716018] INFO: Slab 0x000000004027dd4f objects=33 used=3 fp=0x000000008cd1579c flags=0x17ffffc0010200
- After the patchset
[ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
The documentation and test cases of pGp are also updated.
Below is the result of the test_printf,
[ 5091.307308] test_printf: loaded.
[ 5091.308285] test_printf: all 388 tests passed
[ 5091.309105] test_printf: unloaded.
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: dump full information of page flags in pGp
Documentation/core-api/printk-formats.rst | 2 +-
lib/test_printf.c | 65 ++++++++++++++++++-----
lib/vsprintf.c | 58 +++++++++++++++++++-
mm/slub.c | 13 ++---
4 files changed, 116 insertions(+), 22 deletions(-)
--
2.17.1
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 <[email protected]>
Reviewed-by: Vlastimil Babka <[email protected]>
Acked-by: David Rientjes <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
---
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
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.
- Before the patch,
[ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
- After the patch,
[ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
The Documentation and test cases are also updated.
Cc: David Hildenbrand <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Miaohe Lin <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
---
Documentation/core-api/printk-formats.rst | 2 +-
lib/test_printf.c | 65 ++++++++++++++++++-----
lib/vsprintf.c | 58 +++++++++++++++++++-
3 files changed, 109 insertions(+), 16 deletions(-)
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 6d26c5c6ac48..1374cdd04f0f 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -538,7 +538,7 @@ Flags bitfields such as page flags, gfp_flags
::
- %pGp referenced|uptodate|lru|active|private
+ %pGp Node 0,Zone 2,referenced|uptodate|lru|active|private
%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..4c5e064cbe2e 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -569,6 +569,48 @@ netdev_features(void)
{
}
+static void __init
+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 page_flags = 0;
+ unsigned long size = 0;
+
+#ifdef SECTION_IN_PAGE_FLAGS
+ page_flags |= (sec & SECTIONS_MASK) << SECTIONS_PGSHIFT;
+ snprintf(cmp_buf, BUF_SIZE, "Section %#x,", sec);
+ size = strlen(cmp_buf);
+#endif
+
+ page_flags |= (node & NODES_MASK) << NODES_PGSHIFT;
+ snprintf(cmp_buf + size, BUF_SIZE - size, "Node %d", node);
+ size = strlen(cmp_buf);
+
+ page_flags |= (zone & ZONES_MASK) << ZONES_PGSHIFT;
+ snprintf(cmp_buf + size, BUF_SIZE - size, ",Zone %d", zone);
+ size = strlen(cmp_buf);
+
+#ifndef LAST_CPUPID_NOT_IN_PAGE_FLAGS
+ page_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)
+ page_flags |= (tag & KASAN_TAG_MASK) << KASAN_TAG_PGSHIFT;
+ snprintf(cmp_buf + size, BUF_SIZE - size, ",Kasantag %#x", tag);
+ size = strlen(cmp_buf);
+#endif
+
+ test(cmp_buf, "%pGp", &page_flags);
+
+ if (flags) {
+ page_flags |= flags;
+ snprintf(cmp_buf + size, BUF_SIZE - size, ",%s", name);
+ test(cmp_buf, "%pGp", &page_flags);
+ }
+}
+
static void __init
flags(void)
{
@@ -576,17 +618,16 @@ flags(void)
gfp_t gfp;
char *cmp_buffer;
- flags = 0;
- test("", "%pGp", &flags);
-
- /* Page flags should filter the zone id */
- flags = 1UL << NR_PAGEFLAGS;
- test("", "%pGp", &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);
+ cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
+ if (!cmp_buffer)
+ return;
+ page_flags_test(0, 0, 0, 0, 0, 0, NULL, cmp_buffer);
+ page_flags_test(1, 1, 1, 0x1ffff, 1,
+ (1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
+ | 1UL << PG_active | 1UL << PG_swapbacked),
+ "uptodate|dirty|lru|active|swapbacked",
+ cmp_buffer);
flags = VM_READ | VM_EXEC | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC
| VM_DENYWRITE;
@@ -601,10 +642,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..5c2b02ad60f1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1916,6 +1916,62 @@ 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 page_flags)
+{
+ unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1);
+ int size = ARRAY_SIZE(pfl);
+ bool separator = false;
+ int i;
+
+ for (i = 0; i < size; i++) {
+ if (pfl[i].width == 0)
+ continue;
+
+ if (separator) {
+ if (buf < end)
+ *buf = ',';
+ buf++;
+ }
+
+
+ buf = string(buf, end, pfl[i].name, *pfl[i].spec);
+
+ buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask,
+ *pfl[i].spec);
+ separator = true;
+ }
+
+ if (flags) {
+ 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,7 +1985,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
switch (fmt[1]) {
case 'p':
flags = *(unsigned long *)flags_ptr;
- /* Remove zone id */
+ buf = format_page_flags(buf, end, flags);
flags &= (1UL << NR_PAGEFLAGS) - 1;
names = pageflag_names;
break;
--
2.17.1
On Mon, 2021-02-01 at 19:56 +0800, 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.
>
> - Before the patch,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
>
> - After the patch,
> [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
While debugfs is not an ABI, this format is exported in debugfs to
userspace via mm/page_owner.c read_page_owner/print_page_owner.
Does changing the output format matter to anyone?
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> +static
> +char *format_page_flags(char *buf, char *end, unsigned long page_flags)
> +{
> + unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1);
> + int size = ARRAY_SIZE(pfl);
There's no real value in used-once variables.
> + bool separator = false;
> + int i;
> +
> + for (i = 0; i < size; i++) {
Use ARRAY_SIZE here instead
for (i = 0; i < ARRAY_SIZE(pfl); i++) {
> + if (pfl[i].width == 0)
> + continue;
> +
> + if (separator) {
> + if (buf < end)
> + *buf = ',';
> + buf++;
> + }
> +
> +
> + buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> +
> + buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask,
> + *pfl[i].spec);
> + separator = true;
> + }
Style question:
Might this array be more intelligible with pointers instead of indexes?
Something like:
struct page_flags_layout *p;
for (p = pfl; p < pfl + ARRAY_SIZE(pfl); p++) {
if (p->width == 0)
continue;
if (p > pfl) {
if (buf < end)
*buf = ',';
buf++;
}
buf = string(buf, end, p->name, *p->spec);
buf = number(buf, end, (page_flags >> p->shift) & p->mask, *p->spec);
}
> +
> + if (flags) {
Maybe:
if (page_flags & (BIT(NR_PAGEFLAGS) - 1)) {
> + if (buf < end)
> + *buf = ',';
> + buf++;
> + }
> +
> + return buf;
> +}
> +
On 01.02.21 12:56, 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.
>
> - Before the patch,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
>
> - After the patch,
> [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
For debugging purposes, it might be helpful to have the actual zone name
(and to know if the value is sane). You could obtain it (without other
modifications) via
const char zname = "Invalid";
if (zone < MAX_NR_ZONES)
zname = first_online_pgdat()->node_zones[zone].name;
Similarly, it might also be helpful to indicate if a node is
online/offline/invalid/.
const char nstate = "Invalid";
if (node_online(nid))
nstate = "Online";
else if (node_possible(nid))
nstate = "Offline";
Printing that in addition to the raw value could be helpful. Just some
thoughts.
--
Thanks,
David / dhildenb
On Mon, Feb 1, 2021 at 9:05 PM Joe Perches <[email protected]> wrote:
>
> On Mon, 2021-02-01 at 19:56 +0800, 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.
> >
> > - Before the patch,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
>
> While debugfs is not an ABI, this format is exported in debugfs to
> userspace via mm/page_owner.c read_page_owner/print_page_owner.
>
Right, the page_owner will be affected by this change.
> Does changing the output format matter to anyone?
>
The user tools which parse the page_owner may be affected.
If we don't want to affect the userspace tools, I think we can make a
little change in page_owner as follows,
unsigned long masked_flags = page->flags & (BIT(NR_PAGEFLAGS) - 1);
snprintf("..., %#lx(%pGp)\n", page->flags, &masked_flags);
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > +static
> > +char *format_page_flags(char *buf, char *end, unsigned long page_flags)
> > +{
> > + unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1);
> > + int size = ARRAY_SIZE(pfl);
>
> There's no real value in used-once variables.
>
> > + bool separator = false;
> > + int i;
> > +
> > + for (i = 0; i < size; i++) {
>
> Use ARRAY_SIZE here instead
>
Sure
> for (i = 0; i < ARRAY_SIZE(pfl); i++) {
>
> > + if (pfl[i].width == 0)
> > + continue;
> > +
> > + if (separator) {
> > + if (buf < end)
> > + *buf = ',';
> > + buf++;
> > + }
> > +
> > +
> > + buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > +
> > + buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask,
> > + *pfl[i].spec);
> > + separator = true;
> > + }
>
> Style question:
> Might this array be more intelligible with pointers instead of indexes?
Good suggestion!
I will change it in the next version.
> Something like:
>
> struct page_flags_layout *p;
>
> for (p = pfl; p < pfl + ARRAY_SIZE(pfl); p++) {
> if (p->width == 0)
> continue;
>
> if (p > pfl) {
> if (buf < end)
> *buf = ',';
> buf++;
> }
It doesn't work, because there's a 'continue' above, meaning that the
p may be greater than pfl without doing anything.
>
> buf = string(buf, end, p->name, *p->spec);
> buf = number(buf, end, (page_flags >> p->shift) & p->mask, *p->spec);
> }
>
> > +
> > + if (flags) {
>
> Maybe:
>
> if (page_flags & (BIT(NR_PAGEFLAGS) - 1)) {
>
Sure.
> > + if (buf < end)
> > + *buf = ',';
> > + buf++;
> > + }
> > +
> > + return buf;
> > +}
> > +
>
>
--
Thanks
Yafang
On Mon, Feb 01, 2021 at 07:56:10PM +0800, 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.
>
> - Before the patch,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
>
> - After the patch,
> [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
>
> The Documentation and test cases are also updated.
Thanks for an update, my comments below.
...
> - %pGp referenced|uptodate|lru|active|private
> + %pGp Node 0,Zone 2,referenced|uptodate|lru|active|private
Since of the nature of printf() buffer, I wonder if these should be at the end.
I.o.w. the question is is the added material more important to user to see than
the existed one?
...
> +static void __init
> +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 page_flags = 0;
> + unsigned long size = 0;
> +
> +#ifdef SECTION_IN_PAGE_FLAGS
> + page_flags |= (sec & SECTIONS_MASK) << SECTIONS_PGSHIFT;
> + snprintf(cmp_buf, BUF_SIZE, "Section %#x,", sec);
I would keep it in the same form as latter ones, i.e.
snprintf(cmp_buf + size, BUF_SIZE - size, "Section %#x,", sec);
In this case it will be easier if at some point we might need to reshuffle.
> + size = strlen(cmp_buf);
> +#endif
> +
> + page_flags |= (node & NODES_MASK) << NODES_PGSHIFT;
> + snprintf(cmp_buf + size, BUF_SIZE - size, "Node %d", node);
> + size = strlen(cmp_buf);
> +
> + page_flags |= (zone & ZONES_MASK) << ZONES_PGSHIFT;
> + snprintf(cmp_buf + size, BUF_SIZE - size, ",Zone %d", zone);
> + size = strlen(cmp_buf);
> +
> +#ifndef LAST_CPUPID_NOT_IN_PAGE_FLAGS
> + page_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)
> + page_flags |= (tag & KASAN_TAG_MASK) << KASAN_TAG_PGSHIFT;
> + snprintf(cmp_buf + size, BUF_SIZE - size, ",Kasantag %#x", tag);
> + size = strlen(cmp_buf);
> +#endif
> +
> + test(cmp_buf, "%pGp", &page_flags);
> +
> + if (flags) {
If below will be always for flags set, I would rewrite this condition as
if (!flags)
return;
but it's up to you.
> + page_flags |= flags;
> + snprintf(cmp_buf + size, BUF_SIZE - size, ",%s", name);
> + test(cmp_buf, "%pGp", &page_flags);
> + }
> +}
...
> - flags = 0;
> - flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
> - | 1UL << PG_active | 1UL << PG_swapbacked;
I would leave this untouched and reuse below...
> + cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
> + if (!cmp_buffer)
> + return;
...as
> + page_flags_test(0, 0, 0, 0, 0, 0, NULL, cmp_buffer);
flags = 0;
page_flags_test(0, 0, 0, 0, 0, flags, NULL, cmp_buffer);
> + page_flags_test(1, 1, 1, 0x1ffff, 1,
> + (1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
> + | 1UL << PG_active | 1UL << PG_swapbacked),
> + "uptodate|dirty|lru|active|swapbacked",
> + cmp_buffer);
flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
| 1UL << PG_active | 1UL << PG_swapbacked;
page_flags_test(1, 1, 1, 0x1ffff, 1, flags,
"uptodate|dirty|lru|active|swapbacked",
cmp_buffer);
...
> +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 "},
> +};
Please add trailing space only once where it's needed (below in the code).
...
> +static
> +char *format_page_flags(char *buf, char *end, unsigned long page_flags)
> +{
> + unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1);
> + int size = ARRAY_SIZE(pfl);
> + bool separator = false;
> + int i;
> +
> + for (i = 0; i < size; i++) {
> + if (pfl[i].width == 0)
> + continue;
> +
> + if (separator) {
> + if (buf < end)
> + *buf = ',';
> + buf++;
> + }
> +
> +
One blank line is enough.
> + buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> +
> + buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask,
> + *pfl[i].spec);
> + separator = true;
> + }
> +
> + if (flags) {
> + if (buf < end)
> + *buf = ',';
> + buf++;
> + }
I think you may optimize above to avoid using the separator variable.
DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
unsigned long flags;
unsigned long last;
for (i = 0; i < ARRAY_SIZE(pfl); i++)
__assign_bit(mask, pfl[i].width);
last = find_last_bit(mask, ARRAY_SIZE(pfl));
for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
flags = (page_flags >> pfl[i].shift) & pfl[i].mask;
/* Format: Flag Name + ' ' (space) + Number + ',' (separator) */
buf = string(buf, end, pfl[i].name, *pfl[i].spec);
if (buf < end)
*buf = ' ';
buf++;
buf = number(buf, end, flags, *pfl[i].spec);
/* No separator for the last entry */
if ((page_flags & (BIT(NR_PAGEFLAGS) - 1)) || (i != last)) {
if (buf < end)
*buf = ',';
buf++;
}
}
> + return buf;
> +}
--
With Best Regards,
Andy Shevchenko
On 01.02.21 14:23, David Hildenbrand wrote:
> On 01.02.21 12:56, 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.
>>
>> - Before the patch,
>> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
>>
>> - After the patch,
>> [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
>
> For debugging purposes, it might be helpful to have the actual zone name
> (and to know if the value is sane). You could obtain it (without other
> modifications) via
>
> const char zname = "Invalid";
>
> if (zone < MAX_NR_ZONES)
> zname = first_online_pgdat()->node_zones[zone].name;
>
>
> Similarly, it might also be helpful to indicate if a node is
> online/offline/invalid/.
>
> const char nstate = "Invalid";
>
> if (node_online(nid))
> nstate = "Online";
> else if (node_possible(nid))
> nstate = "Offline";
Just remembering that we have to take care of nid limits:
if (nid >= 0 && nid < MAX_NUMNODES) {
if (node_online(nid))
nstate = "Online";
else if (node_possible(nid))
nstate = "Offline";
}
--
Thanks,
David / dhildenb
On Mon, Feb 01, 2021 at 02:23:33PM +0100, David Hildenbrand wrote:
> On 01.02.21 12:56, 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.
> >
> > - Before the patch,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
>
> For debugging purposes, it might be helpful to have the actual zone name
> (and to know if the value is sane). You could obtain it (without other
> modifications) via
>
> const char zname = "Invalid";
>
> if (zone < MAX_NR_ZONES)
> zname = first_online_pgdat()->node_zones[zone].name;
>
>
> Similarly, it might also be helpful to indicate if a node is
> online/offline/invalid/.
>
> const char nstate = "Invalid";
>
> if (node_online(nid))
> nstate = "Online";
> else if (node_possible(nid))
> nstate = "Offline";
>
>
> Printing that in addition to the raw value could be helpful. Just some
> thoughts.
printf() buffer is not a black hole, esp. when you get it messed with critical
messages (Oops). I suggest to reduce a burden as much as possible. If you wish
to get this, make it caller-configurable, i.e. adding another letter to the
specifier.
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Feb 01, 2021 at 07:56:10PM +0800, 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.
> >
> > - Before the patch,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> >
> > The Documentation and test cases are also updated.
>
> Thanks for an update, my comments below.
>
> ...
>
> > - %pGp referenced|uptodate|lru|active|private
> > + %pGp Node 0,Zone 2,referenced|uptodate|lru|active|private
>
> Since of the nature of printf() buffer, I wonder if these should be at the end.
> I.o.w. the question is is the added material more important to user to see than
> the existed one?
>
The existing one should be more important than the added one.
But the order of output will not match with the value for page->flags.
E.g.
flags=0x17ffffc0010200(slab|head,Node 0,Zone 2,Lastcpupid 0x1fffff)
It may be strange to compare the value with the string.
> ...
>
> > +static void __init
> > +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 page_flags = 0;
> > + unsigned long size = 0;
> > +
> > +#ifdef SECTION_IN_PAGE_FLAGS
> > + page_flags |= (sec & SECTIONS_MASK) << SECTIONS_PGSHIFT;
> > + snprintf(cmp_buf, BUF_SIZE, "Section %#x,", sec);
>
> I would keep it in the same form as latter ones, i.e.
>
> snprintf(cmp_buf + size, BUF_SIZE - size, "Section %#x,", sec);
>
> In this case it will be easier if at some point we might need to reshuffle.
>
Good suggestion.
> > + size = strlen(cmp_buf);
> > +#endif
> > +
> > + page_flags |= (node & NODES_MASK) << NODES_PGSHIFT;
> > + snprintf(cmp_buf + size, BUF_SIZE - size, "Node %d", node);
> > + size = strlen(cmp_buf);
> > +
> > + page_flags |= (zone & ZONES_MASK) << ZONES_PGSHIFT;
> > + snprintf(cmp_buf + size, BUF_SIZE - size, ",Zone %d", zone);
> > + size = strlen(cmp_buf);
> > +
> > +#ifndef LAST_CPUPID_NOT_IN_PAGE_FLAGS
> > + page_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)
> > + page_flags |= (tag & KASAN_TAG_MASK) << KASAN_TAG_PGSHIFT;
> > + snprintf(cmp_buf + size, BUF_SIZE - size, ",Kasantag %#x", tag);
> > + size = strlen(cmp_buf);
> > +#endif
> > +
> > + test(cmp_buf, "%pGp", &page_flags);
> > +
> > + if (flags) {
>
> If below will be always for flags set, I would rewrite this condition as
>
> if (!flags)
> return;
>
> but it's up to you.
>
> > + page_flags |= flags;
> > + snprintf(cmp_buf + size, BUF_SIZE - size, ",%s", name);
> > + test(cmp_buf, "%pGp", &page_flags);
> > + }
> > +}
>
> ...
>
> > - flags = 0;
>
> > - flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
> > - | 1UL << PG_active | 1UL << PG_swapbacked;
>
> I would leave this untouched and reuse below...
>
> > + cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
> > + if (!cmp_buffer)
> > + return;
>
> ...as
>
> > + page_flags_test(0, 0, 0, 0, 0, 0, NULL, cmp_buffer);
>
> flags = 0;
> page_flags_test(0, 0, 0, 0, 0, flags, NULL, cmp_buffer);
>
> > + page_flags_test(1, 1, 1, 0x1ffff, 1,
> > + (1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
> > + | 1UL << PG_active | 1UL << PG_swapbacked),
> > + "uptodate|dirty|lru|active|swapbacked",
> > + cmp_buffer);
>
> flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru
> | 1UL << PG_active | 1UL << PG_swapbacked;
> page_flags_test(1, 1, 1, 0x1ffff, 1, flags,
> "uptodate|dirty|lru|active|swapbacked",
> cmp_buffer);
>
Sure, I will change them.
> ...
>
> > +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 "},
> > +};
>
> Please add trailing space only once where it's needed (below in the code).
>
> ...
>
> > +static
> > +char *format_page_flags(char *buf, char *end, unsigned long page_flags)
> > +{
> > + unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1);
> > + int size = ARRAY_SIZE(pfl);
> > + bool separator = false;
> > + int i;
> > +
> > + for (i = 0; i < size; i++) {
> > + if (pfl[i].width == 0)
> > + continue;
> > +
> > + if (separator) {
> > + if (buf < end)
> > + *buf = ',';
> > + buf++;
> > + }
>
> > +
> > +
>
> One blank line is enough.
>
Thanks for pointing it out.
BTW, it seems we need to improve the check_patch to catch this kind of
issue automatically.
> > + buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > +
> > + buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask,
> > + *pfl[i].spec);
> > + separator = true;
> > + }
> > +
> > + if (flags) {
> > + if (buf < end)
> > + *buf = ',';
> > + buf++;
> > + }
>
> I think you may optimize above to avoid using the separator variable.
>
> DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> unsigned long flags;
> unsigned long last;
>
> for (i = 0; i < ARRAY_SIZE(pfl); i++)
> __assign_bit(mask, pfl[i].width);
>
> last = find_last_bit(mask, ARRAY_SIZE(pfl));
>
> for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> flags = (page_flags >> pfl[i].shift) & pfl[i].mask;
>
> /* Format: Flag Name + ' ' (space) + Number + ',' (separator) */
> buf = string(buf, end, pfl[i].name, *pfl[i].spec);
>
> if (buf < end)
> *buf = ' ';
> buf++;
>
> buf = number(buf, end, flags, *pfl[i].spec);
>
> /* No separator for the last entry */
> if ((page_flags & (BIT(NR_PAGEFLAGS) - 1)) || (i != last)) {
> if (buf < end)
> *buf = ',';
> buf++;
> }
> }
>
Good suggestion. I will think about it.
--
Thanks
Yafang
On Mon, Feb 1, 2021 at 9:34 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Feb 01, 2021 at 02:23:33PM +0100, David Hildenbrand wrote:
> > On 01.02.21 12:56, 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.
> > >
> > > - Before the patch,
> > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > >
> > > - After the patch,
> > > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> >
> > For debugging purposes, it might be helpful to have the actual zone name
> > (and to know if the value is sane). You could obtain it (without other
> > modifications) via
> >
> > const char zname = "Invalid";
> >
> > if (zone < MAX_NR_ZONES)
> > zname = first_online_pgdat()->node_zones[zone].name;
> >
> >
> > Similarly, it might also be helpful to indicate if a node is
> > online/offline/invalid/.
> >
> > const char nstate = "Invalid";
> >
> > if (node_online(nid))
> > nstate = "Online";
> > else if (node_possible(nid))
> > nstate = "Offline";
> >
> >
> > Printing that in addition to the raw value could be helpful. Just some
> > thoughts.
>
> printf() buffer is not a black hole, esp. when you get it messed with critical
> messages (Oops). I suggest to reduce a burden as much as possible. If you wish
> to get this, make it caller-configurable, i.e. adding another letter to the
> specifier.
>
I think David's suggestion will help us to identify issues.
You mean that we should use another one like "%pGpd" - means pGp debug
- to implement it ?
--
Thanks
Yafang
On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
> - Before the patch,
> [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
>
> - After the patch,
> [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
I would suggest it will be easier to parse as:
flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
That should alleviate the concerns about debugfs format change -- we've
never guaranteed that flag names won't change, and they now look enough
like flags that parsers shouldn't fall over them.
On Mon, Feb 1, 2021 at 10:15 PM Matthew Wilcox <[email protected]> wrote:
>
> On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
> > - Before the patch,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
>
> I would suggest it will be easier to parse as:
>
> flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
>
> That should alleviate the concerns about debugfs format change -- we've
> never guaranteed that flag names won't change, and they now look enough
> like flags that parsers shouldn't fall over them.
Good suggestion!
I will do it as you suggested in the next version.
--
Thanks
Yafang
On Mon, Feb 01, 2021 at 09:52:53PM +0800, Yafang Shao wrote:
> On Mon, Feb 1, 2021 at 9:34 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Feb 01, 2021 at 02:23:33PM +0100, David Hildenbrand wrote:
> > > On 01.02.21 12:56, Yafang Shao wrote:
...
> > > Printing that in addition to the raw value could be helpful. Just some
> > > thoughts.
> >
> > printf() buffer is not a black hole, esp. when you get it messed with critical
> > messages (Oops). I suggest to reduce a burden as much as possible. If you wish
> > to get this, make it caller-configurable, i.e. adding another letter to the
> > specifier.
> >
>
> I think David's suggestion will help us to identify issues.
>
> You mean that we should use another one like "%pGpd" - means pGp debug
> - to implement it ?
Yes.
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 01, 2021 at 09:49:59PM +0800, Yafang Shao wrote:
> On Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
...
> > > - Before the patch,
> > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > >
> > > - After the patch,
> > > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> > >
> > > The Documentation and test cases are also updated.
> >
> > Thanks for an update, my comments below.
> >
> > ...
> >
> > > - %pGp referenced|uptodate|lru|active|private
> > > + %pGp Node 0,Zone 2,referenced|uptodate|lru|active|private
> >
> > Since of the nature of printf() buffer, I wonder if these should be at the end.
> > I.o.w. the question is is the added material more important to user to see than
> > the existed one?
> >
>
> The existing one should be more important than the added one.
> But the order of output will not match with the value for page->flags.
> E.g.
> flags=0x17ffffc0010200(slab|head,Node 0,Zone 2,Lastcpupid 0x1fffff)
> It may be strange to compare the value with the string.
More I'm looking at it, more I'm thinking it should have different specifiers
for each group of desired flags to be printed.
So, you leave %pGp as is and then add another letter to add more details, so
user will choose what and in which order they want.
For example, let's assume %pGp == %pGpf and P is a new specifier for what you
are initially adding here:
%pGpfP => referenced|uptodate|lru|active|private,Node 0,Zone 2
%pGpPf => Node 0,Zone 2,referenced|uptodate|lru|active|private
and so on.
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 01, 2021 at 06:16:42PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 01, 2021 at 09:49:59PM +0800, Yafang Shao wrote:
> > On Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
...
> > The existing one should be more important than the added one.
> > But the order of output will not match with the value for page->flags.
> > E.g.
> > flags=0x17ffffc0010200(slab|head,Node 0,Zone 2,Lastcpupid 0x1fffff)
> > It may be strange to compare the value with the string.
>
> More I'm looking at it, more I'm thinking it should have different specifiers
> for each group of desired flags to be printed.
>
> So, you leave %pGp as is and then add another letter to add more details, so
> user will choose what and in which order they want.
>
> For example, let's assume %pGp == %pGpf and P is a new specifier for what you
> are initially adding here:
>
> %pGpfP => referenced|uptodate|lru|active|private,Node 0,Zone 2
> %pGpPf => Node 0,Zone 2,referenced|uptodate|lru|active|private
>
> and so on.
And I agree with Matthew about format, but it doesn't oppose my suggestion
above.
--
With Best Regards,
Andy Shevchenko
On Mon, 2021-02-01 at 14:15 +0000, Matthew Wilcox wrote:
> On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
> > - Before the patch,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
>
> I would suggest it will be easier to parse as:
>
> flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
>
> That should alleviate the concerns about debugfs format change -- we've
> never guaranteed that flag names won't change, and they now look enough
> like flags that parsers shouldn't fall over them.
Seems sensible and would make the generating code simpler too.
But is it worth the vsprintf code expansion for the 5 current uses?
mm/debug.c: pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
mm/memory-failure.c: pr_info("soft offline: %#lx: %s migration failed %d, type %lx (%pGp)\n",
mm/memory-failure.c: pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %lx (%pGp)\n",
mm/memory-failure.c: pr_info("%s: %#lx: unknown page type: %lx (%pGp)\n",
mm/page_owner.c: "PFN %lu type %s Block %lu type %s Flags %#lx(%pGp)\n",
Wouldn't it be more sensible just to put this code in a new function
call in mm?
On Mon, Feb 01, 2021 at 10:51:03AM -0800, Joe Perches wrote:
> On Mon, 2021-02-01 at 14:15 +0000, Matthew Wilcox wrote:
> > On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
> > > - Before the patch,
> > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > >
> > > - After the patch,
> > > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> >
> > I would suggest it will be easier to parse as:
> >
> > flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> >
> > That should alleviate the concerns about debugfs format change -- we've
> > never guaranteed that flag names won't change, and they now look enough
> > like flags that parsers shouldn't fall over them.
>
> Seems sensible and would make the generating code simpler too.
>
> But is it worth the vsprintf code expansion for the 5 current uses?
>
> mm/debug.c: pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
> mm/memory-failure.c: pr_info("soft offline: %#lx: %s migration failed %d, type %lx (%pGp)\n",
> mm/memory-failure.c: pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %lx (%pGp)\n",
> mm/memory-failure.c: pr_info("%s: %#lx: unknown page type: %lx (%pGp)\n",
> mm/page_owner.c: "PFN %lu type %s Block %lu type %s Flags %#lx(%pGp)\n",
>
> Wouldn't it be more sensible just to put this code in a new function
> call in mm?
Does it matter whether the code lives in vsprintf.c or mm/debug.c? It's
built into the kernel core either way. I'm not a huge fan of the current
way %pFoo is handled, but unless/until it's drastically revised, I don't
think this proposed patch makes anything worse.
On Tue, Feb 2, 2021 at 12:16 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Feb 01, 2021 at 09:49:59PM +0800, Yafang Shao wrote:
> > On Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote:
>
> ...
>
> > > > - Before the patch,
> > > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > > >
> > > > - After the patch,
> > > > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head)
> > > >
> > > > The Documentation and test cases are also updated.
> > >
> > > Thanks for an update, my comments below.
> > >
> > > ...
> > >
> > > > - %pGp referenced|uptodate|lru|active|private
> > > > + %pGp Node 0,Zone 2,referenced|uptodate|lru|active|private
> > >
> > > Since of the nature of printf() buffer, I wonder if these should be at the end.
> > > I.o.w. the question is is the added material more important to user to see than
> > > the existed one?
> > >
> >
> > The existing one should be more important than the added one.
> > But the order of output will not match with the value for page->flags.
> > E.g.
> > flags=0x17ffffc0010200(slab|head,Node 0,Zone 2,Lastcpupid 0x1fffff)
> > It may be strange to compare the value with the string.
>
> More I'm looking at it, more I'm thinking it should have different specifiers
> for each group of desired flags to be printed.
>
> So, you leave %pGp as is and then add another letter to add more details, so
> user will choose what and in which order they want.
>
> For example, let's assume %pGp == %pGpf and P is a new specifier for what you
> are initially adding here:
>
> %pGpfP => referenced|uptodate|lru|active|private,Node 0,Zone 2
> %pGpPf => Node 0,Zone 2,referenced|uptodate|lru|active|private
>
> and so on.
Thanks for your suggestion. I will think about it.
--
Thanks
Yafang