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

[David Hildenbrand <david@redhat.com>]

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.

-- 
Cheers,

David / dhildenb