Re: [PATCH 2/2] mm: introduce PAGE_FLAGS() to make output of page flags better

[Yafang Shao <laoar.shao@gmail.com>]

On Fri, Jan 15, 2021 at 4:39 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 15.01.21 07:13, Yafang Shao wrote:
> > There're totally __NR_PAGEFLAGS page flags, but the type of page->flags is
> > unsigned long, that makes the value of page->flags a little misleading when
> > it is printed to the user. We'd better print the real pages flags, instead
> > of the whole 64bits including the random values in the useless high bits.
>
> No, these are *not* random values. They include the nid, zid, and
> section_nr - which are helpful to have at hand when debugging, or
> detecting that something might be messed up there.
>

Thanks for the explanation. I just noticed the page-flags layout in
page-flags-layout.h.

> >
> > There're two choices to achieve that, one of which is clear the useless
>
> Again, not useless.
>
> > high bits when we initlize the page->flags, the other is don't print the
> > high bits when it is showed to the user. The latter one is better because
> > it is in the slow path and the performance won't be impacted.
> >
> > Before that change, the output is,
> > [ 8846.517809] INFO: Slab 0x00000000f42a2c60 objects=33 used=3 fp=0x0000000060d32ca8 flags=0x17ffffc0010200(slab|head)
> >
> > After that change, the output is,
> > [ 8843.757770] INFO: Slab 0x00000000f0e98335 objects=33 used=3 fp=0x00000000b643c7d8 flags=0x10200(slab|head)
> >
>
> Nack to the current approach. If you're going to strip this information,
> you should expose it differently. E.g., printing page_zonenum() or
> page_to_nid(). But still, then we might lose valuable information of
> bits stored in there that shouldn't have been set.
>

How about changing the implementation of pGp in printk() ?
In the new implementation of pGp we can dump the full information of
page->flags, rather than the flag's name only.

For example,
0xXXXXXXXX(node n, nid n, ..., slab|head)

That will make it easier to understand, as it is not easy to look into
the detail of page-flags layout.

> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/linux/page-flags.h | 2 ++
> >  mm/slub.c                  | 2 +-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index ec5d029..db5c017 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -175,6 +175,8 @@ enum pageflags {
> >       PG_reported = PG_uptodate,
> >  };
> >
> > +#define PAGE_FLAGS(flags) (flags & ((1 << __NR_PAGEFLAGS) - 1))
> > +
> >  #ifndef __GENERATING_BOUNDS_H
> >
> >  struct page; /* forward declaration */
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 901a45a..a93a03c 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -643,7 +643,7 @@ static void print_page_info(struct page *page)
> >  {
> >       pr_err("INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n",
> >              page, page->objects, page->inuse, page->freelist,
> > -            page->flags, &page->flags);
> > +            PAGE_FLAGS(page->flags), &page->flags);
> >
> >  }
> >
> >
>
>
> --
> Thanks,
>
> David / dhildenb
>


-- 
Thanks
Yafang