Re: [RFC v2 1/3] mm: move PG_slab flag to page_type

["HORIGUCHI NAOYA(堀口 直也)" <naoya.horiguchi@nec.com>]

On Sun, Nov 06, 2022 at 11:03:53PM +0900, Hyeonggon Yoo wrote:
> For now, only SLAB uses _mapcount field as a number of active objects in
> a slab, and other slab allocators do not use it. As 16 bits are enough
> for that, use remaining 16 bits of _mapcount as page_type even when
> SLAB is used. And then move PG_slab flag to page_type.
> 
> As suggested by Matthew, store number of active objects in negative
> form and use helper when accessing or modifying it.
> 
> Note that page_type is always placed in upper 16 bits of _mapcount to
> avoid confusing normal _mapcount as page_type. As underflow (actually
> I mean, yeah, overflow) is not a concern anymore, use more lower bits.
> 
> Add more folio helpers for PAGE_TYPE_OPS() not to break existing
> slab implementations.
> 
> Remove PG_slab check from PAGE_FLAGS_CHECK_AT_FREE. buddy will still
> check if _mapcount is properly set at free.
> 
> Exclude PG_slab from hwpoison and show_page_flags() for now.
> 
> Note that with this patch, page_mapped() and folio_mapped() always return
> false for slab page.
> 
...

> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 779a426d2cab..9494f47c4cee 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1145,7 +1145,6 @@ static int me_huge_page(struct page_state *ps, struct page *p)
>  #define mlock		(1UL << PG_mlocked)
>  #define lru		(1UL << PG_lru)
>  #define head		(1UL << PG_head)
> -#define slab		(1UL << PG_slab)
>  #define reserved	(1UL << PG_reserved)
>  
>  static struct page_state error_states[] = {
> @@ -1155,13 +1154,6 @@ static struct page_state error_states[] = {
>  	 * PG_buddy pages only make a small fraction of all free pages.
>  	 */
>  
> -	/*
> -	 * Could in theory check if slab page is free or if we can drop
> -	 * currently unused objects without touching them. But just
> -	 * treat it as standard kernel for now.
> -	 */
> -	{ slab,		slab,		MF_MSG_SLAB,	me_kernel },
> -

Hi Hyeonggon,

Actually the above part is dead code now and it's harmless to remove this.
identify_page_state() is never called when handling memory error event
on a slab page because HWPoisonHandlable() returns false for it.

If you remove it, a few other lines using MF_MSG_SLAB will be unneccessary,
so could you remove them too?

As for testing, you can test this case for example like below:

  - install page-types (available in tools/vm/page-types.c),
  - show the list of slab pages by "page-types -b slab -Nl" command
    (the first column is PFNs) and choose one PFNs as target,
  - call "page-types -a <PFN> -X" (requires hwpoison-inject module to be loaded)

In my testing server, dmesg shows like below:

[598746.497805] Injecting memory failure at pfn 0x16295b
[598746.499208] Memory failure: 0x16295b: unhandlable page.
[598746.500570] Memory failure: 0x16295b: recovery action for unknown page: Ignored

And your patchset seems not to affect this behavior.

Thanks,
Naoya Horiguchi