[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4bf9f174-2d0e-554a-c01f-7188ccde0f3f@redhat.com>
Date: Mon, 9 Oct 2023 12:03:19 +0200
From: David Hildenbrand <david@...hat.com>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
Andrew Morton <akpm@...ux-foundation.org>,
Mike Kravetz <mike.kravetz@...cle.com>,
Muchun Song <muchun.song@...ux.dev>
Subject: Re: [PATCH v1 3/3] memory: move exclusivity detection in do_wp_page()
into wp_can_reuse_anon_folio()
On 03.10.23 19:05, Suren Baghdasaryan wrote:
> On Mon, Oct 2, 2023 at 7:29 AM David Hildenbrand <david@...hat.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@...hat.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
Powered by blists - more mailing lists