On Fri, Oct 07, 2022 at 07:02:35PM +0100, Matthew Wilcox wrote: > On Fri, Oct 07, 2022 at 10:36:56PM +0900, Hyeonggon Yoo wrote: > > > First, you say that folio_mapped() returns false for slab pages. That's > > > only true for order-0 slab pages. For larger pages, > > > > > > if (!folio_test_large(folio)) > > > return atomic_read(&folio->_mapcount) >= 0; > > > if (atomic_read(folio_mapcount_ptr(folio)) >= 0) > > > return true; > > > > > > so that's going to depend what folio_mapcount_ptr() aliases with. > > > > IIUC it's true for order > 0 slab too. > > > > As slab pages are not mapped to userspace at all, > > entire compound page nor base pages are not mapped to userspace. > > > > AFAIK followings are true for order > 0 slab: > > - (first tail page)->compound_mapcount is -1 > > That's the part I wasn't sure of. I think we do, in > prep_compound_head(). Right, exactly! > > > - _mapcount of base pages are -1 > > > > So: > > folio_mapped() and page_mapped() (if applied to head page) > > returns false for larger pages with this patch. > > > > I wrote simple testcase and did check that folio_mapped() and page_mapped() > > returns false for both order-0 page and larger pages. (and SLAB > > returned true for them before) FYI, This is still true even after fixing my mistaken test case (see below) > > > > > Second, this patch changes the behaviour of PageSlab() when applied to > > > tail pages. > > > > Altough it changes the way it checks the flag, > > > > it does not change behavior when applied to tail pages - PageSlab() on tail > > page returns false with or without this patch. > > Really? It seems to me that it returns true at the moment. Look: > > __PAGEFLAG(Slab, slab, PF_NO_TAIL) > > #define PF_NO_TAIL(page, enforce) ({ \ > VM_BUG_ON_PGFLAGS(enforce && PageTail(page), page); \ > PF_POISONED_CHECK(compound_head(page)); }) > > so AFAICS, PageSlab checks the Slab bit on the head page, not the > tail page. You are right. I misunderstood it due to my mistakenly written test case (without passing __GFP_COMP... how silly of me :D) Hmm okay, then I will implement PF_NO_TAIL policy that works on page_type. > > > If PageSlab() need to return true for tail pages too, > > we may make it check page_type at head page. > > > > But I'm not sure when it the behavior is needed. > > Can you please share your insight on this? > > There are tools like tools/vm/page-types.c which expect PageSlab to > return true for tail pages. > > > > Which raises the further question of what PageBuddy(), > > > PageTable(), PageGuard() and PageIsolated() should do for multi-page > > > folios, if that is even possible. > > > > For users that uses real compound page like slab, we can make it check > > page_type of head page. (if needed) > > > > But for cases David described, there isn't much thing we can do > > except making them to use real compound pages. > > > > > Third, can we do this without that awkward __u16 thing? Perhaps > > > > > > -#define PG_buddy 0x00000080 > > > -#define PG_offline 0x00000100 > > > -#define PG_table 0x00000200 > > > -#define PG_guard 0x00000400 > > > +#define PG_buddy 0x00010000 > > > +#define PG_offline 0x00020000 > > > +#define PG_table 0x00040000 > > > +#define PG_guard 0x00080000 > > > +#define PG_slab 0x00100000 > > > > > > ... and then use wrappers in slab.c to access the bottom 16 bits? > > > > Definitely! I prefer that way and will adjust in RFC v2. > > > > Thank you for precious feedback. > > No problem. I suggested (in an off-list email) that you consider counting > 'active' by subtraction rather than addition because I have a feeling that > > int active(struct slab *slab) > { > return ~(slab->page_type | PG_slab); > } > > would be better than > > int active(struct slab *slab) > { > return slab->page_type & 0xffff; > } > > at least in part because you don't have to clear the bottom 16 bits of > page_type when you clear PG_slab, and you don't have to re-set them > when you set PG_slab. Yeah, I was wondering what is the benefit of the that approach. After implementing both approach, your suggestion seems better to me too. Many thanks, Matthew! -- Hyeonggon