linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* Re: A potential refcount issue during __folio_split
       [not found] ` <20260222010708.uohpmddmzaa4i4ic@master>
@ 2026-02-22  3:00   ` Zi Yan
  0 siblings, 0 replies; only message in thread
From: Zi Yan @ 2026-02-22  3:00 UTC (permalink / raw)
  To: Wei Yang; +Cc: David Hildenbrand (Arm), Linux MM

+linux-mm list, since the topic is interesting and worth having a record in the list.

On 21 Feb 2026, at 20:07, Wei Yang wrote:

> On Sun, Feb 22, 2026 at 01:04:25AM +0000, Wei Yang wrote:
>> Hi, David & Zi Yan
>>
>> With some tests, I may find one refcount issue during __folio_split(), when
>>
>>  * folio is isolated and @list is provided
>>  * @lock_at is not the large folio's head page
>>
>> The tricky thing is after __folio_freeze_and_split_unmapped() and
>> remap_page(), we would release one refcount for each after-split folio except
>> @lock_at after-split folio.
>>
>> But when @list is provided, we would grab extra refcount in
>> __folio_freeze_and_split_unmapped() for each tail after-split folio by
>> lru_add_split_folio(), except head after-split folio.
>>
>> If @lock_at is the large folio's head page, it is fine. If not, the
>> after-split folio's refcount is not well maintained.
>>
>> Take anonymous large folio mapped in one process for example, the refcount
>> change during uniformly __folio_split() to order-0 would look like this:
>>
>> after lru_add_split_folio()  after remap     after unlock if @lockat == head
>>   f0: 1                      1 + 1 = 2       2
>>   f1: 1 + 1 = 2              2 + 1 = 3       3 - 1 = 2
>>   f2: 1 + 1 = 2              2 + 1 = 3       3 - 1 = 2
>>   f3: 1 + 1 = 2              2 + 1 = 3       3 - 1 = 2
>>
>>                                              after unlock if @lockat == head + 1
>>                                              2 - 1 = 1
>>                                              3
>>                                              3 - 1 = 2
>>                                              3 - 1 = 2
>>
>> This shows the refcount of f0/f1 is not correct, if @lockat != head page.

after lru_add_split_folio(), refcount for head should be 0, since it is frozen,
each of the rest subpages should be refcount == 1. Then, head is unfreezed
and its refcount goes to 1. remap adds 1 to all subpages refcount.
after the unlock loop, every subpage get -1 refcount except lock_at.


>>
>> The good news is there is no use case in kernel now. Then I am not sure this
>> worth a fix. Would like to ask for your opinion first. Hope I don't miss
>> something important.
>>
>> Since there is no real case in kernel, I adjust the current debugfs
>> interface(/sys/kernel/debug/split_huge_pages) to trigger it. Below is the diff
>> for the change. This change could pass the selftests/split_huge_page_test to
>> make sure the code change itself is correct.
>>
>> Then change the lockat to folio_page(folio, 1) could trigger the issue, when
>> trying to split a THP through the debugfs from userspace.
>>
>
> Sorry, the diff is lost:
>
> From c6d4c3d81e16f5f4b509cff884540bec0f91e6c3 Mon Sep 17 00:00:00 2001
> From: Wei Yang <richard.weiyang@gmail.com>
> Date: Thu, 19 Feb 2026 08:44:49 +0800
> Subject: [PATCH] [T]mm/huge_memory: test split with isolation
>
> ---
>  mm/huge_memory.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index ed0375ea22d1..65354c5edfef 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -4621,9 +4621,11 @@ static int split_huge_pages_pid(int pid, unsigned long vaddr_start,
>  	for (addr = vaddr_start; addr < vaddr_end; addr += PAGE_SIZE) {
>  		struct vm_area_struct *vma = vma_lookup(mm, addr);
>  		struct folio_walk fw;
> -		struct folio *folio;
> +		struct folio *folio, *folio2;
> +		struct page *lockat;
>  		struct address_space *mapping;
>  		unsigned int target_order = new_order;
> +		LIST_HEAD(split_folios);
>
>  		if (!vma)
>  			break;
> @@ -4660,32 +4662,41 @@ static int split_huge_pages_pid(int pid, unsigned long vaddr_start,
>  		    folio_expected_ref_count(folio) != folio_ref_count(folio))
>  			goto next;
>
> -		if (!folio_trylock(folio))
> +		if (!folio_isolate_lru(folio))
>  			goto next;
> -		folio_get(folio);
> -		folio_walk_end(&fw, vma);
>
>  		if (!folio_test_anon(folio) && folio->mapping != mapping)
> -			goto unlock;
> +			goto putback;
> +
> +		folio_lock(folio);
> +		folio_walk_end(&fw, vma);
> +		lockat = folio_page(folio, 0);
>
>  		if (in_folio_offset < 0 ||
>  		    in_folio_offset >= folio_nr_pages(folio)) {
> -			if (!split_folio_to_order(folio, target_order))
> +			lockat = folio_page(folio, 0);
> +			if (!split_huge_page_to_list_to_order(lockat, &split_folios, target_order))
>  				split++;
>  		} else {
>  			struct page *split_at = folio_page(folio,
>  							   in_folio_offset);
> -			if (!folio_split(folio, target_order, split_at, NULL))
> +			if (!folio_split(folio, target_order, split_at, &split_folios))
>  				split++;
>  		}
>
> -unlock:
> +		list_add_tail(&folio->lru, &split_folios);
> +		folio_unlock(page_folio(lockat));
>
> -		folio_unlock(folio);
> -		folio_put(folio);
> +		list_for_each_entry_safe(folio, folio2, &split_folios, lru) {
> +			list_del(&folio->lru);
> +			folio_putback_lru(folio);
> +		}
>
>  		cond_resched();
>  		continue;
> +
> +putback:
> +		folio_putback_lru(folio);

        ^^^^^^^^^^ cannot always put folio here.

>  next:
>  		folio_walk_end(&fw, vma);
>  		cond_resched();
> -- 

Your code change is wrong. Because when you are using split_huge_page_to_list_to_order(),
the code pattern should be:

get_page(lock_at);
lock_page(lock_at);
split_huge_page_to_list_to_order(lock_at);
unlock_page(lock_at);
put_page(lock_at);

So the extra refcount in lock_at will be decreased by put_page(lock_at);

But your code change does not do put_page(lock_at) but always does folio_putback_lru(folio),
where folio is the original head.

BTW, in the folio world, I do not think it is possible to perform the aforementioned
split_huge_page_to_list_to_order() pattern any more, since you always work on folio,
the head. Unless there is a need of getting hold of a tail after-split folio after
a folio split, the pattern would be:

tail_page = folio_page(folio, N);

folio_get(folio);
folio_lock(folio);
folio_split(folio, ..., /* new parameter: lock_at = */ tail_page, ...);
tail_folio = page_folio(tail_page);
folio_unlock(tail_folio);
folio_put(tail_folio);



--
Best Regards,
Yan, Zi


^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2026-02-22  3:00 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20260222010425.gbsjzhrew3pg4qrw@master>
     [not found] ` <20260222010708.uohpmddmzaa4i4ic@master>
2026-02-22  3:00   ` A potential refcount issue during __folio_split Zi Yan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox