Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp
On Wed, Feb 10, 2021 at 8:51 PM Petr Mladek wrote:
>
> On Wed 2021-02-10 00:21:37, Yafang Shao wrote:
> > On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek wrote:
> > >
> > > On Tue 2021-02-09 18:56:13, Yafang Shao wrote:
> > > > Currently the pGp only shows the names of page flags, rather than
> > > > the full information including section, node, zone, last cpupid and
> > > > kasan tag. While it is not easy to parse these information manually
> > > > because there're so many flavors. Let's interpret them in pGp as well.
> > > >
> > > > To be compitable with the existed format of pGp, the new introduced ones
> > > > also use '|' as the separator, then the user tools parsing pGp won't
> > > > need to make change, suggested by Matthew. The new information is
> > > > tracked onto the end of the existed one.
> > > >
> > > > On example of the output in mm/slub.c as follows,
> > > > - Before the patch,
> > > > [ 6343.396602] Slab 0x4382e02b objects=33 used=3
> > > > fp=0x9ae06ffc flags=0x17c0010200(slab|head)
> > > >
> > > > - After the patch,
> > > > [ 8838.835456] Slab 0x2828b78a objects=33 used=3
> > > > fp=0xd04efc88
> > > > flags=0x17c0010200(slab|head|node=0|zone=2|lastcpupid=0x1f)
> > > >
> > > > The documentation and test cases are also updated. The output of the
> > > > test cases as follows,
> > > > [ 501.485081] test_printf: loaded.
> > > > [ 501.485768] test_printf: all 388 tests passed
> > > > [ 501.488762] test_printf: unloaded.
> > > >
> > >
> > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > > index 14c9a6af1b23..3f26611adb34 100644
> > > > --- a/lib/vsprintf.c
> > > > +++ b/lib/vsprintf.c
> > > > @@ -1916,6 +1916,66 @@ char *format_flags(char *buf, char *end,
> > > > unsigned long flags,
> > > > return buf;
> > > > }
> > > >
> > > > +struct page_flags_layout {
> > > > + int width;
> > > > + int shift;
> > > > + int mask;
> > > > + const struct printf_spec *spec;
> > > > + const char *name;
> > > > +};
> > > > +
> > > > +static const struct page_flags_layout pfl[] = {
> > > > + {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
> > > > + _dec_spec, "section"},
> > > > + {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
> > > > + _dec_spec, "node"},
> > > > + {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
> > > > + _dec_spec, "zone"},
> > > > + {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
> > > > + _flag_spec, "lastcpupid"},
> > > > + {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
> > > > + _flag_spec, "kasantag"},
> > > > +};
> > > > +
> > > > +static
> > > > +char *format_page_flags(char *buf, char *end, unsigned long flags)
> > > > +{
> > > > + DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> > > > + unsigned long last;
> > > > + int i;
> > > > +
> > > > + if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> > > > + if (buf < end)
> > > > + *buf = '|';
> > > > + buf++;
> > > > + }
> > >
> > > This is far from obvious. You print '|' here because you printed
> > > something somewhere else. See below.
> > >
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(pfl); i++)
> > > > + __assign_bit(i, mask, pfl[i].width);
> > >
> > > The bitmap looks like an overkill. If I get it correctly, it is a
> > > tricky way to handle only flags defined by the used build
> > > configuration. See below.
> > >
> > > > + last = find_last_bit(mask, ARRAY_SIZE(pfl));
> > > > +
> > > > + for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> > > > + /* Format: Flag Name + '=' (equals sign) + Number + '|'
> > > > (separator) */
> > > > + buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > > > +
> > > > + if (buf < end)
> > > > + *buf = '=';
> > > > + buf++;
> > > > + buf = number(buf, end, (flags >> pfl[i].shift) &
> > > > pfl[i].mask,
> > > > + *pfl[i].spec);
> > > > +
> > > > + /* No separator for the last entry */
> > > > + if (i != last) {
> > > > + 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)
> > > > @@ -1929,10 +1989,10 @@ char *flags_string(char *buf, char *end, void
> > > > *flags_ptr,
> > > > switch (fmt[1]) {
> > > > case 'p':
> > > > flags = *(unsigned long *)flags_ptr;
> > > > - /* Remove zone id */
> > > > - flags &= (1UL << NR_PAGEFLAGS) - 1;
> > > > names = pageflag_names;
> > >
> > > The "names" variable is needed only with "break;" when using the final
> > > format_flags(buf, end, flags,
Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp
On Wed, 2021-02-10 at 13:51 +0100, Petr Mladek wrote:
> On Wed 2021-02-10 00:21:37, Yafang Shao wrote:
> > On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek wrote:
[]
> > for (p = pff; p < pff + ARRAY_SIZE(pff); p++) {
>
> This looks a bit non-standard. IMHO, Joe was not against using index.
> He proposed:
>
> for (i = 0; i < ARRAY_SIZE(pfl) && buf < end; i++) {
>
> , see
> https://lore.kernel.org/lkml/e5ea9e8b1190c2a397a1b84dd55bb9c706dc7058.ca...@perches.com/
>
> I am not sure about the (buf < end) check. It might be some
> optimization or it did fit the the old code.
I believe the buf < end bit was broken anyway.
I believe vsprintf is supposed to return the maximum possible length
of the output and the function should not restrict that. The
function should not write beyond the specified end.
> Anyway, I like the currently used:
>
> for (i = 0; i < ARRAY_SIZE(pff); i++) {
>
> It is standard, easy to understand, and thus more safe. I am sure that
> compiler will optimize it very well.
true.
Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp
On Wed 2021-02-10 00:21:37, Yafang Shao wrote:
> On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek wrote:
> >
> > On Tue 2021-02-09 18:56:13, Yafang Shao wrote:
> > > Currently the pGp only shows the names of page flags, rather than
> > > the full information including section, node, zone, last cpupid and
> > > kasan tag. While it is not easy to parse these information manually
> > > because there're so many flavors. Let's interpret them in pGp as well.
> > >
> > > To be compitable with the existed format of pGp, the new introduced ones
> > > also use '|' as the separator, then the user tools parsing pGp won't
> > > need to make change, suggested by Matthew. The new information is
> > > tracked onto the end of the existed one.
> > >
> > > On example of the output in mm/slub.c as follows,
> > > - Before the patch,
> > > [ 6343.396602] Slab 0x4382e02b objects=33 used=3
> > > fp=0x9ae06ffc flags=0x17c0010200(slab|head)
> > >
> > > - After the patch,
> > > [ 8838.835456] Slab 0x2828b78a objects=33 used=3
> > > fp=0xd04efc88
> > > flags=0x17c0010200(slab|head|node=0|zone=2|lastcpupid=0x1f)
> > >
> > > The documentation and test cases are also updated. The output of the
> > > test cases as follows,
> > > [ 501.485081] test_printf: loaded.
> > > [ 501.485768] test_printf: all 388 tests passed
> > > [ 501.488762] test_printf: unloaded.
> > >
> >
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index 14c9a6af1b23..3f26611adb34 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -1916,6 +1916,66 @@ char *format_flags(char *buf, char *end, unsigned
> > > long flags,
> > > return buf;
> > > }
> > >
> > > +struct page_flags_layout {
> > > + int width;
> > > + int shift;
> > > + int mask;
> > > + const struct printf_spec *spec;
> > > + const char *name;
> > > +};
> > > +
> > > +static const struct page_flags_layout pfl[] = {
> > > + {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
> > > + _dec_spec, "section"},
> > > + {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
> > > + _dec_spec, "node"},
> > > + {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
> > > + _dec_spec, "zone"},
> > > + {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
> > > + _flag_spec, "lastcpupid"},
> > > + {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
> > > + _flag_spec, "kasantag"},
> > > +};
> > > +
> > > +static
> > > +char *format_page_flags(char *buf, char *end, unsigned long flags)
> > > +{
> > > + DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> > > + unsigned long last;
> > > + int i;
> > > +
> > > + if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> > > + if (buf < end)
> > > + *buf = '|';
> > > + buf++;
> > > + }
> >
> > This is far from obvious. You print '|' here because you printed
> > something somewhere else. See below.
> >
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(pfl); i++)
> > > + __assign_bit(i, mask, pfl[i].width);
> >
> > The bitmap looks like an overkill. If I get it correctly, it is a
> > tricky way to handle only flags defined by the used build
> > configuration. See below.
> >
> > > + last = find_last_bit(mask, ARRAY_SIZE(pfl));
> > > +
> > > + for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> > > + /* Format: Flag Name + '=' (equals sign) + Number + '|'
> > > (separator) */
> > > + buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > > +
> > > + if (buf < end)
> > > + *buf = '=';
> > > + buf++;
> > > + buf = number(buf, end, (flags >> pfl[i].shift) &
> > > pfl[i].mask,
> > > + *pfl[i].spec);
> > > +
> > > + /* No separator for the last entry */
> > > + if (i != last) {
> > > + 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)
> > > @@ -1929,10 +1989,10 @@ char *flags_string(char *buf, char *end, void
> > > *flags_ptr,
> > > switch (fmt[1]) {
> > > case 'p':
> > > flags = *(unsigned long *)flags_ptr;
> > > - /* Remove zone id */
> > > - flags &= (1UL << NR_PAGEFLAGS) - 1;
> > > names = pageflag_names;
> >
> > The "names" variable is needed only with "break;" when using the final
> > format_flags(buf, end, flags, names);
> >
> > > - break;
> > > + buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) -
> > > 1), names);
> > > + buf = format_page_flags(buf, end, flags);
> >
> > I am sorry for my ignorance. I am not familiar with MM.
> > But it is pretty hard to understand
Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp
On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek wrote:
>
> On Tue 2021-02-09 18:56:13, Yafang Shao wrote:
> > Currently the pGp only shows the names of page flags, rather than
> > the full information including section, node, zone, last cpupid and
> > kasan tag. While it is not easy to parse these information manually
> > because there're so many flavors. Let's interpret them in pGp as well.
> >
> > To be compitable with the existed format of pGp, the new introduced ones
> > also use '|' as the separator, then the user tools parsing pGp won't
> > need to make change, suggested by Matthew. The new information is
> > tracked onto the end of the existed one.
> >
> > On example of the output in mm/slub.c as follows,
> > - Before the patch,
> > [ 6343.396602] Slab 0x4382e02b objects=33 used=3
> > fp=0x9ae06ffc flags=0x17c0010200(slab|head)
> >
> > - After the patch,
> > [ 8838.835456] Slab 0x2828b78a objects=33 used=3
> > fp=0xd04efc88
> > flags=0x17c0010200(slab|head|node=0|zone=2|lastcpupid=0x1f)
> >
> > The documentation and test cases are also updated. The output of the
> > test cases as follows,
> > [ 501.485081] test_printf: loaded.
> > [ 501.485768] test_printf: all 388 tests passed
> > [ 501.488762] test_printf: unloaded.
> >
>
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 14c9a6af1b23..3f26611adb34 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1916,6 +1916,66 @@ char *format_flags(char *buf, char *end, unsigned
> > long flags,
> > return buf;
> > }
> >
> > +struct page_flags_layout {
> > + int width;
> > + int shift;
> > + int mask;
> > + const struct printf_spec *spec;
> > + const char *name;
> > +};
> > +
> > +static const struct page_flags_layout pfl[] = {
> > + {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
> > + _dec_spec, "section"},
> > + {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
> > + _dec_spec, "node"},
> > + {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
> > + _dec_spec, "zone"},
> > + {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
> > + _flag_spec, "lastcpupid"},
> > + {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
> > + _flag_spec, "kasantag"},
> > +};
> > +
> > +static
> > +char *format_page_flags(char *buf, char *end, unsigned long flags)
> > +{
> > + DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> > + unsigned long last;
> > + int i;
> > +
> > + if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> > + if (buf < end)
> > + *buf = '|';
> > + buf++;
> > + }
>
> This is far from obvious. You print '|' here because you printed
> something somewhere else. See below.
>
> > +
> > + for (i = 0; i < ARRAY_SIZE(pfl); i++)
> > + __assign_bit(i, mask, pfl[i].width);
>
> The bitmap looks like an overkill. If I get it correctly, it is a
> tricky way to handle only flags defined by the used build
> configuration. See below.
>
> > + last = find_last_bit(mask, ARRAY_SIZE(pfl));
> > +
> > + for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> > + /* Format: Flag Name + '=' (equals sign) + Number + '|'
> > (separator) */
> > + buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > +
> > + if (buf < end)
> > + *buf = '=';
> > + buf++;
> > + buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> > + *pfl[i].spec);
> > +
> > + /* No separator for the last entry */
> > + if (i != last) {
> > + 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)
> > @@ -1929,10 +1989,10 @@ char *flags_string(char *buf, char *end, void
> > *flags_ptr,
> > switch (fmt[1]) {
> > case 'p':
> > flags = *(unsigned long *)flags_ptr;
> > - /* Remove zone id */
> > - flags &= (1UL << NR_PAGEFLAGS) - 1;
> > names = pageflag_names;
>
> The "names" variable is needed only with "break;" when using the final
> format_flags(buf, end, flags, names);
>
> > - break;
> > + buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) - 1),
> > names);
> > + buf = format_page_flags(buf, end, flags);
>
> I am sorry for my ignorance. I am not familiar with MM.
> But it is pretty hard to understand what call does what.
>
> I have found the following comment in include/linux/page_flags.h:
>
> * The page flags field is split into two parts, the main flags area
> * which extends from the low bits upwards, and the fields area which
> * extends from the high bits downwards.
>
> Sigh, I know that you already
Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp
On Tue 2021-02-09 16:16:01, Andy Shevchenko wrote: > On Tue, Feb 09, 2021 at 02:53:53PM +0100, Petr Mladek wrote: > > On Tue 2021-02-09 18:56:13, Yafang Shao wrote: > > ... > > > I am sorry for my ignorance. I am not familiar with MM. > > But it is pretty hard to understand what call does what. > > > > I have found the following comment in include/linux/page_flags.h: > > > > * The page flags field is split into two parts, the main flags area > > * which extends from the low bits upwards, and the fields area which > > * extends from the high bits downwards. > > > > Sigh, I know that you already reworked this several times because > > people "nitpicked" about the code style. But it seems that it > > rather diverged instead of converged. > > > > What about the following? > > Isn't is some like v1 or v2? Yes. And people suggested only some micro-optimizations and reported few small bugs there. But the code was heavily reworked in v3 to support the new %pGp[bl] variants. It also added the trick with the bitmap which invalidated all the previous suggestions. v3 and v4 review focused on behavior changes. We finally agreed on it. Let's give it more cycle and clean up the code after so many shuffles. > > Note: It is inpired by the names "main area" and "fields area" > > mentioned in the above comment from page_flags.h. > > I have later realized that "page_flags_layout" actually made > > sense as well. Feel free to rename page_flags_fileds > > back to page_flags_layout. > > > > Anyway, this is my proposal: > > What about to create a one format_flags() function which accepts new data > structure and do something like > > buf = format_flags(main_area); > buf = format_flags(fields_area); > return buf; I am not sure that it would make things easier. The handling of the main area is trivial and reuses existing structures. The handling of the fields area seems to be much more complicated. Best Regards, Petr
Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp
On Tue, Feb 09, 2021 at 02:53:53PM +0100, Petr Mladek wrote: > On Tue 2021-02-09 18:56:13, Yafang Shao wrote: ... > I am sorry for my ignorance. I am not familiar with MM. > But it is pretty hard to understand what call does what. > > I have found the following comment in include/linux/page_flags.h: > > * The page flags field is split into two parts, the main flags area > * which extends from the low bits upwards, and the fields area which > * extends from the high bits downwards. > > Sigh, I know that you already reworked this several times because > people "nitpicked" about the code style. But it seems that it > rather diverged instead of converged. > > What about the following? Isn't is some like v1 or v2? > Note: It is inpired by the names "main area" and "fields area" > mentioned in the above comment from page_flags.h. > I have later realized that "page_flags_layout" actually made > sense as well. Feel free to rename page_flags_fileds > back to page_flags_layout. > > Anyway, this is my proposal: What about to create a one format_flags() function which accepts new data structure and do something like buf = format_flags(main_area); buf = format_flags(fields_area); return buf; ? -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp
On Tue 2021-02-09 18:56:13, Yafang Shao wrote:
> Currently the pGp only shows the names of page flags, rather than
> the full information including section, node, zone, last cpupid and
> kasan tag. While it is not easy to parse these information manually
> because there're so many flavors. Let's interpret them in pGp as well.
>
> To be compitable with the existed format of pGp, the new introduced ones
> also use '|' as the separator, then the user tools parsing pGp won't
> need to make change, suggested by Matthew. The new information is
> tracked onto the end of the existed one.
>
> On example of the output in mm/slub.c as follows,
> - Before the patch,
> [ 6343.396602] Slab 0x4382e02b objects=33 used=3
> fp=0x9ae06ffc flags=0x17c0010200(slab|head)
>
> - After the patch,
> [ 8838.835456] Slab 0x2828b78a objects=33 used=3
> fp=0xd04efc88
> flags=0x17c0010200(slab|head|node=0|zone=2|lastcpupid=0x1f)
>
> The documentation and test cases are also updated. The output of the
> test cases as follows,
> [ 501.485081] test_printf: loaded.
> [ 501.485768] test_printf: all 388 tests passed
> [ 501.488762] test_printf: unloaded.
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 14c9a6af1b23..3f26611adb34 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1916,6 +1916,66 @@ char *format_flags(char *buf, char *end, unsigned long
> flags,
> return buf;
> }
>
> +struct page_flags_layout {
> + int width;
> + int shift;
> + int mask;
> + const struct printf_spec *spec;
> + const char *name;
> +};
> +
> +static const struct page_flags_layout pfl[] = {
> + {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
> + _dec_spec, "section"},
> + {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
> + _dec_spec, "node"},
> + {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
> + _dec_spec, "zone"},
> + {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
> + _flag_spec, "lastcpupid"},
> + {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
> + _flag_spec, "kasantag"},
> +};
> +
> +static
> +char *format_page_flags(char *buf, char *end, unsigned long flags)
> +{
> + DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> + unsigned long last;
> + int i;
> +
> + if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> + if (buf < end)
> + *buf = '|';
> + buf++;
> + }
This is far from obvious. You print '|' here because you printed
something somewhere else. See below.
> +
> + for (i = 0; i < ARRAY_SIZE(pfl); i++)
> + __assign_bit(i, mask, pfl[i].width);
The bitmap looks like an overkill. If I get it correctly, it is a
tricky way to handle only flags defined by the used build
configuration. See below.
> + last = find_last_bit(mask, ARRAY_SIZE(pfl));
> +
> + for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> + /* Format: Flag Name + '=' (equals sign) + Number + '|'
> (separator) */
> + buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> +
> + if (buf < end)
> + *buf = '=';
> + buf++;
> + buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> + *pfl[i].spec);
> +
> + /* No separator for the last entry */
> + if (i != last) {
> + 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)
> @@ -1929,10 +1989,10 @@ char *flags_string(char *buf, char *end, void
> *flags_ptr,
> switch (fmt[1]) {
> case 'p':
> flags = *(unsigned long *)flags_ptr;
> - /* Remove zone id */
> - flags &= (1UL << NR_PAGEFLAGS) - 1;
> names = pageflag_names;
The "names" variable is needed only with "break;" when using the final
format_flags(buf, end, flags, names);
> - break;
> + buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) - 1),
> names);
> + buf = format_page_flags(buf, end, flags);
I am sorry for my ignorance. I am not familiar with MM.
But it is pretty hard to understand what call does what.
I have found the following comment in include/linux/page_flags.h:
* The page flags field is split into two parts, the main flags area
* which extends from the low bits upwards, and the fields area which
* extends from the high bits downwards.
Sigh, I know that you already reworked this several times because
people "nitpicked" about the code style. But it seems that it
rather diverged instead of converged.
What about the following?
Note: It is inpired by the names "main area" and "fields area"
mentioned in the above comment from page_flags.h.
I have later realized
Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp
On 2/9/21 11:56 AM, Yafang Shao wrote: > Currently the pGp only shows the names of page flags, rather than > the full information including section, node, zone, last cpupid and > kasan tag. While it is not easy to parse these information manually > because there're so many flavors. Let's interpret them in pGp as well. > > To be compitable with the existed format of pGp, the new introduced ones > also use '|' as the separator, then the user tools parsing pGp won't > need to make change, suggested by Matthew. The new information is > tracked onto the end of the existed one. > > On example of the output in mm/slub.c as follows, > - Before the patch, > [ 6343.396602] Slab 0x4382e02b objects=33 used=3 > fp=0x9ae06ffc flags=0x17c0010200(slab|head) > > - After the patch, > [ 8838.835456] Slab 0x2828b78a objects=33 used=3 > fp=0xd04efc88 > flags=0x17c0010200(slab|head|node=0|zone=2|lastcpupid=0x1f) > > The documentation and test cases are also updated. The output of the > test cases as follows, > [ 501.485081] test_printf: loaded. > [ 501.485768] test_printf: all 388 tests passed > [ 501.488762] test_printf: unloaded. > > Signed-off-by: Yafang Shao > Cc: David Hildenbrand > Cc: Joe Perches > Cc: Miaohe Lin > Cc: Vlastimil Babka > Cc: Andy Shevchenko > Cc: Matthew Wilcox Acked-by: Vlastimil Babka The 'pfl' array should be even useful in kernel crash dump tools!
Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in pGp
On 09.02.21 11:56, Yafang Shao wrote: Currently the pGp only shows the names of page flags, rather than the full information including section, node, zone, last cpupid and kasan tag. While it is not easy to parse these information manually because there're so many flavors. Let's interpret them in pGp as well. To be compitable with the existed format of pGp, the new introduced ones also use '|' as the separator, then the user tools parsing pGp won't need to make change, suggested by Matthew. The new information is tracked onto the end of the existed one. On example of the output in mm/slub.c as follows, - Before the patch, [ 6343.396602] Slab 0x4382e02b objects=33 used=3 fp=0x9ae06ffc flags=0x17c0010200(slab|head) - After the patch, [ 8838.835456] Slab 0x2828b78a objects=33 used=3 fp=0xd04efc88 flags=0x17c0010200(slab|head|node=0|zone=2|lastcpupid=0x1f) Did not review in detail, but LGTM. Thanks, this will be very helpful! -- Thanks, David / dhildenb
