Re: [PATCH v1 3/3] memory: move exclusivity detection in do_wp_page() into wp_can_reuse_anon_folio()

[David Hildenbrand <david@redhat.com>]

On 03.10.23 19:05, Suren Baghdasaryan wrote:
> On Mon, Oct 2, 2023 at 7:29 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> Let's clean up do_wp_page() a bit, removing two labels and making it
>> a easier to read.
>>
>> wp_can_reuse_anon_folio() now only operates on the whole folio. Move the
>> SetPageAnonExclusive() out into do_wp_page(). No need to do this under
>> page lock -- the page table lock is sufficient.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   mm/memory.c | 88 +++++++++++++++++++++++++++--------------------------
>>   1 file changed, 45 insertions(+), 43 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 1f0e3317cbdd..512f6f05620e 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3358,6 +3358,44 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
>>          return ret;
>>   }
>>

Sorry for the late response.

>> +static bool wp_can_reuse_anon_folio(struct folio *folio,
>> +                                   struct vm_area_struct *vma)
> 
> Since this function is calling folio_move_anon_rmap(), I would suggest
> changing its name to wp_reuse_anon_folio(). This would clarify that

folio_move_anon_rmap() is *not* the reuse part, it's just an rmap 
optimization. You could remove the call here and the whole thing would 
still work :) In fact, we can call folio_move_anon_rmap() whenever we 
sure the folio belongs to a single VMA only and we're holding the page 
lock. ... but we cannot always reuse a folio in that case because there 
might be GUP references from another process.

Reuse is
* Setting PageAnonExclusive
* Write fault: wunprotect the page -> wp_page_reuse()

I went a bit back and forth while cleaning that function up, but calling 
it wp_reuse_anon_folio() would end up being confusing with 
wp_page_reuse() called afterwards [we should probably rename that one to 
wp_page_wunprotect() independently]. So I prefer to leave the actual 
(sub)page reuse part in the caller.

> it's actually doing that operation instead of just checking if it's
> possible. That would also let us keep unconditional
> SetPageAnonExclusive() in it and do that under folio lock like it used
> to do (keeping rules simple). Other than that, it looks good to me.

I really want to avoid passing a "struct page" to that function; once 
we're dealing with PTE-mapped THP, the page might actually be a tail 
page of the folio.

Thanks!

-- 
Cheers,

David / dhildenb