| Message ID | 20211012182647.1605095-6-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improvements to %pGp | expand |
On Wed, Oct 13, 2021 at 2:34 AM Matthew Wilcox (Oracle) <willy@infradead.org> wrote: > > All existing users of %pGp want the hex value as well as the decoded > flag names. This looks awkward (passing the same parameter to printf > twice), so move that functionality into the core. If we want, we > can make that optional with flag arguments to %pGp in the future. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > lib/test_printf.c | 10 +++++++++- > lib/vsprintf.c | 8 ++++++++ > mm/debug.c | 2 +- > mm/memory-failure.c | 8 ++++---- > mm/page_owner.c | 4 ++-- > mm/slub.c | 4 ++-- > 6 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 662c3785aa57..a60b1a749e87 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -613,6 +613,10 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > bool append = false; > int i; > > + for (i = 0; i < ARRAY_SIZE(values); i++) > + page_flags |= (values[i] & pft[i].mask) << pft[i].shift; > + snprintf(cmp_buf + size, BUF_SIZE - size, "%#lx(", page_flags); > + size = strlen(cmp_buf); > if (flags & PAGEFLAGS_MASK) { > snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); > size = strlen(cmp_buf); > @@ -628,7 +632,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > cmp_buf[size] = '\0'; > } > > - page_flags |= (values[i] & pft[i].mask) << pft[i].shift; > snprintf(cmp_buf + size, BUF_SIZE - size, "%s=", pft[i].name); > size = strlen(cmp_buf); > snprintf(cmp_buf + size, BUF_SIZE - size, pft[i].fmt, > @@ -637,6 +640,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > append = true; > } > > + if (size < BUF_SIZE) { Same with the prev patch. Should it be: if (size < BUF_SIZE - 1) { ? Except that, the other parts look good to me. > + cmp_buf[size++] = ')'; > + cmp_buf[size] = '\0'; > + } > + > test(cmp_buf, "%pGp", &page_flags); > } > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index d7ad44f2c8f5..214098248610 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -2023,6 +2023,11 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) > bool append = false; > int i; > > + buf = number(buf, end, flags, default_flag_spec); > + if (buf < end) > + *buf = '('; > + buf++; > + > /* Page flags from the main area. */ > if (main_flags) { > buf = format_flags(buf, end, main_flags, pageflag_names); > @@ -2051,6 +2056,9 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) > > append = true; > } > + if (buf < end) > + *buf = ')'; > + buf++; > > return buf; > } > diff --git a/mm/debug.c b/mm/debug.c > index fae0f81ad831..714be101dec9 100644 > --- a/mm/debug.c > +++ b/mm/debug.c > @@ -162,7 +162,7 @@ static void __dump_page(struct page *page) > out_mapping: > BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1); > > - pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags, > + pr_warn("%sflags: %pGp%s\n", type, &head->flags, > page_cma ? " CMA" : ""); > print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32, > sizeof(unsigned long), page, > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 3e6449f2102a..e4e122a2e9b1 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -2109,14 +2109,14 @@ static int __soft_offline_page(struct page *page) > if (!list_empty(&pagelist)) > putback_movable_pages(&pagelist); > > - pr_info("soft offline: %#lx: %s migration failed %d, type %lx (%pGp)\n", > - pfn, msg_page[huge], ret, page->flags, &page->flags); > + pr_info("soft offline: %#lx: %s migration failed %d, type %pGp\n", > + pfn, msg_page[huge], ret, &page->flags); > if (ret > 0) > ret = -EBUSY; > } > } else { > - pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %lx (%pGp)\n", > - pfn, msg_page[huge], page_count(page), page->flags, &page->flags); > + pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %pGp\n", > + pfn, msg_page[huge], page_count(page), &page->flags); > ret = -EBUSY; > } > return ret; > diff --git a/mm/page_owner.c b/mm/page_owner.c > index 62402d22539b..4afc713ca525 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -351,12 +351,12 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > pageblock_mt = get_pageblock_migratetype(page); > page_mt = gfp_migratetype(page_owner->gfp_mask); > ret += snprintf(kbuf + ret, count - ret, > - "PFN %lu type %s Block %lu type %s Flags %#lx(%pGp)\n", > + "PFN %lu type %s Block %lu type %s Flags %pGp\n", > pfn, > migratetype_names[page_mt], > pfn >> pageblock_order, > migratetype_names[pageblock_mt], > - page->flags, &page->flags); > + &page->flags); > > if (ret >= count) > goto err; > diff --git a/mm/slub.c b/mm/slub.c > index 3d2025f7163b..f7ac28646580 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -763,9 +763,9 @@ void print_tracking(struct kmem_cache *s, void *object) > > static void print_page_info(struct page *page) > { > - pr_err("Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n", > + pr_err("Slab 0x%p objects=%u used=%u fp=0x%p flags=%pGp\n", > page, page->objects, page->inuse, page->freelist, > - page->flags, &page->flags); > + &page->flags); > > } > > -- > 2.32.0 >
On Tue 2021-10-12 19:26:47, Matthew Wilcox (Oracle) wrote: > All existing users of %pGp want the hex value as well as the decoded > flag names. This looks awkward (passing the same parameter to printf > twice), so move that functionality into the core. If we want, we > can make that optional with flag arguments to %pGp in the future. It makes sense. Just the selftest code is pain, see below ;-) > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 662c3785aa57..a60b1a749e87 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -613,6 +613,10 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > bool append = false; > int i; > > + for (i = 0; i < ARRAY_SIZE(values); i++) > + page_flags |= (values[i] & pft[i].mask) << pft[i].shift; I can't get the idea behind this. IMHO, the value might be zero even when the related flag is set. And the %pGp code seems to always print page flags from page_flags_fields[] when the field width is not zero. Or do I misread the code? Best Regards, Petr
On 10/12/21 11:56 PM, Matthew Wilcox (Oracle) wrote: > All existing users of %pGp want the hex value as well as the decoded > flag names. This looks awkward (passing the same parameter to printf Agreed, it is awkward. Always thought about this, passing the exact same parameter twice for the print functions. > twice), so move that functionality into the core. If we want, we > can make that optional with flag arguments to %pGp in the future. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > lib/test_printf.c | 10 +++++++++- > lib/vsprintf.c | 8 ++++++++ > mm/debug.c | 2 +- > mm/memory-failure.c | 8 ++++---- > mm/page_owner.c | 4 ++-- > mm/slub.c | 4 ++-- > 6 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 662c3785aa57..a60b1a749e87 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -613,6 +613,10 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > bool append = false; > int i; > > + for (i = 0; i < ARRAY_SIZE(values); i++) > + page_flags |= (values[i] & pft[i].mask) << pft[i].shift; > + snprintf(cmp_buf + size, BUF_SIZE - size, "%#lx(", page_flags); > + size = strlen(cmp_buf); > if (flags & PAGEFLAGS_MASK) { > snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); > size = strlen(cmp_buf); > @@ -628,7 +632,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > cmp_buf[size] = '\0'; > } > > - page_flags |= (values[i] & pft[i].mask) << pft[i].shift; > snprintf(cmp_buf + size, BUF_SIZE - size, "%s=", pft[i].name); > size = strlen(cmp_buf); > snprintf(cmp_buf + size, BUF_SIZE - size, pft[i].fmt, > @@ -637,6 +640,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > append = true; > } > > + if (size < BUF_SIZE) { Should be BUF_SIZE - 1 instead ? But seems like it is still inconclusive whether writing these directly into the buffer, instead of snprintf() is better or not. > + cmp_buf[size++] = ')'; > + cmp_buf[size] = '\0'; > + } > + > test(cmp_buf, "%pGp", &page_flags); > } > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index d7ad44f2c8f5..214098248610 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -2023,6 +2023,11 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) > bool append = false; > int i; > > + buf = number(buf, end, flags, default_flag_spec); > + if (buf < end) > + *buf = '('; > + buf++; > + > /* Page flags from the main area. */ > if (main_flags) { > buf = format_flags(buf, end, main_flags, pageflag_names); > @@ -2051,6 +2056,9 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) > > append = true; > } > + if (buf < end) > + *buf = ')'; > + buf++; > > return buf; > } > diff --git a/mm/debug.c b/mm/debug.c > index fae0f81ad831..714be101dec9 100644 > --- a/mm/debug.c > +++ b/mm/debug.c > @@ -162,7 +162,7 @@ static void __dump_page(struct page *page) > out_mapping: > BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1); > > - pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags, > + pr_warn("%sflags: %pGp%s\n", type, &head->flags, > page_cma ? " CMA" : ""); > print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32, > sizeof(unsigned long), page, > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 3e6449f2102a..e4e122a2e9b1 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -2109,14 +2109,14 @@ static int __soft_offline_page(struct page *page) > if (!list_empty(&pagelist)) > putback_movable_pages(&pagelist); > > - pr_info("soft offline: %#lx: %s migration failed %d, type %lx (%pGp)\n", > - pfn, msg_page[huge], ret, page->flags, &page->flags); > + pr_info("soft offline: %#lx: %s migration failed %d, type %pGp\n", > + pfn, msg_page[huge], ret, &page->flags); > if (ret > 0) > ret = -EBUSY; > } > } else { > - pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %lx (%pGp)\n", > - pfn, msg_page[huge], page_count(page), page->flags, &page->flags); > + pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %pGp\n", > + pfn, msg_page[huge], page_count(page), &page->flags); > ret = -EBUSY; > } > return ret; > diff --git a/mm/page_owner.c b/mm/page_owner.c > index 62402d22539b..4afc713ca525 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -351,12 +351,12 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > pageblock_mt = get_pageblock_migratetype(page); > page_mt = gfp_migratetype(page_owner->gfp_mask); > ret += snprintf(kbuf + ret, count - ret, > - "PFN %lu type %s Block %lu type %s Flags %#lx(%pGp)\n", > + "PFN %lu type %s Block %lu type %s Flags %pGp\n", > pfn, > migratetype_names[page_mt], > pfn >> pageblock_order, > migratetype_names[pageblock_mt], > - page->flags, &page->flags); > + &page->flags); > > if (ret >= count) > goto err; > diff --git a/mm/slub.c b/mm/slub.c > index 3d2025f7163b..f7ac28646580 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -763,9 +763,9 @@ void print_tracking(struct kmem_cache *s, void *object) > > static void print_page_info(struct page *page) > { > - pr_err("Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n", > + pr_err("Slab 0x%p objects=%u used=%u fp=0x%p flags=%pGp\n", > page, page->objects, page->inuse, page->freelist, > - page->flags, &page->flags); > + &page->flags); > > } Otherwise this change LGTM.
On Fri 2021-10-15 12:55:48, Petr Mladek wrote: > On Tue 2021-10-12 19:26:47, Matthew Wilcox (Oracle) wrote: > > All existing users of %pGp want the hex value as well as the decoded > > flag names. This looks awkward (passing the same parameter to printf > > twice), so move that functionality into the core. If we want, we > > can make that optional with flag arguments to %pGp in the future. > > It makes sense. Just the selftest code is pain, see below ;-) > > > diff --git a/lib/test_printf.c b/lib/test_printf.c > > index 662c3785aa57..a60b1a749e87 100644 > > --- a/lib/test_printf.c > > +++ b/lib/test_printf.c > > @@ -613,6 +613,10 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > > bool append = false; > > int i; > > > > + for (i = 0; i < ARRAY_SIZE(values); i++) > > + page_flags |= (values[i] & pft[i].mask) << pft[i].shift; > > I can't get the idea behind this. IMHO, the value might be zero even > when the related flag is set. > > And the %pGp code seems to always print page flags from > page_flags_fields[] when the field width is not zero. > > Or do I misread the code? Just for record. I really did misread the code. It constructs flags from the given values. It does reverse operation to format_page_flags(). Best Regards, Petr
diff --git a/lib/test_printf.c b/lib/test_printf.c index 662c3785aa57..a60b1a749e87 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -613,6 +613,10 @@ page_flags_test(int section, int node, int zone, int last_cpupid, bool append = false; int i; + for (i = 0; i < ARRAY_SIZE(values); i++) + page_flags |= (values[i] & pft[i].mask) << pft[i].shift; + snprintf(cmp_buf + size, BUF_SIZE - size, "%#lx(", page_flags); + size = strlen(cmp_buf); if (flags & PAGEFLAGS_MASK) { snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); size = strlen(cmp_buf); @@ -628,7 +632,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid, cmp_buf[size] = '\0'; } - page_flags |= (values[i] & pft[i].mask) << pft[i].shift; snprintf(cmp_buf + size, BUF_SIZE - size, "%s=", pft[i].name); size = strlen(cmp_buf); snprintf(cmp_buf + size, BUF_SIZE - size, pft[i].fmt, @@ -637,6 +640,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid, append = true; } + if (size < BUF_SIZE) { + cmp_buf[size++] = ')'; + cmp_buf[size] = '\0'; + } + test(cmp_buf, "%pGp", &page_flags); } diff --git a/lib/vsprintf.c b/lib/vsprintf.c index d7ad44f2c8f5..214098248610 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -2023,6 +2023,11 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) bool append = false; int i; + buf = number(buf, end, flags, default_flag_spec); + if (buf < end) + *buf = '('; + buf++; + /* Page flags from the main area. */ if (main_flags) { buf = format_flags(buf, end, main_flags, pageflag_names); @@ -2051,6 +2056,9 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) append = true; } + if (buf < end) + *buf = ')'; + buf++; return buf; } diff --git a/mm/debug.c b/mm/debug.c index fae0f81ad831..714be101dec9 100644 --- a/mm/debug.c +++ b/mm/debug.c @@ -162,7 +162,7 @@ static void __dump_page(struct page *page) out_mapping: BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1); - pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags, + pr_warn("%sflags: %pGp%s\n", type, &head->flags, page_cma ? " CMA" : ""); print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32, sizeof(unsigned long), page, diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 3e6449f2102a..e4e122a2e9b1 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -2109,14 +2109,14 @@ static int __soft_offline_page(struct page *page) if (!list_empty(&pagelist)) putback_movable_pages(&pagelist); - pr_info("soft offline: %#lx: %s migration failed %d, type %lx (%pGp)\n", - pfn, msg_page[huge], ret, page->flags, &page->flags); + pr_info("soft offline: %#lx: %s migration failed %d, type %pGp\n", + pfn, msg_page[huge], ret, &page->flags); if (ret > 0) ret = -EBUSY; } } else { - pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %lx (%pGp)\n", - pfn, msg_page[huge], page_count(page), page->flags, &page->flags); + pr_info("soft offline: %#lx: %s isolation failed, page count %d, type %pGp\n", + pfn, msg_page[huge], page_count(page), &page->flags); ret = -EBUSY; } return ret; diff --git a/mm/page_owner.c b/mm/page_owner.c index 62402d22539b..4afc713ca525 100644 --- a/mm/page_owner.c +++ b/mm/page_owner.c @@ -351,12 +351,12 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, pageblock_mt = get_pageblock_migratetype(page); page_mt = gfp_migratetype(page_owner->gfp_mask); ret += snprintf(kbuf + ret, count - ret, - "PFN %lu type %s Block %lu type %s Flags %#lx(%pGp)\n", + "PFN %lu type %s Block %lu type %s Flags %pGp\n", pfn, migratetype_names[page_mt], pfn >> pageblock_order, migratetype_names[pageblock_mt], - page->flags, &page->flags); + &page->flags); if (ret >= count) goto err; diff --git a/mm/slub.c b/mm/slub.c index 3d2025f7163b..f7ac28646580 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -763,9 +763,9 @@ void print_tracking(struct kmem_cache *s, void *object) static void print_page_info(struct page *page) { - pr_err("Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n", + pr_err("Slab 0x%p objects=%u used=%u fp=0x%p flags=%pGp\n", page, page->objects, page->inuse, page->freelist, - page->flags, &page->flags); + &page->flags); }
All existing users of %pGp want the hex value as well as the decoded flag names. This looks awkward (passing the same parameter to printf twice), so move that functionality into the core. If we want, we can make that optional with flag arguments to %pGp in the future. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- lib/test_printf.c | 10 +++++++++- lib/vsprintf.c | 8 ++++++++ mm/debug.c | 2 +- mm/memory-failure.c | 8 ++++---- mm/page_owner.c | 4 ++-- mm/slub.c | 4 ++-- 6 files changed, 26 insertions(+), 10 deletions(-)