Re: [PATCH v4 5/6] mm: cma: add cma_alloc_frozen{_compound}()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2025/12/22 10:30, Zi Yan wrote:
> On 18 Dec 2025, at 23:09, Kefeng Wang wrote:
> 
>> On 2025/12/18 23:52, Zi Yan wrote:
>>> On 18 Dec 2025, at 7:54, Kefeng Wang wrote:
>>>
>>>> On 2025/12/18 3:38, Zi Yan wrote:
>>>>> On 17 Dec 2025, at 3:02, Kefeng Wang wrote:
>>>>>
>>>>>> On 2025/12/17 2:40, Zi Yan wrote:
>>>>>>> On 16 Dec 2025, at 6:48, Kefeng Wang wrote:
>>>>>>>
>>>>>>>> Introduce cma_alloc_frozen{_compound}() helper to alloc pages without
>>>>>>>> incrementing their refcount, then convert hugetlb cma to use the
>>>>>>>> cma_alloc_frozen_compound() and cma_release_frozen() and remove the
>>>>>>>> unused cma_{alloc,free}_folio(), also move the cma_validate_zones()
>>>>>>>> into mm/internal.h since no outside user.
>>>>>>>>
>>>>>>>> The set_pages_refcounted() is only called to set non-compound pages
>>>>>>>> after above changes, so remove the processing about PageHead.
>>>>>>>>
>>>>>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>>>>>> ---
>>>>>>>>      include/linux/cma.h | 26 ++++++------------------
>>>>>>>>      mm/cma.c            | 48 +++++++++++++++++++++++++--------------------
>>>>>>>>      mm/hugetlb_cma.c    | 24 +++++++++++++----------
>>>>>>>>      mm/internal.h       | 10 +++++-----
>>>>>>>>      4 files changed, 52 insertions(+), 56 deletions(-)
>>>>>>>>
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>>>      static bool __cma_release(struct cma *cma, const struct page *pages,
>>>>>>>> -			  unsigned long count, bool compound)
>>>>>>>> +			  unsigned long count, bool frozen)
>>>>>>>>      {
>>>>>>>>      	unsigned long pfn, end;
>>>>>>>>      	int r;
>>>>>>>> @@ -974,8 +982,8 @@ static bool __cma_release(struct cma *cma, const struct page *pages,
>>>>>>>>      		return false;
>>>>>>>>      	}
>>>>>>>>
>>>>>>>> -	if (compound)
>>>>>>>> -		__free_pages((struct page *)pages, compound_order(pages));
>>>>>>>> +	if (frozen)
>>>>>>>> +		free_contig_frozen_range(pfn, count);
>>>>>>>>      	else
>>>>>>>>      		free_contig_range(pfn, count);
>>>>>>>
>>>>>>> Can we get rid of free_contig_range() branch by making cma_release() put
>>>>>>> each page’s refcount? Then, __cma_relase() becomes cma_release_frozen()
>>>>>>> and the release pattern matches allocation pattern:
>>>>>>> 1. cma_alloc() calls cma_alloc_frozen() and manipulates page refcount.
>>>>>>> 2. cma_release() manipulates page refcount and calls cma_release_frozen().
>>>>>>>
>>>>>>
>>>>>> Have considered similar things before, but we need manipulates page
>>>>>> refcount only find the correct cma memrange from cma/pages, it seems
>>>>>> that no big improvement, any more comments?
>>>>>>
>>>>>> 1) for cma_release:
>>>>>>       a. cma find memrange
>>>>>>       b. manipulates page refcount when cmr found
>>>>>>       c. free page and release cma resource
>>>>>> 2) for cma_release_frozen
>>>>>>       a. cma find memrange
>>>>>>       b. free page and release cma resource whne cmr found
>>>>>
>>>>> Right, I think it makes code simpler.
>>>>>
>>>>> Basically add a helper function:
>>>>> struct cma_memrange* find_cma_memrange(struct cma *cma,
>>>>> 		const struct page *pages, unsigned long count);
>>>>>
>>>>> Then
>>>>>
>>>>> __cma_release_frozen()
>>>>> {
>>>>> 	free_contig_frozen_range(pfn, count);
>>>>> 	cma_clear_bitmap(cma, cmr, pfn, count);
>>>>> 	cma_sysfs_account_release_pages(cma, count);
>>>>> 	trace_cma_release(cma->name, pfn, pages, count);
>>>>> }
>>>>>
>>>>>
>>>>> cma_release()
>>>>> {
>>>>> 	cmr = find_cma_memrange();
>>>>>
>>>>> 	if (!cmr)
>>>>> 		return false;
>>>>> 	
>>>>> 	for (; count--; pages++)
>>>>> 		VM_WARN_ON(!put_page_testzero(pages);
>>>>>
>>>>> 	__cma_release_frozen();
>>>>> }
>>>>>
>>>>> cma_release_frozen()
>>>>> {
>>>>> 	cmr = find_cma_memrange();
>>>>>
>>>>> 	if (!cmr)
>>>>> 		return false;
>>>>>
>>>>> 	__cma_release_frozen();
>>>>>
>>>>> }
>>>>>
>>>>> Let me know your thoughts.
>>>>
>>>> Yes, this is exactly what I described above that needs to be done, but I
>>>> think it will add more codes :)
>>>>
>>>> Our goal is that convert all cma_{alloc,release} caller to
>>>> cma_frozen_{alloc,release}, and complete remove free_contig_range in cma, Maybe no changes? But if you prefer above way, I can also update
>>>> it.
>>>
>>> If the goal is to replace all cma_{alloc,release}() calls with the frozen version,
>>> there is no need to make the change as I suggested. Are you planning to send
>>> another patchset to do the replacement after this one?
>>
>> There are few callers, the following can be straightforwardly converted
>> to a frozen version.
>>
>>    mm/cma_debug.c
>>    drivers/dma-buf/heaps/cma_heap.c
>>    drivers/s390/char/vmcp.c
>>    arch/powerpc/kvm/book3s_hv_builtin.c
>>
>> For the DMA part, we suppose there is no driver using the page refcount, as too many drivers are involved, maybe there is a very special usage in the driver，I can't be sure.
>>
>>    kernel/dma/contiguous.c
> 
> Even if drivers are not using page refcount, these allocated cma pages should
> be still have a elevated refcount to indicate they are in use, right? So
> cma_alloc() and cma_release() can be converted to frozen + refcount manipulation,
> right? I am not sure letting drivers use froze pages directly is the right move.

OK. I previously simply believed there might be an opportunity, but it
seems it might not be that straightforward.

> Frozen pages should only be changed by the one freezes the pages and no one else
> should touch it.

Anyway，thank for sharing your points, I will change cma_release()
according to your suggestion in next version.

Thanks.

> 
> Best Regards,
> Yan, Zi
>