| Message ID | ZHI0YKzZADjr1nyq@casper.infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Do not print page type when the page has no type | expand |
On Sat, May 27, 2023 at 05:48:32PM +0100, Matthew Wilcox wrote: > It is confusing and unnecessary to print the page type when the > page has no type. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > --- > > I did not run the test-suite. I'm in the middle of debugging something > else and this is an unwelcome distraction. If this doesn't work quite > right, please fix it. I ran the test suite, and a test case failed: [ 1.001641] test_printf: loaded. [ 1.001736] test_printf: vsnprintf(buf, 256, "%pGt", ...) returned 41, expected 5 [ 1.001738] test_printf: vsnprintf(buf, 2, "%pGt", ...) returned 41, expected 5 [ 1.001738] test_printf: vsnprintf(buf, 0, "%pGt", ...) returned 41, expected 5 [ 1.001739] test_printf: kvasprintf(..., "%pGt", ...) returned '0xa(offline|guard|table|buddy|0xfffff875)', expected '0xa()' [ 1.001774] test_printf: failed 4 out of 432 tests The code mostly looks fine and the patch makes sense to me. But I'm not sure if it's a nice behavior to print garbage when it does not have a page type, although I can hardly imagine users of this flag other than __dump_page(). I'd rather keep printk part unchanged and add page_has_type() check in __dump_page(). Thanks!
On Sun 2023-05-28 17:01:59, Hyeonggon Yoo wrote: > On Sat, May 27, 2023 at 05:48:32PM +0100, Matthew Wilcox wrote: > > It is confusing and unnecessary to print the page type when the > > page has no type. > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > > > --- > > > > I did not run the test-suite. I'm in the middle of debugging something > > else and this is an unwelcome distraction. If this doesn't work quite > > right, please fix it. > > I ran the test suite, and a test case failed: > > [ 1.001641] test_printf: loaded. > [ 1.001736] test_printf: vsnprintf(buf, 256, "%pGt", ...) returned 41, expected 5 > [ 1.001738] test_printf: vsnprintf(buf, 2, "%pGt", ...) returned 41, expected 5 > [ 1.001738] test_printf: vsnprintf(buf, 0, "%pGt", ...) returned 41, expected 5 > [ 1.001739] test_printf: kvasprintf(..., "%pGt", ...) returned '0xa(offline|guard|table|buddy|0xfffff875)', expected '0xa()' > [ 1.001774] test_printf: failed 4 out of 432 tests > > The code mostly looks fine and the patch makes sense to me. > But I'm not sure if it's a nice behavior to print garbage when it does not > have a page type, although I can hardly imagine users of this flag other > than __dump_page(). I'd rather keep printk part unchanged and add > page_has_type() check in __dump_page(). I agree with Hyeonggon. The change in __dump_page() makes sense. But vsprintf() should stay clever and do not print garbage. Best Regards, Petr
On Mon, May 29, 2023 at 05:27:29PM +0200, Petr Mladek wrote: > On Sun 2023-05-28 17:01:59, Hyeonggon Yoo wrote: > > The code mostly looks fine and the patch makes sense to me. > > But I'm not sure if it's a nice behavior to print garbage when it does not > > have a page type, although I can hardly imagine users of this flag other > > than __dump_page(). I'd rather keep printk part unchanged and add > > page_has_type() check in __dump_page(). > > I agree with Hyeonggon. The change in __dump_page() makes sense. > But vsprintf() should stay clever and do not print garbage. The caller (and, let's face it, there's only ever going to be one caller) shouldn't pass garbage in the first place.
On Mon 2023-05-29 16:42:09, Matthew Wilcox wrote: > On Mon, May 29, 2023 at 05:27:29PM +0200, Petr Mladek wrote: > > On Sun 2023-05-28 17:01:59, Hyeonggon Yoo wrote: > > > The code mostly looks fine and the patch makes sense to me. > > > But I'm not sure if it's a nice behavior to print garbage when it does not > > > have a page type, although I can hardly imagine users of this flag other > > > than __dump_page(). I'd rather keep printk part unchanged and add > > > page_has_type() check in __dump_page(). > > > > I agree with Hyeonggon. The change in __dump_page() makes sense. > > But vsprintf() should stay clever and do not print garbage. > > The caller (and, let's face it, there's only ever going to be one > caller) shouldn't pass garbage in the first place. By other words you say that vsprintf() might produce "garbage" when the given value is invalid. And the check is not worth the complexity. It might make sense. Well, we should keep test of the invalid value in test_printf.c. And it would need to be updated when a new page_type is added because it would produce another garbage. From this POV, I would prefer to keep the check in vsprintf.c. All in all. It is about a compromise. I can't judge how much the extra page_type_has_type() complicates mm code. Best Regards, Petr
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h index 1c68d67b832f..3ac7b7e697a3 100644 --- a/include/linux/page-flags.h +++ b/include/linux/page-flags.h @@ -903,14 +903,9 @@ static inline bool is_page_hwpoison(struct page *page) #define PageType(page, flag) \ ((page->page_type & (PAGE_TYPE_BASE | flag)) == PAGE_TYPE_BASE) -static inline int page_type_has_type(unsigned int page_type) -{ - return (int)page_type < PAGE_MAPCOUNT_RESERVE; -} - static inline int page_has_type(struct page *page) { - return page_type_has_type(page->page_type); + return (int)page->page_type < PAGE_MAPCOUNT_RESERVE; } #define PAGE_TYPE_OPS(uname, lname) \ diff --git a/lib/test_printf.c b/lib/test_printf.c index 7677ebccf3c3..442e4c41bccb 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -648,9 +648,7 @@ static void __init page_type_test(unsigned int page_type, const char *name, unsigned long size; size = scnprintf(cmp_buf, BUF_SIZE, "%#x(", page_type); - if (page_type_has_type(page_type)) - size += scnprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); - + size += scnprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); snprintf(cmp_buf + size, BUF_SIZE - size, ")"); test(cmp_buf, "%pGt", &page_type); } diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 40f560959b16..0a0e5c4cffc9 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -2061,8 +2061,7 @@ char *format_page_type(char *buf, char *end, unsigned int page_type) *buf = '('; buf++; - if (page_type_has_type(page_type)) - buf = format_flags(buf, end, ~page_type, pagetype_names); + buf = format_flags(buf, end, ~page_type, pagetype_names); if (buf < end) *buf = ')'; diff --git a/mm/debug.c b/mm/debug.c index c7b228097bd9..6eb0ecb9b368 100644 --- a/mm/debug.c +++ b/mm/debug.c @@ -120,7 +120,8 @@ static void __dump_page(struct page *page) pr_warn("%sflags: %pGp%s\n", type, &head->flags, page_cma ? " CMA" : ""); - pr_warn("page_type: %pGt\n", &head->page_type); + if (page_has_type(head)) + pr_warn("page_type: %pGt\n", &head->page_type); print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32, sizeof(unsigned long), page,
It is confusing and unnecessary to print the page type when the page has no type. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- I did not run the test-suite. I'm in the middle of debugging something else and this is an unwelcome distraction. If this doesn't work quite right, please fix it.