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

[Hyeonggon Yoo <42.hyeyoo@gmail.com>]

On Tue, Nov 08, 2022 at 05:39:24AM +0000, HORIGUCHI NAOYA(堀口 直也) wrote:
> 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,
>

Hi Naoya.

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

Oh wasn't aware of it. Thanks. That makes it easier.

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

Sure. Will split this to separate patch then.

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

Cool! Will test it before sending next version.

Thanks for looking at this :-)

-- 
Thanks,
Hyeonggon