Re: [PATCH v5 3/3] vsprintf: dump full information of page flags in pGp

[Yafang Shao]

On Mon, Feb 22, 2021 at 8:38 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> Hello,
>
> first, I am sorry for the late reply. I have marked the thread as
> proceed by mistake last week...
>
>
> On Mon 2021-02-15 23:51:41, 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.
> >
> > One example of the output in mm/slub.c as follows,
> > - Before the patch,
> > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> >
> > - After the patch,
> > [ 8448.272530] Slab 0x0000000090797883 objects=33 used=3 fp=0x00000000790f1c26 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> >
> > The documentation and test cases are also updated. The output of the
> > test cases as follows,
> > [11585.830272] test_printf: loaded.
> > [11585.830454] test_printf: all 388 tests passed
> > [11585.831401] test_printf: unloaded.
> >
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > +static
> > +char *format_page_flags(char *buf, char *end, unsigned long flags)
> > +{
> > +     unsigned long main_flags = flags & (BIT(NR_PAGEFLAGS) - 1);
> > +     bool append = false;
> > +     int i;
> > +
> > +     /* Page flags from the main area. */
> > +     if (main_flags) {
> > +             buf = format_flags(buf, end, main_flags, pageflag_names);
> > +             append = true;
> > +     }
> > +
> > +     /* Page flags from the fields area */
> > +     for (i = 0; i < ARRAY_SIZE(pff); i++) {
> > +             /* Skip undefined fields. */
> > +             if (!pff[i].width)
> > +                     continue;
> > +
> > +             /* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
> > +             if (append) {
> > +                     if (buf < end)
> > +                             *buf = '|';
> > +                     buf++;
> > +             }
> > +
> > +             buf = string(buf, end, pff[i].name, *pff[i].spec);
>
> I have found one more small issue.
>
> The purpose of the flag-specific printk_spec is to define the format
> how the value is printed. The name of the flag should be printed
> using default_str_spec.
>
> It works because the string is printed as-is with both
> default_dec_spec and default_flag_spec. But it would be better
> to use the string format.
>

Thanks for the explanation.

> > +             if (buf < end)
> > +                     *buf = '=';
> > +             buf++;
> > +             buf = number(buf, end, (flags >> pff[i].shift) & pff[i].mask,
> > +                          *pff[i].spec);
> > +
> > +             append = true;
> > +     }
> > +
> > +     return buf;
> > +}
>
> Otherwise, the patch looks to me. The issue is cosmetic and might be
> fixed either by re-spinning just this patch or by a followup patch.

I will send a separate followup patch.

> Either way, feel free to use:
>
> Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>
>

Thanks

> Another question where to push this change. It is pity the we
> finalized it in the middle of the merge window. It has to spend
> at least few days in linux-next.
>
> I would like to hear from Andy before I push it into linux-next.
> There is still theoretical chance to get it into 5.12 when Linus
> prolongs the merge window by one week. it has been delayed by
> a long lasting power outage.
>
> Best Regards,
> Petr



-- 
Thanks
Yafang