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

[David Hildenbrand <david@redhat.com>]

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.

> 
> 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.

> 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