Received-SPF: pass (google.com: domain of linux-kernel+bounces-201725-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <2b2e209d-863a-4c21-b7a5-2d328866cd0e@gmail.com>
Date: Wed, 5 Jun 2024 11:38:23 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 03/10] mm/ksm: use folio in try_to_merge_one_page
To: David Hildenbrand <david@redhat.com>, alexs@kernel.org,
 Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
 linux-kernel@vger.kernel.org, izik.eidus@ravellosystems.com,
 willy@infradead.org, aarcange@redhat.com, chrisw@sous-sol.org,
 hughd@google.com
References: <20240604042454.2012091-1-alexs@kernel.org>
 <20240604042454.2012091-4-alexs@kernel.org>
 <8a4d048c-0764-442a-aa70-48518068f163@redhat.com>
Content-Language: en-US
From: Alex Shi <seakeel@gmail.com>
In-Reply-To: <8a4d048c-0764-442a-aa70-48518068f163@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On 6/4/24 4:19 PM, David Hildenbrand wrote:
>>
>> @@ -1489,11 +1494,11 @@ static int try_to_merge_one_page(struct vm_area_struct *vma, struct page *page,
>>        * prefer to continue scanning and merging different pages,
>>        * then come back to this page when it is unlocked.
>>        */
>> -    if (!trylock_page(page))
>> +    if (!folio_trylock(folio))
>>           goto out;
>>   -    if (PageTransCompound(page)) {
>> -        if (split_huge_page(page))
>> +    if (folio_test_large(folio)) {
>> +        if (split_folio(folio))
>>               goto out_unlock;
>>       }
> 
> Careful: there is a subtle change:
> 
> In the old code, after split_huge_page() succeeded, you would have a ref on *page* that you have to drop.
> 
> Now, you would have a ref on *folio* -- IOW the head page that calling code has to drop.
> 
> Is that handled accordingly? IOW, is there no code that would assume it would drop the reference on the *page* instead of on the *folio* (that, after split succeeds, would be an order-0 folio)
> 
> If so, worth spelling out in the description (you say "So in the calling path to try_to_merge_one_page() parameter pages are actually folios", but I am not sure if that means that all page refcounting code was changed to folio refcounting code).

Hi David,

Thanks a lot for comments. Yes, the ref on folio was handled. I will mention it again in commit log.
The 2nd patch in this series would just pass folios into next step, tail page will be warned, so the ref on 'page' here is also the ref on folio. they are same.


Thanks
Alex