On Mon, Oct 9, 2023 at 3:03 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > > On 03.10.23 19:05, Suren Baghdasaryan wrote: > > On Mon, Oct 2, 2023 at 7:29 AM David Hildenbrand <david@xxxxxxxxxx> 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@xxxxxxxxxx> > >> --- > >> 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 >
