* [PATCH v2 0/3] mm, vsprintf: dump full information of page flags in pGp @ 2021-02-01 11:56 Yafang Shao 2021-02-01 11:56 ` [PATCH v2 1/3] mm, slub: use pGp to print page flags Yafang Shao ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Yafang Shao @ 2021-02-01 11:56 UTC (permalink / raw) To: andriy.shevchenko, david, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux Cc: linux-mm, linux-kernel, Yafang Shao 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/3] mm, slub: use pGp to print page flags 2021-02-01 11:56 [PATCH v2 0/3] mm, vsprintf: dump full information of page flags in pGp Yafang Shao @ 2021-02-01 11:56 ` Yafang Shao 2021-02-01 11:56 ` [PATCH v2 2/3] mm, slub: don't combine pr_err with INFO Yafang Shao 2021-02-01 11:56 ` [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp Yafang Shao 2 siblings, 0 replies; 20+ messages in thread From: Yafang Shao @ 2021-02-01 11:56 UTC (permalink / raw) To: andriy.shevchenko, david, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux Cc: linux-mm, linux-kernel, Yafang Shao 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 <david@redhat.com> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Acked-by: David Rientjes <rientjes@google.com> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/3] mm, slub: don't combine pr_err with INFO 2021-02-01 11:56 [PATCH v2 0/3] mm, vsprintf: dump full information of page flags in pGp Yafang Shao 2021-02-01 11:56 ` [PATCH v2 1/3] mm, slub: use pGp to print page flags Yafang Shao @ 2021-02-01 11:56 ` Yafang Shao 2021-02-01 11:56 ` [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp Yafang Shao 2 siblings, 0 replies; 20+ messages in thread From: Yafang Shao @ 2021-02-01 11:56 UTC (permalink / raw) To: andriy.shevchenko, david, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux Cc: linux-mm, linux-kernel, Yafang Shao It is strange to combine "pr_err" with "INFO", so let's remove the prefix completely. This patch is motivated by David's comment[1]. - before the patch [ 8846.517809] INFO: Slab 0x00000000f42a2c60 objects=33 used=3 fp=0x0000000060d32ca8 flags=0x17ffffc0010200(slab|head) - after the patch [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head) [1]. https://lore.kernel.org/linux-mm/b9c0f2b6-e9b0-0c36-ebdd-2bc684c5a762@redhat.com/#t Suggested-by: Vlastimil Babka <vbabka@suse.cz> Cc: David Hildenbrand <david@redhat.com> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- mm/slub.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index 87ff086e68a4..2514c37ab4e4 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -612,7 +612,7 @@ static void print_track(const char *s, struct track *t, unsigned long pr_time) if (!t->addr) return; - pr_err("INFO: %s in %pS age=%lu cpu=%u pid=%d\n", + pr_err("%s in %pS age=%lu cpu=%u pid=%d\n", s, (void *)t->addr, pr_time - t->when, t->cpu, t->pid); #ifdef CONFIG_STACKTRACE { @@ -638,7 +638,7 @@ 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=%#lx(%pGp)\n", + pr_err("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); @@ -695,7 +695,7 @@ static void print_trailer(struct kmem_cache *s, struct page *page, u8 *p) print_page_info(page); - pr_err("INFO: Object 0x%p @offset=%tu fp=0x%p\n\n", + pr_err("Object 0x%p @offset=%tu fp=0x%p\n\n", p, p - addr, get_freepointer(s, p)); if (s->flags & SLAB_RED_ZONE) @@ -788,7 +788,7 @@ static int check_bytes_and_report(struct kmem_cache *s, struct page *page, end--; slab_bug(s, "%s overwritten", what); - pr_err("INFO: 0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n", + pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n", fault, end - 1, fault - addr, fault[0], value); print_trailer(s, page, object); @@ -3854,7 +3854,7 @@ static void list_slab_objects(struct kmem_cache *s, struct page *page, for_each_object(p, s, addr, page->objects) { if (!test_bit(__obj_to_index(s, addr, p), map)) { - pr_err("INFO: Object 0x%p @offset=%tu\n", p, p - addr); + pr_err("Object 0x%p @offset=%tu\n", p, p - addr); print_tracking(s, p); } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 11:56 [PATCH v2 0/3] mm, vsprintf: dump full information of page flags in pGp Yafang Shao 2021-02-01 11:56 ` [PATCH v2 1/3] mm, slub: use pGp to print page flags Yafang Shao 2021-02-01 11:56 ` [PATCH v2 2/3] mm, slub: don't combine pr_err with INFO Yafang Shao @ 2021-02-01 11:56 ` Yafang Shao 2021-02-01 13:05 ` Joe Perches ` (3 more replies) 2 siblings, 4 replies; 20+ messages in thread From: Yafang Shao @ 2021-02-01 11:56 UTC (permalink / raw) To: andriy.shevchenko, david, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux Cc: linux-mm, linux-kernel, Yafang Shao, Joe Perches 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(-) 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 11:56 ` [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp Yafang Shao @ 2021-02-01 13:05 ` Joe Perches 2021-02-01 13:27 ` Yafang Shao 2021-02-01 13:23 ` David Hildenbrand ` (2 subsequent siblings) 3 siblings, 1 reply; 20+ messages in thread From: Joe Perches @ 2021-02-01 13:05 UTC (permalink / raw) To: Yafang Shao, andriy.shevchenko, david, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux Cc: linux-mm, linux-kernel 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; > +} > + ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 13:05 ` Joe Perches @ 2021-02-01 13:27 ` Yafang Shao 0 siblings, 0 replies; 20+ messages in thread From: Yafang Shao @ 2021-02-01 13:27 UTC (permalink / raw) To: Joe Perches Cc: Andy Shevchenko, David Hildenbrand, Vlastimil Babka, Miaohe Lin, Christoph Lameter, penberg, David Rientjes, iamjoonsoo.kim, Andrew Morton, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Linux MM, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 11:56 ` [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp Yafang Shao 2021-02-01 13:05 ` Joe Perches @ 2021-02-01 13:23 ` David Hildenbrand 2021-02-01 13:32 ` David Hildenbrand 2021-02-01 13:34 ` Andy Shevchenko 2021-02-01 13:27 ` Andy Shevchenko 2021-02-01 14:15 ` Matthew Wilcox 3 siblings, 2 replies; 20+ messages in thread From: David Hildenbrand @ 2021-02-01 13:23 UTC (permalink / raw) To: Yafang Shao, andriy.shevchenko, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux Cc: linux-mm, linux-kernel, Joe Perches 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 13:23 ` David Hildenbrand @ 2021-02-01 13:32 ` David Hildenbrand 2021-02-01 13:34 ` Andy Shevchenko 1 sibling, 0 replies; 20+ messages in thread From: David Hildenbrand @ 2021-02-01 13:32 UTC (permalink / raw) To: Yafang Shao, andriy.shevchenko, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux Cc: linux-mm, linux-kernel, Joe Perches 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 13:23 ` David Hildenbrand 2021-02-01 13:32 ` David Hildenbrand @ 2021-02-01 13:34 ` Andy Shevchenko 2021-02-01 13:52 ` Yafang Shao 1 sibling, 1 reply; 20+ messages in thread From: Andy Shevchenko @ 2021-02-01 13:34 UTC (permalink / raw) To: David Hildenbrand Cc: Yafang Shao, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux, linux-mm, linux-kernel, Joe Perches 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 13:34 ` Andy Shevchenko @ 2021-02-01 13:52 ` Yafang Shao 2021-02-01 16:03 ` Andy Shevchenko 0 siblings, 1 reply; 20+ messages in thread From: Yafang Shao @ 2021-02-01 13:52 UTC (permalink / raw) To: Andy Shevchenko Cc: David Hildenbrand, Vlastimil Babka, Miaohe Lin, Christoph Lameter, penberg, David Rientjes, iamjoonsoo.kim, Andrew Morton, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Linux MM, LKML, Joe Perches 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 ? -- Thanks Yafang ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 13:52 ` Yafang Shao @ 2021-02-01 16:03 ` Andy Shevchenko 0 siblings, 0 replies; 20+ messages in thread From: Andy Shevchenko @ 2021-02-01 16:03 UTC (permalink / raw) To: Yafang Shao Cc: David Hildenbrand, Vlastimil Babka, Miaohe Lin, Christoph Lameter, penberg, David Rientjes, iamjoonsoo.kim, Andrew Morton, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Linux MM, LKML, Joe Perches 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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 11:56 ` [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp Yafang Shao 2021-02-01 13:05 ` Joe Perches 2021-02-01 13:23 ` David Hildenbrand @ 2021-02-01 13:27 ` Andy Shevchenko 2021-02-01 13:49 ` Yafang Shao 2021-02-01 14:15 ` Matthew Wilcox 3 siblings, 1 reply; 20+ messages in thread From: Andy Shevchenko @ 2021-02-01 13:27 UTC (permalink / raw) To: Yafang Shao Cc: david, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux, linux-mm, linux-kernel, Joe Perches 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 13:27 ` Andy Shevchenko @ 2021-02-01 13:49 ` Yafang Shao 2021-02-01 16:16 ` Andy Shevchenko 0 siblings, 1 reply; 20+ messages in thread From: Yafang Shao @ 2021-02-01 13:49 UTC (permalink / raw) To: Andy Shevchenko Cc: David Hildenbrand, Vlastimil Babka, Miaohe Lin, Christoph Lameter, penberg, David Rientjes, iamjoonsoo.kim, Andrew Morton, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Linux MM, LKML, Joe Perches 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. -- Thanks Yafang ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 13:49 ` Yafang Shao @ 2021-02-01 16:16 ` Andy Shevchenko 2021-02-01 16:20 ` Andy Shevchenko 2021-02-02 13:25 ` Yafang Shao 0 siblings, 2 replies; 20+ messages in thread From: Andy Shevchenko @ 2021-02-01 16:16 UTC (permalink / raw) To: Yafang Shao Cc: David Hildenbrand, Vlastimil Babka, Miaohe Lin, Christoph Lameter, penberg, David Rientjes, iamjoonsoo.kim, Andrew Morton, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Linux MM, LKML, Joe Perches 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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 16:16 ` Andy Shevchenko @ 2021-02-01 16:20 ` Andy Shevchenko 2021-02-02 13:25 ` Yafang Shao 1 sibling, 0 replies; 20+ messages in thread From: Andy Shevchenko @ 2021-02-01 16:20 UTC (permalink / raw) To: Yafang Shao Cc: David Hildenbrand, Vlastimil Babka, Miaohe Lin, Christoph Lameter, penberg, David Rientjes, iamjoonsoo.kim, Andrew Morton, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Linux MM, LKML, Joe Perches 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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 16:16 ` Andy Shevchenko 2021-02-01 16:20 ` Andy Shevchenko @ 2021-02-02 13:25 ` Yafang Shao 1 sibling, 0 replies; 20+ messages in thread From: Yafang Shao @ 2021-02-02 13:25 UTC (permalink / raw) To: Andy Shevchenko Cc: David Hildenbrand, Vlastimil Babka, Miaohe Lin, Christoph Lameter, penberg, David Rientjes, iamjoonsoo.kim, Andrew Morton, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Linux MM, LKML, Joe Perches 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. -- Thanks Yafang ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 11:56 ` [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp Yafang Shao ` (2 preceding siblings ...) 2021-02-01 13:27 ` Andy Shevchenko @ 2021-02-01 14:15 ` Matthew Wilcox 2021-02-01 14:44 ` Yafang Shao 2021-02-01 18:51 ` Joe Perches 3 siblings, 2 replies; 20+ messages in thread From: Matthew Wilcox @ 2021-02-01 14:15 UTC (permalink / raw) To: Yafang Shao Cc: andriy.shevchenko, david, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux, linux-mm, linux-kernel, Joe Perches 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 14:15 ` Matthew Wilcox @ 2021-02-01 14:44 ` Yafang Shao 2021-02-01 18:51 ` Joe Perches 1 sibling, 0 replies; 20+ messages in thread From: Yafang Shao @ 2021-02-01 14:44 UTC (permalink / raw) To: Matthew Wilcox Cc: Andy Shevchenko, David Hildenbrand, Vlastimil Babka, Miaohe Lin, Christoph Lameter, penberg, David Rientjes, iamjoonsoo.kim, Andrew Morton, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Linux MM, LKML, Joe Perches 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. -- Thanks Yafang ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 14:15 ` Matthew Wilcox 2021-02-01 14:44 ` Yafang Shao @ 2021-02-01 18:51 ` Joe Perches 2021-02-01 18:59 ` Matthew Wilcox 1 sibling, 1 reply; 20+ messages in thread From: Joe Perches @ 2021-02-01 18:51 UTC (permalink / raw) To: Matthew Wilcox, Yafang Shao Cc: andriy.shevchenko, david, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux, linux-mm, linux-kernel 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? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp 2021-02-01 18:51 ` Joe Perches @ 2021-02-01 18:59 ` Matthew Wilcox 0 siblings, 0 replies; 20+ messages in thread From: Matthew Wilcox @ 2021-02-01 18:59 UTC (permalink / raw) To: Joe Perches Cc: Yafang Shao, andriy.shevchenko, david, vbabka, linmiaohe, cl, penberg, rientjes, iamjoonsoo.kim, akpm, pmladek, rostedt, sergey.senozhatsky, linux, linux-mm, linux-kernel 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-02-02 13:26 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-01 11:56 [PATCH v2 0/3] mm, vsprintf: dump full information of page flags in pGp Yafang Shao 2021-02-01 11:56 ` [PATCH v2 1/3] mm, slub: use pGp to print page flags Yafang Shao 2021-02-01 11:56 ` [PATCH v2 2/3] mm, slub: don't combine pr_err with INFO Yafang Shao 2021-02-01 11:56 ` [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp Yafang Shao 2021-02-01 13:05 ` Joe Perches 2021-02-01 13:27 ` Yafang Shao 2021-02-01 13:23 ` David Hildenbrand 2021-02-01 13:32 ` David Hildenbrand 2021-02-01 13:34 ` Andy Shevchenko 2021-02-01 13:52 ` Yafang Shao 2021-02-01 16:03 ` Andy Shevchenko 2021-02-01 13:27 ` Andy Shevchenko 2021-02-01 13:49 ` Yafang Shao 2021-02-01 16:16 ` Andy Shevchenko 2021-02-01 16:20 ` Andy Shevchenko 2021-02-02 13:25 ` Yafang Shao 2021-02-01 14:15 ` Matthew Wilcox 2021-02-01 14:44 ` Yafang Shao 2021-02-01 18:51 ` Joe Perches 2021-02-01 18:59 ` Matthew Wilcox
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).