linux-kernel - Re: [PATCH] mm/rmap: vm_flags including VM

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] [day] [month] [year] [list]

Message-ID: <edd8fc72-7c94-4b9b-941c-7014c5f706ea@vivo.com>
Date:   Thu, 26 Oct 2023 10:04:19 +0800
From:   zhiguojiang <justinjiang@...o.com>
To:     David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Cc:     opensource.kernel@...o.com
Subject: Re: [PATCH] mm/rmap: vm_flags including VM_EXEC can exit timely



在 2023/10/25 23:04, David Hildenbrand 写道:
> On 25.10.23 05:04, zhiguojiang wrote:
>>
>>
>> 在 2023/10/25 9:17, zhiguojiang 写道:
>>>
>>>
>>> 在 2023/10/24 23:51, David Hildenbrand 写道:
>>>> On 24.10.23 16:49, Zhiguo Jiang wrote:
>>>>> When pra->vm_flags include VM_EXEC flag and folio is file detected in
>>>>> folio_referenced_one(), the folio referenced traversal process can be
>>>>> exited timely to reduce the detecting folio referenced time.
>>>>>
>>>>
>>>> Can you further elaborate what the logic behind that is?
>>>>
>>>> Why can we stop here if we're dealing with a pagecache folio in an
>>>> executable VMA?
>>>>
>>> Functions call flow：folio_referenced() --> rmap_walk() -->
>>> rmap_walk_ksm()/rmap_walk_anon()/rmap_walk_file() --> rwc->rmap_one()
>>> --> folio_referenced_one(). And folio_referenced() is called by two
>>> interfaces: folio_check_references() and shrink_active_list().
>>>
>>> 1. folio_check_references():
>>> When (referenced_ptes > 0 && (vm_flags & VM_EXEC) &&
>>> folio_is_file_lru(folio)) is detected in folio_check_references(),
>>> FOLIOREF_ACTIVATE will be returned and the folio will be added to the
>>> active file lru. So when VM_EXEC is detected in
>>> folio_referenced_one(), we can stop continuing to detect the reference
>>> relationship between this folio and other vmas, and exit directly to
>>> avoid unnecessary traversal.
>>>
>>> 2. shrink_active_list():
>>> The shrink_active_list() is the same as the folio_check_references().
>>>
>
> Thanks, that all belongs into the patch description in some condensed 
> form.
>
> Should that "(vm_flags & VM_EXEC) && folio_is_file_lru(folio)" in all 
> three places somehow be factored out in a function with a suitable 
> name, so all these cases can be identified easily? Just a thought.
>
> Then, add a comment to the code you're adding. There are plenty of 
> comments for the other two cases you mentioned.
>
>
> folio_referenced() documents:
>
>     "Quick test_and_clear_referenced for all mappings of a folio"
>
> IIUC, you're code will stop doing that, as you break in the middle
> of processing some mappings, but not all.
>
Thank you for review, this patch will indeed interrupt 
test_and_clear_referenced for all mappings of a folio. This patch can be 
ignore.
> Please describe why that is okay and add it to the patch description 
> and update the function description.
>
>>>>> Signed-off-by: Zhiguo Jiang <justinjiang@...o.com>
>>>>> ---
>>>>>    mm/rmap.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>    mode change 100644 => 100755 mm/rmap.c
>>>>>
>>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>>> index 7a27a2b41802..932f3b7e8521
>>>>> --- a/mm/rmap.c
>>>>> +++ b/mm/rmap.c
>>>>> @@ -884,6 +884,8 @@ static bool folio_referenced_one(struct folio
>>>>> *folio,
>>>>>        if (referenced) {
>>>>>            pra->referenced++;
>>>>>            pra->vm_flags |= vma->vm_flags & ~VM_LOCKED;
>>>>> +        if ((pra->vm_flags | VM_EXEC) && folio_is_file_lru(folio))
>>>>> +            return false;
>>>>>        }
>>>>>          if (!pra->mapcount)
>> Sorry, Patch mistake in writing, patch should be:
>> +        if ((pra->vm_flags & VM_EXEC) && folio_is_file_lru(folio))
>> +            return false;
>
> So this was not even properly tested? :/
>
> Of course I have to ask: what's the net (performance) benefit of this 
> change?
>