[PATCH] Do not print page type when the page has no type

[PATCH] Do not print page type when the page has no type

[Matthew Wilcox]

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.

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,

Re: [PATCH] Do not print page type when the page has no type

[Hyeonggon Yoo]

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!

-- 
Hyeonggon Yoo

Doing kernel stuff as a hobby
Undergraduate | Chungnam National University
Dept. Computer Science & Engineering

Re: [PATCH] Do not print page type when the page has no type

[Petr Mladek]

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

Re: [PATCH] Do not print page type when the page has no type

[Matthew Wilcox]

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.

Re: [PATCH] Do not print page type when the page has no type

[Petr Mladek]

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