| Message ID | 20221218101901.373450-4-42.hyeyoo@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | move PG_slab flag to page_type | expand |
On Sun, Dec 18, 2022 at 07:19:00PM +0900, Hyeonggon Yoo wrote: > %pGp format is used to print 'flags' field of struct page. > As some page flags (e.g. PG_buddy, see page-flags.h for more details) > are set in page_type field, introduce %pGt format which provides > human readable output of page_type. > > Note that the sense of bits are different in page_type. if page_type is > 0xffffffff, no flags are set. if PG_slab (0x00100000) flag is set, > page_type is 0xffefffff. Clearing a bit means we set the bit. > > Bits in page_type are inverted when printing page type names. ... > +#define __def_pagetype_names \ > + {PG_slab, "slab" }, \ > + {PG_offline, "offline" }, \ > + {PG_guard, "guard" }, \ > + {PG_table, "table" }, \ > + {PG_buddy, "buddy" } Wondering if it will be more robust to define a helper macro #define DEF_PAGETYPE_NAME(_name) { PG_##_name, __stringify(_name) } ... #undef DEF_PAGETYPE_MASK In this case it decreases the chances of typo in the strings and flags.
On 12/19/22 01:44, Andy Shevchenko wrote: > On Sun, Dec 18, 2022 at 07:19:00PM +0900, Hyeonggon Yoo wrote: >> %pGp format is used to print 'flags' field of struct page. >> As some page flags (e.g. PG_buddy, see page-flags.h for more details) >> are set in page_type field, introduce %pGt format which provides >> human readable output of page_type. >> >> Note that the sense of bits are different in page_type. if page_type is >> 0xffffffff, no flags are set. if PG_slab (0x00100000) flag is set, >> page_type is 0xffefffff. Clearing a bit means we set the bit. >> >> Bits in page_type are inverted when printing page type names. > > ... > >> +#define __def_pagetype_names \ >> + {PG_slab, "slab" }, \ >> + {PG_offline, "offline" }, \ >> + {PG_guard, "guard" }, \ >> + {PG_table, "table" }, \ >> + {PG_buddy, "buddy" } > > Wondering if it will be more robust to define a helper macro > > #define DEF_PAGETYPE_NAME(_name) { PG_##_name, __stringify(_name) } > ... > #undef DEF_PAGETYPE_MASK > > In this case it decreases the chances of typo in the strings and flags. I'd say Yes. (and #undef DEF_PAGETYPE_NAME methinks; or change both to _MASK)
On Mon, Dec 19, 2022 at 11:35:38AM -0800, Randy Dunlap wrote: > On 12/19/22 01:44, Andy Shevchenko wrote: > > On Sun, Dec 18, 2022 at 07:19:00PM +0900, Hyeonggon Yoo wrote: ... > > #define DEF_PAGETYPE_NAME(_name) { PG_##_name, __stringify(_name) } > > ... > > #undef DEF_PAGETYPE_MASK > > > > In this case it decreases the chances of typo in the strings and flags. > > I'd say Yes. (and #undef DEF_PAGETYPE_NAME methinks; or change both to _MASK) Ah, it's me who used two different names for the same macro :-)
On Sun 2022-12-18 19:19:00, Hyeonggon Yoo wrote: > %pGp format is used to print 'flags' field of struct page. > As some page flags (e.g. PG_buddy, see page-flags.h for more details) > are set in page_type field, introduce %pGt format which provides > human readable output of page_type. > > Note that the sense of bits are different in page_type. if page_type is > 0xffffffff, no flags are set. if PG_slab (0x00100000) flag is set, > page_type is 0xffefffff. Clearing a bit means we set the bit. > > Bits in page_type are inverted when printing page type names. > > --- a/Documentation/core-api/printk-formats.rst > +++ b/Documentation/core-api/printk-formats.rst > @@ -575,12 +575,13 @@ The field width is passed by value, the bitmap is passed by reference. > Helper macros cpumask_pr_args() and nodemask_pr_args() are available to ease > printing cpumask and nodemask. > > -Flags bitfields such as page flags, gfp_flags > +Flags bitfields such as page flags, page_type, gfp_flags > --------------------------------------------- Please, underline the entire title. Otherwise, "make htmldoc" complains ;-) /prace/kernel/linux/Documentation/core-api/printk-formats.rst:579: WARNING: Title underline too short. Flags bitfields such as page flags, page_type, gfp_flags > > :: > > %pGp 0x17ffffc0002036(referenced|uptodate|lru|active|private|node=0|zone=2|lastcpupid=0x1fffff) > + %pGt 0xffefffff(slab) > %pGg GFP_USER|GFP_DMA32|GFP_NOWARN > %pGv read|exec|mayread|maywrite|mayexec|denywrite > Please, explain this also in the paragraph below these examples. I would personally refactor it to an itemized list, something like: <proposal> For printing flags bitfields as a collection of symbolic constants that would construct the value. The type of flags is given by the third character. Currently supported are: - p - [p]age flags, expects value of type (``unsigned long *``) - t - page [t]ype, expects value of type (``unsigned int *``) - v - [v]ma_flags, expects value of type (``unsigned long *``) - g - [g]fp_flags, expects value of type (``gfp_t *``) The flag names and print order depends on the particular type. </proposal> Rant: Sigh, it looks a bit error prone when similar pointer modifiers expects pointers to different types. I wish there was a way how to check the passed pointer type at compilation time. But it is generic problem with these %p* modifiers. Otherwise the patch looks fine for the vsprinf side. Best Regards, Petr
On Tue, Dec 20, 2022 at 04:20:26PM +0100, Petr Mladek wrote: > On Sun 2022-12-18 19:19:00, Hyeonggon Yoo wrote: > > %pGp format is used to print 'flags' field of struct page. > > As some page flags (e.g. PG_buddy, see page-flags.h for more details) > > are set in page_type field, introduce %pGt format which provides > > human readable output of page_type. > > > > Note that the sense of bits are different in page_type. if page_type is > > 0xffffffff, no flags are set. if PG_slab (0x00100000) flag is set, > > page_type is 0xffefffff. Clearing a bit means we set the bit. > > > > Bits in page_type are inverted when printing page type names. > > > > --- a/Documentation/core-api/printk-formats.rst > > +++ b/Documentation/core-api/printk-formats.rst > > @@ -575,12 +575,13 @@ The field width is passed by value, the bitmap is passed by reference. > > Helper macros cpumask_pr_args() and nodemask_pr_args() are available to ease > > printing cpumask and nodemask. > > > > -Flags bitfields such as page flags, gfp_flags > > +Flags bitfields such as page flags, page_type, gfp_flags > > --------------------------------------------- > > Please, underline the entire title. Otherwise, "make htmldoc" > complains ;-) > > /prace/kernel/linux/Documentation/core-api/printk-formats.rst:579: WARNING: Title underline too short. > Flags bitfields such as page flags, page_type, gfp_flags Still not getting used to rst format ;) Will fix, thanks! > > > > > > :: > > > > %pGp 0x17ffffc0002036(referenced|uptodate|lru|active|private|node=0|zone=2|lastcpupid=0x1fffff) > > + %pGt 0xffefffff(slab) > > %pGg GFP_USER|GFP_DMA32|GFP_NOWARN > > %pGv read|exec|mayread|maywrite|mayexec|denywrite > > > > Please, explain this also in the paragraph below these examples. > I would personally refactor it to an itemized list, something like: > > <proposal> > For printing flags bitfields as a collection of symbolic constants that > would construct the value. The type of flags is given by the third > character. Currently supported are: > > - p - [p]age flags, expects value of type (``unsigned long *``) > - t - page [t]ype, expects value of type (``unsigned int *``) > - v - [v]ma_flags, expects value of type (``unsigned long *``) > - g - [g]fp_flags, expects value of type (``gfp_t *``) > > The flag names and print order depends on the particular type. > </proposal> The proposal sounds reasonable to me, will adjust in next version. > Rant: > Sigh, it looks a bit error prone when similar pointer modifiers > expects pointers to different types. I wish there was a way how > to check the passed pointer type at compilation time. But it > is generic problem with these %p* modifiers. From my limited knowledge, it seems that there is no way to check this :/ > > Otherwise the patch looks fine for the vsprinf side. Thank you for looking at this! > > Best Regards, > Petr
On Tue, Dec 20, 2022 at 12:58:53PM +0200, Andy Shevchenko wrote: > On Mon, Dec 19, 2022 at 11:35:38AM -0800, Randy Dunlap wrote: > > On 12/19/22 01:44, Andy Shevchenko wrote: > > > On Sun, Dec 18, 2022 at 07:19:00PM +0900, Hyeonggon Yoo wrote: > > ... > > > > #define DEF_PAGETYPE_NAME(_name) { PG_##_name, __stringify(_name) } > > > ... > > > #undef DEF_PAGETYPE_MASK > > > > > > In this case it decreases the chances of typo in the strings and flags. > > > > I'd say Yes. (and #undef DEF_PAGETYPE_NAME methinks; or change both to _MASK) > > Ah, it's me who used two different names for the same macro :-) It seems like a good way to make fewer mistakes. Will do in next version, thanks! :-)
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index dbe1aacc79d0..582e965508eb 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -575,12 +575,13 @@ The field width is passed by value, the bitmap is passed by reference. Helper macros cpumask_pr_args() and nodemask_pr_args() are available to ease printing cpumask and nodemask. -Flags bitfields such as page flags, gfp_flags +Flags bitfields such as page flags, page_type, gfp_flags --------------------------------------------- :: %pGp 0x17ffffc0002036(referenced|uptodate|lru|active|private|node=0|zone=2|lastcpupid=0x1fffff) + %pGt 0xffefffff(slab) %pGg GFP_USER|GFP_DMA32|GFP_NOWARN %pGv read|exec|mayread|maywrite|mayexec|denywrite diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h index 8301912f8c25..57f52d00e761 100644 --- a/include/trace/events/mmflags.h +++ b/include/trace/events/mmflags.h @@ -138,6 +138,13 @@ IF_HAVE_PG_SKIP_KASAN_POISON(PG_skip_kasan_poison, "skip_kasan_poison") __def_pageflag_names \ ) : "none" +#define __def_pagetype_names \ + {PG_slab, "slab" }, \ + {PG_offline, "offline" }, \ + {PG_guard, "guard" }, \ + {PG_table, "table" }, \ + {PG_buddy, "buddy" } + #if defined(CONFIG_X86) #define __VM_ARCH_SPECIFIC_1 {VM_PAT, "pat" } #elif defined(CONFIG_PPC) diff --git a/lib/test_printf.c b/lib/test_printf.c index d34dc636b81c..e0d0770d5eec 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -642,12 +642,26 @@ page_flags_test(int section, int node, int zone, int last_cpupid, test(cmp_buf, "%pGp", &flags); } +static void __init page_type_test(unsigned int page_type, const char *name, + char *cmp_buf) +{ + 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); + + snprintf(cmp_buf + size, BUF_SIZE - size, ")"); + test(cmp_buf, "%pGt", &page_type); +} + static void __init flags(void) { unsigned long flags; char *cmp_buffer; gfp_t gfp; + unsigned int page_type; cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL); if (!cmp_buffer) @@ -687,6 +701,18 @@ flags(void) gfp |= __GFP_ATOMIC; test(cmp_buffer, "%pGg", &gfp); + page_type = ~0; + page_type_test(page_type, "", cmp_buffer); + + page_type = 10; + page_type_test(page_type, "", cmp_buffer); + + page_type = ~PG_slab; + page_type_test(page_type, "slab", cmp_buffer); + + page_type = ~(PG_slab | PG_table | PG_buddy); + page_type_test(page_type, "slab|table|buddy", cmp_buffer); + kfree(cmp_buffer); } diff --git a/lib/vsprintf.c b/lib/vsprintf.c index be71a03c936a..fbe320b5e89f 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -2052,6 +2052,25 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) return buf; } +static +char *format_page_type(char *buf, char *end, unsigned int page_type) +{ + buf = number(buf, end, page_type, default_flag_spec); + + if (buf < end) + *buf = '('; + buf++; + + if (page_type_has_type(page_type)) + buf = format_flags(buf, end, ~page_type, pagetype_names); + + if (buf < end) + *buf = ')'; + buf++; + + return buf; +} + static noinline_for_stack char *flags_string(char *buf, char *end, void *flags_ptr, struct printf_spec spec, const char *fmt) @@ -2065,6 +2084,8 @@ char *flags_string(char *buf, char *end, void *flags_ptr, switch (fmt[1]) { case 'p': return format_page_flags(buf, end, *(unsigned long *)flags_ptr); + case 't': + return format_page_type(buf, end, *(unsigned int *)flags_ptr); case 'v': flags = *(unsigned long *)flags_ptr; names = vmaflag_names; diff --git a/mm/debug.c b/mm/debug.c index 7f8e5f744e42..5ce6b359004a 100644 --- a/mm/debug.c +++ b/mm/debug.c @@ -36,6 +36,11 @@ const struct trace_print_flags pageflag_names[] = { {0, NULL} }; +const struct trace_print_flags pagetype_names[] = { + __def_pagetype_names, + {0, NULL} +}; + const struct trace_print_flags gfpflag_names[] = { __def_gfpflag_names, {0, NULL} diff --git a/mm/internal.h b/mm/internal.h index bcf75a8b032d..b4ba6fd6051c 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -773,6 +773,7 @@ static inline void flush_tlb_batched_pending(struct mm_struct *mm) #endif /* CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH */ extern const struct trace_print_flags pageflag_names[]; +extern const struct trace_print_flags pagetype_names[]; extern const struct trace_print_flags vmaflag_names[]; extern const struct trace_print_flags gfpflag_names[];
%pGp format is used to print 'flags' field of struct page. As some page flags (e.g. PG_buddy, see page-flags.h for more details) are set in page_type field, introduce %pGt format which provides human readable output of page_type. Note that the sense of bits are different in page_type. if page_type is 0xffffffff, no flags are set. if PG_slab (0x00100000) flag is set, page_type is 0xffefffff. Clearing a bit means we set the bit. Bits in page_type are inverted when printing page type names. Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- Documentation/core-api/printk-formats.rst | 3 ++- include/trace/events/mmflags.h | 7 ++++++ lib/test_printf.c | 26 +++++++++++++++++++++++ lib/vsprintf.c | 21 ++++++++++++++++++ mm/debug.c | 5 +++++ mm/internal.h | 1 + 6 files changed, 62 insertions(+), 1 deletion(-)