| Message ID | 20210201115610.87808-4-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, vsprintf: dump full information of page flags in pGp | expand |
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.
On Mon, Feb 1, 2021 at 9:05 PM Joe Perches <joe@perches.com> 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; > +}
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"; }
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.
On Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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.
On Mon, Feb 1, 2021 at 9:34 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 ?
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 <willy@infradead.org> 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.
On Mon, Feb 01, 2021 at 09:52:53PM +0800, Yafang Shao wrote: > On Mon, Feb 1, 2021 at 9:34 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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.
On Mon, Feb 01, 2021 at 09:49:59PM +0800, Yafang Shao wrote: > On Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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.
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 > > <andriy.shevchenko@linux.intel.com> 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.
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 <andriy.shevchenko@linux.intel.com> 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 > > <andriy.shevchenko@linux.intel.com> 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.
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;
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 <david@redhat.com> Cc: Joe Perches <joe@perches.com> Cc: Miaohe Lin <linmiaohe@huawei.com> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- Documentation/core-api/printk-formats.rst | 2 +- lib/test_printf.c | 65 ++++++++++++++++++----- lib/vsprintf.c | 58 +++++++++++++++++++- 3 files changed, 109 insertions(+), 16 deletions(-)