Re: [RFC PATCH 1/3] mm, hugetlb: Clean up locking in hugetlb_fault and hugetlb_wp

[David Hildenbrand <david@redhat.com>]

>> As stated elsewhere, the mapcount check + folio_move_anon_rmap need the
>> folio lock.
> 
> Could you elaborate what would go wrong if we do folio_move_anon_rmap()
> without folio lock here?  Just to make sure we're on the same page: we
> already have pgtable lock held, and we decided to reuse an anonymous
> hugetlb page.

For now we have

VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);

right at the beginning of folio_move_anon_rmap().

That dates back to

commit c44b674323f4a2480dbeb65d4b487fa5f06f49e0
Author: Rik van Riel <riel@redhat.com>
Date:   Fri Mar 5 13:42:09 2010 -0800

     rmap: move exclusively owned pages to own anon_vma in do_wp_page()
     
     When the parent process breaks the COW on a page, both the original which
     is mapped at child and the new page which is mapped parent end up in that
     same anon_vma.  Generally this won't be a problem, but for some workloads
     it could preserve the O(N) rmap scanning complexity.
     
     A simple fix is to ensure that, when a page which is mapped child gets
     reused in do_wp_page, because we already are the exclusive owner, the page
     gets moved to our own exclusive child's anon_vma.


My recollection is that the folio lock protects folio->mapping. So relevant rmap walks
that hold the folio lock can assume that folio->mapping and
thereby folio_anon_vma() cannot change.

folio_lock_anon_vma_read() documents something regarding the folio lock protecting the
anon_vma.

I can only speculate that locking the folio is cheaper than locking the relevant anon_vma, and
that rmap code depends on that.


I'll note that in the introducing commit we didn't use the WRITE_ONCE, though. That was added in

commit 16f5e707d6f6f7644ff07e583b8f18c3dcc5499f
Author: Alex Shi <alexs@kernel.org>
Date:   Tue Dec 15 12:33:42 2020 -0800

     mm/rmap: stop store reordering issue on page->mapping

But I don't think that the folio lock was a replacement to that WRITE_ONCE.

-- 
Cheers,

David / dhildenb