On Saturday, 5 June 2021 10:41:03 AM AEST Shakeel Butt wrote:
External email: Use caution opening links or attachments
On Fri, Jun 4, 2021 at 1:49 PM Liam Howlett liam.howlett@oracle.com wrote:
- Shakeel Butt shakeelb@google.com [210525 19:45]:
On Tue, May 25, 2021 at 11:40 AM Liam Howlett liam.howlett@oracle.com
wrote:
[...]
+/*
- Walks the vma's mapping a page and mlocks the page if any locked
vma's are
- found. Once one is found the page is locked and the scan can be
terminated.
- */
Can you please add that this requires the mmap_sem() lock to the comments?
Why does this require mmap_sem() lock? Also mmap_sem() lock of which
mm_struct?
Doesn't the mlock_vma_page() require the mmap_sem() for reading? The mm_struct in vma->vm_mm;
We are traversing all the vmas where this page is mapped of possibly different mm_structs. I don't think we want to take mmap_sem() of all those mm_structs. The commit b87537d9e2fe ("mm: rmap use pte lock not mmap_sem to set PageMlocked") removed exactly that.
From what I can see, at least the following paths have mmap_lock held for writing:
munlock_vma_pages_range() from __do_munmap() munlokc_vma_pages_range() from remap_file_pages()
The following path does not hold mmap_sem:
exit_mmap() -> munlock_vma_pages_all() -> munlock_vma_pages_range().
I would really suggest all to carefully read the commit message of b87537d9e2fe ("mm: rmap use pte lock not mmap_sem to set PageMlocked").
Particularly the following paragraph: ... Vlastimil Babka points out another race which this patch protects
against.
try_to_unmap_one() might reach its mlock_vma_page() TestSetPageMlocked
a
moment after munlock_vma_pages_all() did its Phase 1
TestClearPageMlocked:
leaving PageMlocked and unevictable when it should be evictable.
mmap_sem
is ineffective because exit_mmap() does not hold it; page lock
ineffective
because __munlock_pagevec() only takes it afterwards, in Phase 2; pte
lock
is effective because __munlock_pagevec_fill() takes it to get the page, after VM_LOCKED was cleared from vm_flags, so visible to
try_to_unmap_one.
...
Alistair, please bring back the VM_LOCKED check with pte lock held and the comment "Holding pte lock, we do *not* need mmap_lock here".
Actually thanks for highlighting that paragraph. I have gone back through the code again in munlock_vma_pages_range() and think I have a better understanding of it now. So now I agree - the check of VM_LOCKED under the PTL is important to ensure mlock_vma_page() does not run after VM_LOCKED has been cleared and __munlock_pagevec_fill() has run.
Will post v10 to fix this and the try_to_munlock reference pointed out by Liam which I missed for v9. Thanks Shakeel for taking the time to point this out.
One positive outcome of this cleanup patch is the removal of unnecessary invalidation (unmapping for kvm case) of secondary mmus.