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:56, Harry Yoo wrote:
> 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)

Yes, willy's plan is to make page_folio() fail later. For now, we can 
just do racy checks ahead of time (kpagecount code is inherently racy).

-- 
Cheers,

David / dhildenb