Re: [PATCH v2 mm-new] mm: check if folio has valid mapcount before folio_test_{anon,ksm}() when necessary

[Harry Yoo <harry.yoo@oracle.com>]

On Tue, Jul 08, 2025 at 10:32:44AM +0200, David Hildenbrand wrote:
> On 08.07.25 10:30, Harry Yoo wrote:
> > On Tue, Jul 08, 2025 at 10:07:26AM +0200, David Hildenbrand wrote:
> > > >    	if (folio_test_anon(folio)) {
> > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > > > index 970600d79dac..00ba2c8b221e 100644
> > > > --- a/include/linux/page-flags.h
> > > > +++ b/include/linux/page-flags.h
> > > > @@ -706,12 +706,15 @@ PAGEFLAG_FALSE(VmemmapSelfHosted, vmemmap_self_hosted)
> > > >     * address_space which maps the folio from disk; whereas "folio_mapped"
> > > >     * refers to user virtual address space into which the folio is mapped.
> > > >     *
> > > > - * For slab pages, since slab reuses the bits in struct page to store its
> > > > - * internal states, the folio->mapping does not exist as such, nor do
> > > > - * these flags below.  So in order to avoid testing non-existent bits,
> > > > - * please make sure that folio_test_slab(folio) actually evaluates to
> > > > - * false before calling the following functions (e.g., folio_test_anon).
> > > > - * See mm/slab.h.
> > > > + * For certain typed pages like slabs, since they reuse bits in struct page
> > > > + * to store internal states, folio->mapping does not point to a valid
> > > > + * mapping, nor do these flags exist. To avoid testing non-existent bits,
> > > > + * make sure folio_has_mapcount() actually evaluates to true before calling
> > > > + * the following functions (e.g., folio_test_anon).
> > > > + *
> > > > + * The folio_has_mapcount() check can be skipped if the folio is mapped
> > > > + * to userspace, since a folio with !folio_has_mapcount() cannot be mapped
> > > > + * to userspace at all.
> > > >     */
> > > >    #define FOLIO_MAPPING_ANON	0x1
> > > >    #define FOLIO_MAPPING_ANON_KSM	0x2
> > > > @@ -1092,6 +1095,11 @@ static inline bool PageHuge(const struct page *page)
> > > >    	return folio_test_hugetlb(page_folio(page));
> > > >    }
> > > > +static inline bool folio_has_mapcount(const struct folio *folio)
> > > > +{
> > > > +	return !page_has_type(&folio->page) || folio_test_hugetlb(folio);
> > > > +}
> > > > +
> > > 
> > > I don't like the naming.
> > 
> > I don't like it either, but I couldn't come up with a better name :(
> > 
> > > The thing is, in the future only folios will have a mapcount.
> > 
> > Agreed.
> > 
> > > Asking whether a folio has a mapcount is asking the wrong question.
> > 
> > Agreed.
> > 
> > > It's rather, that we want to reject *pages* early, and not cast them to the
> > > folio in the first place.
> > 
> > Totally agreed.
> > 
> > But that's the future, and we're not there yet.
> > What should we do now?
> 
> Check before converting from page to folio in stable_page_flags() and leave
> folio_expected_ref_count() alone. In folio_expected_ref_count() it's a
> sanity check that should never happen.

TBH it wasn't clear when the page-to-folio conversion should be
rejected... (I can't just change page_folio() to reject that, right now)

But now I get your point. It shouldn't cast to folio and do
folio-specific checks if it can't be a folio.

The suggestion makes sense and will do that.
Appreciate your comment, David!

-- 
Cheers,
Harry / Hyeonggon