linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Do not print page type when the page has no type
@ 2023-05-27 16:48 Matthew Wilcox
  2023-05-28  8:01 ` Hyeonggon Yoo
  0 siblings, 1 reply; 5+ messages in thread
From: Matthew Wilcox @ 2023-05-27 16:48 UTC (permalink / raw)
  To: Hyeonggon Yoo, Andrew Morton, Petr Mladek; +Cc: linux-mm

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,


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] Do not print page type when the page has no type
  2023-05-27 16:48 [PATCH] Do not print page type when the page has no type Matthew Wilcox
@ 2023-05-28  8:01 ` Hyeonggon Yoo
  2023-05-29 15:27   ` Petr Mladek
  0 siblings, 1 reply; 5+ messages in thread
From: Hyeonggon Yoo @ 2023-05-28  8:01 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Andrew Morton, Petr Mladek, linux-mm

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Do not print page type when the page has no type
  2023-05-28  8:01 ` Hyeonggon Yoo
@ 2023-05-29 15:27   ` Petr Mladek
  2023-05-29 15:42     ` Matthew Wilcox
  0 siblings, 1 reply; 5+ messages in thread
From: Petr Mladek @ 2023-05-29 15:27 UTC (permalink / raw)
  To: Hyeonggon Yoo; +Cc: Matthew Wilcox, Andrew Morton, linux-mm

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Do not print page type when the page has no type
  2023-05-29 15:27   ` Petr Mladek
@ 2023-05-29 15:42     ` Matthew Wilcox
  2023-05-30  7:35       ` Petr Mladek
  0 siblings, 1 reply; 5+ messages in thread
From: Matthew Wilcox @ 2023-05-29 15:42 UTC (permalink / raw)
  To: Petr Mladek; +Cc: Hyeonggon Yoo, Andrew Morton, linux-mm

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Do not print page type when the page has no type
  2023-05-29 15:42     ` Matthew Wilcox
@ 2023-05-30  7:35       ` Petr Mladek
  0 siblings, 0 replies; 5+ messages in thread
From: Petr Mladek @ 2023-05-30  7:35 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Hyeonggon Yoo, Andrew Morton, linux-mm

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-05-30  7:36 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-27 16:48 [PATCH] Do not print page type when the page has no type Matthew Wilcox
2023-05-28  8:01 ` Hyeonggon Yoo
2023-05-29 15:27   ` Petr Mladek
2023-05-29 15:42     ` Matthew Wilcox
2023-05-30  7:35       ` Petr Mladek

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).