Re: [PATCH v3 2/3] mm: gup: allow long-term pins of CMA-allocated HugeTLB folios

[Qi Zheng]

Hi David,

On 11/11/26 3:43 PM, David Hildenbrand (Arm) wrote:
> On 11/9/26 04:10, Qi Zheng wrote:
>>
>>
>> On 11/8/26 1:58 AM, David Hildenbrand (Arm) wrote:
>>> On 11/7/26 11:30, Qi Zheng wrote:
>>>> From: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
>>>>
>>>> Long-term pinning treats explicitly CMA-allocated HugeTLB folios as
>>>> unpinnable, causing unnecessary migration or pinning failures.
>>>>
>>>> These folios already own their CMA bitmap ranges, and memory blocks
>>>> overlapping CMA areas cannot be offlined. Allow HugeTLB folios with
>>>> HPG_cma to be pinned directly, including in ZONE_MOVABLE.
>>>>
>>>> Fixes: cf11e85fc08c ("mm: hugetlb: optionally allocate gigantic hugepages using cma")
>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>> Suggested-by: Muchun Song <muchun.song@xxxxxxxxx>
>>>> Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
>>>> Acked-by: Muchun Song <muchun.song@xxxxxxxxx>
>>>> Reviewed-by: Oscar Salvador <osalvador@xxxxxxx>
>>>> Reviewed-by: Jose A. Perez de Azpillaga <azpijr@xxxxxxxxx>
>>>> ---
>>>>    include/linux/mm.h | 29 +----------------------------
>>>>    mm/gup.c           | 31 +++++++++++++++++++++++++++++++
>>>>    2 files changed, 32 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>> index 2d0b8a4c9974f..75dd40265d3f4 100644
>>>> --- a/include/linux/mm.h
>>>> +++ b/include/linux/mm.h
>>>> @@ -2975,35 +2975,8 @@ static inline bool is_zero_folio(const struct folio *folio)
>>>>    	return is_zero_page(&folio->page);
>>>>    }
>>>>    
>>>> -/* MIGRATE_CMA and ZONE_MOVABLE do not allow pin folios */
>>>>    #ifdef CONFIG_MIGRATION
>>>> -static inline bool folio_is_longterm_pinnable(struct folio *folio)
>>>> -{
>>>> -#ifdef CONFIG_CMA
>>>> -	int mt = folio_migratetype(folio);
>>>> -
>>>> -	if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE)
>>>> -		return false;
>>>> -#endif
>>>> -	/* The zero page can be "pinned" but gets special handling. */
>>>> -	if (is_zero_folio(folio))
>>>> -		return true;
>>>> -
>>>> -	/* Coherent device memory must always allow eviction. */
>>>> -	if (folio_is_device_coherent(folio))
>>>> -		return false;
>>>> -
>>>> -	/*
>>>> -	 * Filesystems can only tolerate transient delays to truncate and
>>>> -	 * hole-punch operations
>>>> -	 */
>>>> -	if (folio_is_fsdax(folio))
>>>> -		return false;
>>>> -
>>>> -	/* Otherwise, non-movable zone folios can be pinned. */
>>>> -	return !folio_is_zone_movable(folio);
>>>> -
>>>> -}
>>>
>>> Moving code while adding new stuff to it, please don't.
>>>
>>> Move code first, then modify it.
>>>
>>> I don't want to play "find the difference between two pictures" on a Saturday
>>> evening.
>>
> 
> You resent your series way too fast.

Got it.

> 
> Now we have discussions in to threads, congratulations ;)
> 
>> ;), got it.
>>
>> I moved the code to resolve a build dependency.
>>
> 
> Why can't that be resolved differently? What is the dependency?

The dependency is that the new check wants to use
`folio_test_hugetlb_cma()`, which is a HugeTLB-specific inline helper
from `include/linux/hugetlb.h`.

`folio_is_longterm_pinnable()` currently lives in `include/linux/mm.h`,
while `include/linux/hugetlb.h` already includes `linux/mm.h`. So I
can't simply use `folio_test_hugetlb_cma()` from `mm.h`

> 
>>>
>>>> +bool folio_is_longterm_pinnable(struct folio *folio);
>>>>    #else
>>>>    static inline bool folio_is_longterm_pinnable(struct folio *folio)
>>>>    {
>>>> diff --git a/mm/gup.c b/mm/gup.c
>>>> index 2b405cce2ca5c..f41701c603500 100644
>>>> --- a/mm/gup.c
>>>> +++ b/mm/gup.c
>>>> @@ -2216,6 +2216,37 @@ struct page *get_dump_page(unsigned long addr, int *locked)
>>>>    
>>>>    #ifdef CONFIG_MIGRATION
>>>>    
>>>> +bool folio_is_longterm_pinnable(struct folio *folio)
>>>> +{
>>>> +#ifdef CONFIG_CMA
>>>> +	int mt = folio_migratetype(folio);
>>>> +
>>>> +	/* These HugeTLB folios own the CMA memory, so they can be long-term pinned. */
>>>> +	if (folio_test_hugetlb(folio) && folio_test_hugetlb_cma(folio))
>>>> +		return true;
>>> More hugetlb hackery ... don't like that, sorry.
>>
>> Agree.
>>
>>>
>>> I think this just goes down to a problem we discussed a while ago that is
>>> independent of hugetlb: we'd need some way to identify that the the page was
>>> actually allocated by the CMA owner, and it's not just some other movable
>>> allcoation that ended up there.
>>>
>>> For something generic non-hugetlb thing, can't we call into CMA to ask whether
>>> the current chunk is marked allocated?
>>
>> The existing bitmap is set before allocation completes, so the range
>> may still contain borrowed movable pages. A set bit alone cannot
>> establish ownership.

The more accurate code path is as follows.

Assuming the 2M buddy allocation is successful, page P is borrowed from
the 1G CMA area.

Now, proceed to request a new 1G huge page:

only_alloc_fresh_hugetlb_folio
--> alloc_gigantic_frozen_folio
     --> hugetlb_cma_alloc_frozen_folio
         --> cma_range_alloc
                 /* set bitmap!! */
             --> bitmap_set(cmr->bitmap, bitmap_no, bitmap_count);

                 <time A>

                 /* may fail */
                 alloc_contig_frozen_range

         /* set HPG_cma!! */
         folio_set_hugetlb_cma(folio);

At time A, for page P, a bitmap was set, but HPG_cma was not set.

If a concurrent FOLL_LONGTERM pin uses the bitmap alone to determine
ownership, it could incorrectly allow P to remain pinned. If
alloc_contig_frozen_range() fails, cma_range_alloc() clears the bitmap
bits, while the long-term pin on P remains.

HPG_cma is set only after explicit CMA allocation succeeds. It records
the folio's own CMA ownership, whereas a set bitmap bit can represent
another allocation's in-flight reservation. Therefore, the bitmap
alone cannot replace HPG_cma in this check.

>>
>> A generic query may need separate ownership tracking, such as an
>> owned_bitmap recording successfully allocated pages. Its updates and
>> lookups would also need to handle concurrency with fast GUP.
> 
> Or simply two bitmaps? "trying allocation - allocated"

As a fix, perhaps this is a bit too complex? :(

Alternatively, we could adjust the timing for setting the bitmap, though
this might introduce new concurrency issues.

Thanks,
Qi