* [PATCH 0/5] Improvements to %pGp
@ 2021-10-12 18:26 Matthew Wilcox (Oracle)
2021-10-12 18:26 ` [PATCH 1/5] test_printf: Make pft array const Matthew Wilcox (Oracle)
` (4 more replies)
0 siblings, 5 replies; 26+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-10-12 18:26 UTC (permalink / raw)
To: Yafang Shao
Cc: Matthew Wilcox (Oracle),
Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka,
Rasmus Villemoes
The first four patches are improvements to page_flags_test().
The fifth is the patch I originally sent out which exposed some of
the weaknesses in page_flags_test().
Matthew Wilcox (Oracle) (5):
test_printf: Make pft array const
test_printf: Assign all flags to page_flags
test_printf: Remove custom appending of '|'
test_printf: Append '|' more efficiently
vsprintf: Make %pGp print the hex value
lib/test_printf.c | 48 +++++++++++++++++++++------------------------
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, 39 insertions(+), 35 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 1/5] test_printf: Make pft array const 2021-10-12 18:26 [PATCH 0/5] Improvements to %pGp Matthew Wilcox (Oracle) @ 2021-10-12 18:26 ` Matthew Wilcox (Oracle) 2021-10-13 5:20 ` Yafang Shao ` (2 more replies) 2021-10-12 18:26 ` [PATCH 2/5] test_printf: Assign all flags to page_flags Matthew Wilcox (Oracle) ` (3 subsequent siblings) 4 siblings, 3 replies; 26+ messages in thread From: Matthew Wilcox (Oracle) @ 2021-10-12 18:26 UTC (permalink / raw) To: Yafang Shao Cc: Matthew Wilcox (Oracle), Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka, Rasmus Villemoes Instead of assigning ptf[i].value, leave the values in the on-stack array and then we can make the array const. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- lib/test_printf.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/lib/test_printf.c b/lib/test_printf.c index 55082432f37e..a52c1c3a55ba 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -586,22 +586,21 @@ struct page_flags_test { int width; int shift; int mask; - unsigned long value; const char *fmt; const char *name; }; -static struct page_flags_test pft[] = { +static const struct page_flags_test pft[] = { {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, - 0, "%d", "section"}, + "%d", "section"}, {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, - 0, "%d", "node"}, + "%d", "node"}, {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, - 0, "%d", "zone"}, + "%d", "zone"}, {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, - 0, "%#x", "lastcpupid"}, + "%#x", "lastcpupid"}, {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, - 0, "%#x", "kasantag"}, + "%#x", "kasantag"}, }; static void __init @@ -627,10 +626,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid, #endif } - /* Set the test value */ - for (i = 0; i < ARRAY_SIZE(pft); i++) - pft[i].value = values[i]; - for (i = 0; i < ARRAY_SIZE(pft); i++) { if (!pft[i].width) continue; @@ -640,11 +635,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid, size = strlen(cmp_buf); } - page_flags |= (pft[i].value & pft[i].mask) << pft[i].shift; + 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, - pft[i].value & pft[i].mask); + values[i] & pft[i].mask); size = strlen(cmp_buf); append = true; } -- 2.32.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] test_printf: Make pft array const 2021-10-12 18:26 ` [PATCH 1/5] test_printf: Make pft array const Matthew Wilcox (Oracle) @ 2021-10-13 5:20 ` Yafang Shao 2021-10-15 9:21 ` Petr Mladek 2021-10-18 5:32 ` Anshuman Khandual 2 siblings, 0 replies; 26+ messages in thread From: Yafang Shao @ 2021-10-13 5:20 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Sergey Senozhatsky, Petr Mladek, Linux MM, Vlastimil Babka, Rasmus Villemoes On Wed, Oct 13, 2021 at 2:29 AM Matthew Wilcox (Oracle) <willy@infradead.org> wrote: > > Instead of assigning ptf[i].value, leave the values in the on-stack > array and then we can make the array const. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: Yafang Shao <laoar.shao@gmail.com> > --- > lib/test_printf.c | 21 ++++++++------------- > 1 file changed, 8 insertions(+), 13 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 55082432f37e..a52c1c3a55ba 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -586,22 +586,21 @@ struct page_flags_test { > int width; > int shift; > int mask; > - unsigned long value; > const char *fmt; > const char *name; > }; > > -static struct page_flags_test pft[] = { > +static const struct page_flags_test pft[] = { > {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, > - 0, "%d", "section"}, > + "%d", "section"}, > {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, > - 0, "%d", "node"}, > + "%d", "node"}, > {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, > - 0, "%d", "zone"}, > + "%d", "zone"}, > {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, > - 0, "%#x", "lastcpupid"}, > + "%#x", "lastcpupid"}, > {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, > - 0, "%#x", "kasantag"}, > + "%#x", "kasantag"}, > }; > > static void __init > @@ -627,10 +626,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > #endif > } > > - /* Set the test value */ > - for (i = 0; i < ARRAY_SIZE(pft); i++) > - pft[i].value = values[i]; > - > for (i = 0; i < ARRAY_SIZE(pft); i++) { > if (!pft[i].width) > continue; > @@ -640,11 +635,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > size = strlen(cmp_buf); > } > > - page_flags |= (pft[i].value & pft[i].mask) << pft[i].shift; > + 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, > - pft[i].value & pft[i].mask); > + values[i] & pft[i].mask); > size = strlen(cmp_buf); > append = true; > } > -- > 2.32.0 > -- Thanks Yafang ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] test_printf: Make pft array const 2021-10-12 18:26 ` [PATCH 1/5] test_printf: Make pft array const Matthew Wilcox (Oracle) 2021-10-13 5:20 ` Yafang Shao @ 2021-10-15 9:21 ` Petr Mladek 2021-10-18 5:32 ` Anshuman Khandual 2 siblings, 0 replies; 26+ messages in thread From: Petr Mladek @ 2021-10-15 9:21 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Yafang Shao, Sergey Senozhatsky, linux-mm, Vlastimil Babka, Rasmus Villemoes On Tue 2021-10-12 19:26:43, Matthew Wilcox (Oracle) wrote: > Instead of assigning ptf[i].value, leave the values in the on-stack > array and then we can make the array const. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] test_printf: Make pft array const 2021-10-12 18:26 ` [PATCH 1/5] test_printf: Make pft array const Matthew Wilcox (Oracle) 2021-10-13 5:20 ` Yafang Shao 2021-10-15 9:21 ` Petr Mladek @ 2021-10-18 5:32 ` Anshuman Khandual 2 siblings, 0 replies; 26+ messages in thread From: Anshuman Khandual @ 2021-10-18 5:32 UTC (permalink / raw) To: Matthew Wilcox (Oracle), Yafang Shao Cc: Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka, Rasmus Villemoes On 10/12/21 11:56 PM, Matthew Wilcox (Oracle) wrote: > Instead of assigning ptf[i].value, leave the values in the on-stack > array and then we can make the array const. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > lib/test_printf.c | 21 ++++++++------------- > 1 file changed, 8 insertions(+), 13 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 55082432f37e..a52c1c3a55ba 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -586,22 +586,21 @@ struct page_flags_test { > int width; > int shift; > int mask; > - unsigned long value; > const char *fmt; > const char *name; > }; > > -static struct page_flags_test pft[] = { > +static const struct page_flags_test pft[] = { > {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, > - 0, "%d", "section"}, > + "%d", "section"}, > {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, > - 0, "%d", "node"}, > + "%d", "node"}, > {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, > - 0, "%d", "zone"}, > + "%d", "zone"}, > {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, > - 0, "%#x", "lastcpupid"}, > + "%#x", "lastcpupid"}, > {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, > - 0, "%#x", "kasantag"}, > + "%#x", "kasantag"}, > }; > > static void __init > @@ -627,10 +626,6 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > #endif > } > > - /* Set the test value */ > - for (i = 0; i < ARRAY_SIZE(pft); i++) > - pft[i].value = values[i]; > - > for (i = 0; i < ARRAY_SIZE(pft); i++) { > if (!pft[i].width) > continue; > @@ -640,11 +635,11 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > size = strlen(cmp_buf); > } > > - page_flags |= (pft[i].value & pft[i].mask) << pft[i].shift; > + 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, > - pft[i].value & pft[i].mask); > + values[i] & pft[i].mask); > size = strlen(cmp_buf); > append = true; > } > Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com> ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/5] test_printf: Assign all flags to page_flags 2021-10-12 18:26 [PATCH 0/5] Improvements to %pGp Matthew Wilcox (Oracle) 2021-10-12 18:26 ` [PATCH 1/5] test_printf: Make pft array const Matthew Wilcox (Oracle) @ 2021-10-12 18:26 ` Matthew Wilcox (Oracle) 2021-10-13 5:20 ` Yafang Shao ` (2 more replies) 2021-10-12 18:26 ` [PATCH 3/5] test_printf: Remove custom appending of '|' Matthew Wilcox (Oracle) ` (2 subsequent siblings) 4 siblings, 3 replies; 26+ messages in thread From: Matthew Wilcox (Oracle) @ 2021-10-12 18:26 UTC (permalink / raw) To: Yafang Shao Cc: Matthew Wilcox (Oracle), Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka, Rasmus Villemoes Even unknown flags should be passed to %pGp so that we can test what it does with them (today: nothing). Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- lib/test_printf.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/test_printf.c b/lib/test_printf.c index a52c1c3a55ba..f744b0498672 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -608,14 +608,12 @@ 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 values[] = {section, node, zone, last_cpupid, kasan_tag}; - unsigned long page_flags = 0; + unsigned long page_flags = flags; unsigned long size = 0; bool append = false; int i; - flags &= PAGEFLAGS_MASK; - if (flags) { - page_flags |= flags; + if (flags & PAGEFLAGS_MASK) { snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); size = strlen(cmp_buf); #if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \ -- 2.32.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] test_printf: Assign all flags to page_flags 2021-10-12 18:26 ` [PATCH 2/5] test_printf: Assign all flags to page_flags Matthew Wilcox (Oracle) @ 2021-10-13 5:20 ` Yafang Shao 2021-10-13 9:25 ` Kirill A. Shutemov 2021-10-15 10:14 ` Petr Mladek 2 siblings, 0 replies; 26+ messages in thread From: Yafang Shao @ 2021-10-13 5:20 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Sergey Senozhatsky, Petr Mladek, Linux MM, Vlastimil Babka, Rasmus Villemoes On Wed, Oct 13, 2021 at 2:31 AM Matthew Wilcox (Oracle) <willy@infradead.org> wrote: > > Even unknown flags should be passed to %pGp so that we can test what it > does with them (today: nothing). > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: Yafang Shao <laoar.shao@gmail.com> > --- > lib/test_printf.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index a52c1c3a55ba..f744b0498672 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -608,14 +608,12 @@ 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 values[] = {section, node, zone, last_cpupid, kasan_tag}; > - unsigned long page_flags = 0; > + unsigned long page_flags = flags; > unsigned long size = 0; > bool append = false; > int i; > > - flags &= PAGEFLAGS_MASK; > - if (flags) { > - page_flags |= flags; > + if (flags & PAGEFLAGS_MASK) { > snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); > size = strlen(cmp_buf); > #if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \ > -- > 2.32.0 > -- Thanks Yafang ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] test_printf: Assign all flags to page_flags 2021-10-12 18:26 ` [PATCH 2/5] test_printf: Assign all flags to page_flags Matthew Wilcox (Oracle) 2021-10-13 5:20 ` Yafang Shao @ 2021-10-13 9:25 ` Kirill A. Shutemov 2021-10-15 10:22 ` Petr Mladek 2021-10-18 5:33 ` Anshuman Khandual 2021-10-15 10:14 ` Petr Mladek 2 siblings, 2 replies; 26+ messages in thread From: Kirill A. Shutemov @ 2021-10-13 9:25 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Yafang Shao, Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka, Rasmus Villemoes On Tue, Oct 12, 2021 at 07:26:44PM +0100, Matthew Wilcox (Oracle) wrote: > Even unknown flags should be passed to %pGp so that we can test what it > does with them (today: nothing). > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > lib/test_printf.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index a52c1c3a55ba..f744b0498672 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -608,14 +608,12 @@ 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 values[] = {section, node, zone, last_cpupid, kasan_tag}; > - unsigned long page_flags = 0; > + unsigned long page_flags = flags; > unsigned long size = 0; > bool append = false; > int i; > > - flags &= PAGEFLAGS_MASK; > - if (flags) { > - page_flags |= flags; > + if (flags & PAGEFLAGS_MASK) { > snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); > size = strlen(cmp_buf); > #if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \ Do we even need 'page_flags'? Can't we do everything on 'flags' directly? -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] test_printf: Assign all flags to page_flags 2021-10-13 9:25 ` Kirill A. Shutemov @ 2021-10-15 10:22 ` Petr Mladek 2021-10-18 5:33 ` Anshuman Khandual 1 sibling, 0 replies; 26+ messages in thread From: Petr Mladek @ 2021-10-15 10:22 UTC (permalink / raw) To: Kirill A. Shutemov Cc: Matthew Wilcox (Oracle), Yafang Shao, Sergey Senozhatsky, linux-mm, Vlastimil Babka, Rasmus Villemoes On Wed 2021-10-13 12:25:30, Kirill A. Shutemov wrote: > On Tue, Oct 12, 2021 at 07:26:44PM +0100, Matthew Wilcox (Oracle) wrote: > > Even unknown flags should be passed to %pGp so that we can test what it > > does with them (today: nothing). > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > --- > > lib/test_printf.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/lib/test_printf.c b/lib/test_printf.c > > index a52c1c3a55ba..f744b0498672 100644 > > --- a/lib/test_printf.c > > +++ b/lib/test_printf.c > > @@ -608,14 +608,12 @@ 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 values[] = {section, node, zone, last_cpupid, kasan_tag}; > > - unsigned long page_flags = 0; > > + unsigned long page_flags = flags; > > unsigned long size = 0; > > bool append = false; > > int i; > > > > - flags &= PAGEFLAGS_MASK; > > - if (flags) { > > - page_flags |= flags; > > + if (flags & PAGEFLAGS_MASK) { > > snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); > > size = strlen(cmp_buf); > > #if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \ > > Do we even need 'page_flags'? Can't we do everything on 'flags' directly? I have to say that the meaning of "flags" and "page_flags" variable is far from clear [*] It would be worth documentation how they are supposed to be used. I am going to comment more on this in the 5th patch. Best Regards, Petr [*] Shame on me. I did not pay much attention to the test code when reviewing the original code. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] test_printf: Assign all flags to page_flags 2021-10-13 9:25 ` Kirill A. Shutemov 2021-10-15 10:22 ` Petr Mladek @ 2021-10-18 5:33 ` Anshuman Khandual 1 sibling, 0 replies; 26+ messages in thread From: Anshuman Khandual @ 2021-10-18 5:33 UTC (permalink / raw) To: Kirill A. Shutemov, Matthew Wilcox (Oracle) Cc: Yafang Shao, Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka, Rasmus Villemoes On 10/13/21 2:55 PM, Kirill A. Shutemov wrote: > On Tue, Oct 12, 2021 at 07:26:44PM +0100, Matthew Wilcox (Oracle) wrote: >> Even unknown flags should be passed to %pGp so that we can test what it >> does with them (today: nothing). >> >> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> >> --- >> lib/test_printf.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/lib/test_printf.c b/lib/test_printf.c >> index a52c1c3a55ba..f744b0498672 100644 >> --- a/lib/test_printf.c >> +++ b/lib/test_printf.c >> @@ -608,14 +608,12 @@ 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 values[] = {section, node, zone, last_cpupid, kasan_tag}; >> - unsigned long page_flags = 0; >> + unsigned long page_flags = flags; >> unsigned long size = 0; >> bool append = false; >> int i; >> >> - flags &= PAGEFLAGS_MASK; >> - if (flags) { >> - page_flags |= flags; >> + if (flags & PAGEFLAGS_MASK) { >> snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); >> size = strlen(cmp_buf); >> #if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \ > > Do we even need 'page_flags'? Can't we do everything on 'flags' directly? Agreed, +1. Otherwise the change LGTM. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] test_printf: Assign all flags to page_flags 2021-10-12 18:26 ` [PATCH 2/5] test_printf: Assign all flags to page_flags Matthew Wilcox (Oracle) 2021-10-13 5:20 ` Yafang Shao 2021-10-13 9:25 ` Kirill A. Shutemov @ 2021-10-15 10:14 ` Petr Mladek 2 siblings, 0 replies; 26+ messages in thread From: Petr Mladek @ 2021-10-15 10:14 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Yafang Shao, Sergey Senozhatsky, linux-mm, Vlastimil Babka, Rasmus Villemoes On Tue 2021-10-12 19:26:44, Matthew Wilcox (Oracle) wrote: > Even unknown flags should be passed to %pGp so that we can test what it > does with them (today: nothing). > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Makes sense. Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/5] test_printf: Remove custom appending of '|' 2021-10-12 18:26 [PATCH 0/5] Improvements to %pGp Matthew Wilcox (Oracle) 2021-10-12 18:26 ` [PATCH 1/5] test_printf: Make pft array const Matthew Wilcox (Oracle) 2021-10-12 18:26 ` [PATCH 2/5] test_printf: Assign all flags to page_flags Matthew Wilcox (Oracle) @ 2021-10-12 18:26 ` Matthew Wilcox (Oracle) 2021-10-13 5:21 ` Yafang Shao 2021-10-18 5:41 ` Anshuman Khandual 2021-10-12 18:26 ` [PATCH 4/5] test_printf: Append '|' more efficiently Matthew Wilcox (Oracle) 2021-10-12 18:26 ` [PATCH 5/5] vsprintf: Make %pGp print the hex value Matthew Wilcox (Oracle) 4 siblings, 2 replies; 26+ messages in thread From: Matthew Wilcox (Oracle) @ 2021-10-12 18:26 UTC (permalink / raw) To: Yafang Shao Cc: Matthew Wilcox (Oracle), Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka, Rasmus Villemoes Instead of having an ifdef to decide whether to print a |, use the 'append' functionality of the main loop to print it. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- lib/test_printf.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/test_printf.c b/lib/test_printf.c index f744b0498672..60cdf4ba991e 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -616,12 +616,7 @@ page_flags_test(int section, int node, int zone, int last_cpupid, if (flags & PAGEFLAGS_MASK) { snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); size = strlen(cmp_buf); -#if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \ - LAST_CPUPID_WIDTH || KASAN_TAG_WIDTH - /* Other information also included in page flags */ - snprintf(cmp_buf + size, BUF_SIZE - size, "|"); - size = strlen(cmp_buf); -#endif + append = true; } for (i = 0; i < ARRAY_SIZE(pft); i++) { -- 2.32.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] test_printf: Remove custom appending of '|' 2021-10-12 18:26 ` [PATCH 3/5] test_printf: Remove custom appending of '|' Matthew Wilcox (Oracle) @ 2021-10-13 5:21 ` Yafang Shao 2021-10-15 10:23 ` Petr Mladek 2021-10-18 5:41 ` Anshuman Khandual 1 sibling, 1 reply; 26+ messages in thread From: Yafang Shao @ 2021-10-13 5:21 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Sergey Senozhatsky, Petr Mladek, Linux MM, Vlastimil Babka, Rasmus Villemoes On Wed, Oct 13, 2021 at 2:32 AM Matthew Wilcox (Oracle) <willy@infradead.org> wrote: > > Instead of having an ifdef to decide whether to print a |, use the > 'append' functionality of the main loop to print it. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: Yafang Shao <laoar.shao@gmail.com> > --- > lib/test_printf.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index f744b0498672..60cdf4ba991e 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -616,12 +616,7 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > if (flags & PAGEFLAGS_MASK) { > snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); > size = strlen(cmp_buf); > -#if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \ > - LAST_CPUPID_WIDTH || KASAN_TAG_WIDTH > - /* Other information also included in page flags */ > - snprintf(cmp_buf + size, BUF_SIZE - size, "|"); > - size = strlen(cmp_buf); > -#endif > + append = true; > } > > for (i = 0; i < ARRAY_SIZE(pft); i++) { > -- > 2.32.0 > -- Thanks Yafang ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] test_printf: Remove custom appending of '|' 2021-10-13 5:21 ` Yafang Shao @ 2021-10-15 10:23 ` Petr Mladek 0 siblings, 0 replies; 26+ messages in thread From: Petr Mladek @ 2021-10-15 10:23 UTC (permalink / raw) To: Yafang Shao Cc: Matthew Wilcox (Oracle), Sergey Senozhatsky, Linux MM, Vlastimil Babka, Rasmus Villemoes On Wed 2021-10-13 13:21:15, Yafang Shao wrote: > On Wed, Oct 13, 2021 at 2:32 AM Matthew Wilcox (Oracle) > <willy@infradead.org> wrote: > > > > Instead of having an ifdef to decide whether to print a |, use the > > 'append' functionality of the main loop to print it. > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > Reviewed-by: Yafang Shao <laoar.shao@gmail.com> Makes sense. Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] test_printf: Remove custom appending of '|' 2021-10-12 18:26 ` [PATCH 3/5] test_printf: Remove custom appending of '|' Matthew Wilcox (Oracle) 2021-10-13 5:21 ` Yafang Shao @ 2021-10-18 5:41 ` Anshuman Khandual 1 sibling, 0 replies; 26+ messages in thread From: Anshuman Khandual @ 2021-10-18 5:41 UTC (permalink / raw) To: Matthew Wilcox (Oracle), Yafang Shao Cc: Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka, Rasmus Villemoes On 10/12/21 11:56 PM, Matthew Wilcox (Oracle) wrote: > Instead of having an ifdef to decide whether to print a |, use the > 'append' functionality of the main loop to print it. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > lib/test_printf.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index f744b0498672..60cdf4ba991e 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -616,12 +616,7 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > if (flags & PAGEFLAGS_MASK) { > snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); > size = strlen(cmp_buf); > -#if SECTIONS_WIDTH || NODES_WIDTH || ZONES_WIDTH || \ > - LAST_CPUPID_WIDTH || KASAN_TAG_WIDTH > - /* Other information also included in page flags */ > - snprintf(cmp_buf + size, BUF_SIZE - size, "|"); > - size = strlen(cmp_buf); > -#endif > + append = true; > } > > for (i = 0; i < ARRAY_SIZE(pft); i++) { > Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com> ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/5] test_printf: Append '|' more efficiently 2021-10-12 18:26 [PATCH 0/5] Improvements to %pGp Matthew Wilcox (Oracle) ` (2 preceding siblings ...) 2021-10-12 18:26 ` [PATCH 3/5] test_printf: Remove custom appending of '|' Matthew Wilcox (Oracle) @ 2021-10-12 18:26 ` Matthew Wilcox (Oracle) 2021-10-13 5:22 ` Yafang Shao 2021-10-12 18:26 ` [PATCH 5/5] vsprintf: Make %pGp print the hex value Matthew Wilcox (Oracle) 4 siblings, 1 reply; 26+ messages in thread From: Matthew Wilcox (Oracle) @ 2021-10-12 18:26 UTC (permalink / raw) To: Yafang Shao Cc: Matthew Wilcox (Oracle), Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka, Rasmus Villemoes Instead of calling snprintf(), just append '|' by hand. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- lib/test_printf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/test_printf.c b/lib/test_printf.c index 60cdf4ba991e..662c3785aa57 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid, if (!pft[i].width) continue; - if (append) { - snprintf(cmp_buf + size, BUF_SIZE - size, "|"); - size = strlen(cmp_buf); + if (append && size < BUF_SIZE) { + cmp_buf[size++] = '|'; + cmp_buf[size] = '\0'; } page_flags |= (values[i] & pft[i].mask) << pft[i].shift; -- 2.32.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 4/5] test_printf: Append '|' more efficiently 2021-10-12 18:26 ` [PATCH 4/5] test_printf: Append '|' more efficiently Matthew Wilcox (Oracle) @ 2021-10-13 5:22 ` Yafang Shao 2021-10-13 9:27 ` Kirill A. Shutemov 2021-10-13 9:59 ` Rasmus Villemoes 0 siblings, 2 replies; 26+ messages in thread From: Yafang Shao @ 2021-10-13 5:22 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Sergey Senozhatsky, Petr Mladek, Linux MM, Vlastimil Babka, Rasmus Villemoes On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle) <willy@infradead.org> wrote: > > Instead of calling snprintf(), just append '|' by hand. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > lib/test_printf.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 60cdf4ba991e..662c3785aa57 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > if (!pft[i].width) > continue; > > - if (append) { > - snprintf(cmp_buf + size, BUF_SIZE - size, "|"); > - size = strlen(cmp_buf); > + if (append && size < BUF_SIZE) { Should it be: if (append && size < BUF_SIZE - 1) ? > + cmp_buf[size++] = '|'; > + cmp_buf[size] = '\0'; > } > > page_flags |= (values[i] & pft[i].mask) << pft[i].shift; > -- > 2.32.0 > -- Thanks Yafang ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/5] test_printf: Append '|' more efficiently 2021-10-13 5:22 ` Yafang Shao @ 2021-10-13 9:27 ` Kirill A. Shutemov 2021-10-18 5:42 ` Anshuman Khandual 2021-10-13 9:59 ` Rasmus Villemoes 1 sibling, 1 reply; 26+ messages in thread From: Kirill A. Shutemov @ 2021-10-13 9:27 UTC (permalink / raw) To: Yafang Shao Cc: Matthew Wilcox (Oracle), Sergey Senozhatsky, Petr Mladek, Linux MM, Vlastimil Babka, Rasmus Villemoes On Wed, Oct 13, 2021 at 01:22:10PM +0800, Yafang Shao wrote: > On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle) > <willy@infradead.org> wrote: > > > > Instead of calling snprintf(), just append '|' by hand. > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > --- > > lib/test_printf.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/lib/test_printf.c b/lib/test_printf.c > > index 60cdf4ba991e..662c3785aa57 100644 > > --- a/lib/test_printf.c > > +++ b/lib/test_printf.c > > @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > > if (!pft[i].width) > > continue; > > > > - if (append) { > > - snprintf(cmp_buf + size, BUF_SIZE - size, "|"); > > - size = strlen(cmp_buf); > > + if (append && size < BUF_SIZE) { > > Should it be: > if (append && size < BUF_SIZE - 1) > ? Yeah, looks like off-by-one to me. -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/5] test_printf: Append '|' more efficiently 2021-10-13 9:27 ` Kirill A. Shutemov @ 2021-10-18 5:42 ` Anshuman Khandual 0 siblings, 0 replies; 26+ messages in thread From: Anshuman Khandual @ 2021-10-18 5:42 UTC (permalink / raw) To: Kirill A. Shutemov, Yafang Shao Cc: Matthew Wilcox (Oracle), Sergey Senozhatsky, Petr Mladek, Linux MM, Vlastimil Babka, Rasmus Villemoes On 10/13/21 2:57 PM, Kirill A. Shutemov wrote: > On Wed, Oct 13, 2021 at 01:22:10PM +0800, Yafang Shao wrote: >> On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle) >> <willy@infradead.org> wrote: >>> >>> Instead of calling snprintf(), just append '|' by hand. >>> >>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> >>> --- >>> lib/test_printf.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/lib/test_printf.c b/lib/test_printf.c >>> index 60cdf4ba991e..662c3785aa57 100644 >>> --- a/lib/test_printf.c >>> +++ b/lib/test_printf.c >>> @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid, >>> if (!pft[i].width) >>> continue; >>> >>> - if (append) { >>> - snprintf(cmp_buf + size, BUF_SIZE - size, "|"); >>> - size = strlen(cmp_buf); >>> + if (append && size < BUF_SIZE) { >> >> Should it be: >> if (append && size < BUF_SIZE - 1) >> ? > > Yeah, looks like off-by-one to me. Agreed. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/5] test_printf: Append '|' more efficiently 2021-10-13 5:22 ` Yafang Shao 2021-10-13 9:27 ` Kirill A. Shutemov @ 2021-10-13 9:59 ` Rasmus Villemoes 2021-10-15 10:37 ` Petr Mladek 1 sibling, 1 reply; 26+ messages in thread From: Rasmus Villemoes @ 2021-10-13 9:59 UTC (permalink / raw) To: Yafang Shao, Matthew Wilcox (Oracle) Cc: Sergey Senozhatsky, Petr Mladek, Linux MM, Vlastimil Babka On 13/10/2021 07.22, Yafang Shao wrote: > On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle) > <willy@infradead.org> wrote: >> >> Instead of calling snprintf(), just append '|' by hand. >> >> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> >> --- >> lib/test_printf.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/lib/test_printf.c b/lib/test_printf.c >> index 60cdf4ba991e..662c3785aa57 100644 >> --- a/lib/test_printf.c >> +++ b/lib/test_printf.c >> @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid, >> if (!pft[i].width) >> continue; >> >> - if (append) { >> - snprintf(cmp_buf + size, BUF_SIZE - size, "|"); >> - size = strlen(cmp_buf); >> + if (append && size < BUF_SIZE) { > > Should it be: > if (append && size < BUF_SIZE - 1) > ? > Perhaps, but the whole thing screams "don't do it like this". We have seq_buf to abstract a "buffer with known length you can do a bunch of snprintfs to". That's what should be used. Then the test can also error out if seq_buf_has_overflowed(), because that's a bug in the test. Alternatively, the right pattern is "size += scnprintf(cmp_buf + size, BUF_SIZE - size)", since that will eventually saturate size at BUF_SIZE-1. Rasmus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/5] test_printf: Append '|' more efficiently 2021-10-13 9:59 ` Rasmus Villemoes @ 2021-10-15 10:37 ` Petr Mladek 0 siblings, 0 replies; 26+ messages in thread From: Petr Mladek @ 2021-10-15 10:37 UTC (permalink / raw) To: Rasmus Villemoes Cc: Yafang Shao, Matthew Wilcox (Oracle), Sergey Senozhatsky, Linux MM, Vlastimil Babka On Wed 2021-10-13 11:59:38, Rasmus Villemoes wrote: > On 13/10/2021 07.22, Yafang Shao wrote: > > On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle) > > <willy@infradead.org> wrote: > >> > >> Instead of calling snprintf(), just append '|' by hand. > >> > >> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > >> --- > >> lib/test_printf.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/lib/test_printf.c b/lib/test_printf.c > >> index 60cdf4ba991e..662c3785aa57 100644 > >> --- a/lib/test_printf.c > >> +++ b/lib/test_printf.c > >> @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > >> if (!pft[i].width) > >> continue; > >> > >> - if (append) { > >> - snprintf(cmp_buf + size, BUF_SIZE - size, "|"); > >> - size = strlen(cmp_buf); > >> + if (append && size < BUF_SIZE) { > > > > Should it be: > > if (append && size < BUF_SIZE - 1) > > Perhaps, but the whole thing screams "don't do it like this". We have > seq_buf to abstract a "buffer with known length you can do a bunch of > snprintfs to". That's what should be used. Then the test can also error > out if seq_buf_has_overflowed(), because that's a bug in the test. Interesting idea. > Alternatively, the right pattern is "size += scnprintf(cmp_buf + size, > BUF_SIZE - size)", since that will eventually saturate size at BUF_SIZE-1. Yup, this is well known pattern so that it is much easier to make it correctly and review. The performance is not important here. Best Regards, Petr ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/5] vsprintf: Make %pGp print the hex value 2021-10-12 18:26 [PATCH 0/5] Improvements to %pGp Matthew Wilcox (Oracle) ` (3 preceding siblings ...) 2021-10-12 18:26 ` [PATCH 4/5] test_printf: Append '|' more efficiently Matthew Wilcox (Oracle) @ 2021-10-12 18:26 ` Matthew Wilcox (Oracle) 2021-10-13 5:24 ` Yafang Shao ` (2 more replies) 4 siblings, 3 replies; 26+ messages in thread From: Matthew Wilcox (Oracle) @ 2021-10-12 18:26 UTC (permalink / raw) To: Yafang Shao Cc: Matthew Wilcox (Oracle), Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka, Rasmus Villemoes 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) { + 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] vsprintf: Make %pGp print the hex value 2021-10-12 18:26 ` [PATCH 5/5] vsprintf: Make %pGp print the hex value Matthew Wilcox (Oracle) @ 2021-10-13 5:24 ` Yafang Shao 2021-10-15 10:55 ` Petr Mladek 2021-10-18 6:13 ` Anshuman Khandual 2 siblings, 0 replies; 26+ messages in thread From: Yafang Shao @ 2021-10-13 5:24 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Sergey Senozhatsky, Petr Mladek, Linux MM, Vlastimil Babka, Rasmus Villemoes 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 > -- Thanks Yafang ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] vsprintf: Make %pGp print the hex value 2021-10-12 18:26 ` [PATCH 5/5] vsprintf: Make %pGp print the hex value Matthew Wilcox (Oracle) 2021-10-13 5:24 ` Yafang Shao @ 2021-10-15 10:55 ` Petr Mladek 2021-10-27 11:28 ` Petr Mladek 2021-10-18 6:13 ` Anshuman Khandual 2 siblings, 1 reply; 26+ messages in thread From: Petr Mladek @ 2021-10-15 10:55 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Yafang Shao, Sergey Senozhatsky, linux-mm, Vlastimil Babka, Rasmus Villemoes 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] vsprintf: Make %pGp print the hex value 2021-10-15 10:55 ` Petr Mladek @ 2021-10-27 11:28 ` Petr Mladek 0 siblings, 0 replies; 26+ messages in thread From: Petr Mladek @ 2021-10-27 11:28 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Yafang Shao, Sergey Senozhatsky, linux-mm, Vlastimil Babka, Rasmus Villemoes 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] vsprintf: Make %pGp print the hex value 2021-10-12 18:26 ` [PATCH 5/5] vsprintf: Make %pGp print the hex value Matthew Wilcox (Oracle) 2021-10-13 5:24 ` Yafang Shao 2021-10-15 10:55 ` Petr Mladek @ 2021-10-18 6:13 ` Anshuman Khandual 2 siblings, 0 replies; 26+ messages in thread From: Anshuman Khandual @ 2021-10-18 6:13 UTC (permalink / raw) To: Matthew Wilcox (Oracle), Yafang Shao Cc: Sergey Senozhatsky, Petr Mladek, linux-mm, Vlastimil Babka, Rasmus Villemoes 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-10-27 11:28 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-12 18:26 [PATCH 0/5] Improvements to %pGp Matthew Wilcox (Oracle) 2021-10-12 18:26 ` [PATCH 1/5] test_printf: Make pft array const Matthew Wilcox (Oracle) 2021-10-13 5:20 ` Yafang Shao 2021-10-15 9:21 ` Petr Mladek 2021-10-18 5:32 ` Anshuman Khandual 2021-10-12 18:26 ` [PATCH 2/5] test_printf: Assign all flags to page_flags Matthew Wilcox (Oracle) 2021-10-13 5:20 ` Yafang Shao 2021-10-13 9:25 ` Kirill A. Shutemov 2021-10-15 10:22 ` Petr Mladek 2021-10-18 5:33 ` Anshuman Khandual 2021-10-15 10:14 ` Petr Mladek 2021-10-12 18:26 ` [PATCH 3/5] test_printf: Remove custom appending of '|' Matthew Wilcox (Oracle) 2021-10-13 5:21 ` Yafang Shao 2021-10-15 10:23 ` Petr Mladek 2021-10-18 5:41 ` Anshuman Khandual 2021-10-12 18:26 ` [PATCH 4/5] test_printf: Append '|' more efficiently Matthew Wilcox (Oracle) 2021-10-13 5:22 ` Yafang Shao 2021-10-13 9:27 ` Kirill A. Shutemov 2021-10-18 5:42 ` Anshuman Khandual 2021-10-13 9:59 ` Rasmus Villemoes 2021-10-15 10:37 ` Petr Mladek 2021-10-12 18:26 ` [PATCH 5/5] vsprintf: Make %pGp print the hex value Matthew Wilcox (Oracle) 2021-10-13 5:24 ` Yafang Shao 2021-10-15 10:55 ` Petr Mladek 2021-10-27 11:28 ` Petr Mladek 2021-10-18 6:13 ` Anshuman Khandual
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.