linux-kernel - Re: [PATCH 03/10] mm/ksm: use folio in try_to_merge_one

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f21b037b-07f4-41dc-acfc-1bd4de634511@redhat.com>
Date: Wed, 5 Jun 2024 09:50:38 +0200
From: David Hildenbrand <david@...hat.com>
To: Alex Shi <seakeel@...il.com>, alexs@...nel.org,
 Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, izik.eidus@...ellosystems.com,
 willy@...radead.org, aarcange@...hat.com, chrisw@...s-sol.org,
 hughd@...gle.com
Subject: Re: [PATCH 03/10] mm/ksm: use folio in try_to_merge_one_page

On 05.06.24 05:38, Alex Shi wrote:
> 
> 
> 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.

Okay, but the 2nd patch is wrong and should be dropped. We can easily 
end up with tail pages here.

We have to be a bit careful here.

-- 
Cheers,

David / dhildenb