lkml.org 
[lkml]   [2023]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v1 3/3] memory: move exclusivity detection in do_wp_page() into wp_can_reuse_anon_folio()
    On Mon, Oct 9, 2023 at 3:03 AM David Hildenbrand <david@redhat.com> wrote:
    >
    > 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()

    Ok, fair enough. It's not the reuse, only a preparation step. My
    concern is that wp_can_reuse_anon_folio() with a bool being returned
    looks like a function that only checks for a possibility of an
    operation while it's doing more than that. However I don't have a
    really good suggestion to improve the naming, so treat it as a nitpick
    and feel free to ignore.

    >
    > 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.

    Oh, I didn't realize that vmf->page and folio->page might differ in
    here. Ok, sounds reasonable.
    Thanks,
    Suren.

    >
    > Thanks!
    >
    > --
    > Cheers,
    >
    > David / dhildenb
    >

    \
     
     \ /
      Last update: 2023-10-09 18:40    [W:9.196 / U:0.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site