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 [off-list ref] wrote:

quoted

* Shakeel Butt [off-list ref] [210525 19:45]:

quoted

On Tue, May 25, 2021 at 11:40 AM Liam Howlett [off-list ref] 

wrote:

quoted

quoted

quoted

[...]

quoted

quoted

+/*
+ * Walks the vma's mapping a page and mlocks the page if any locked 

vma's are

quoted

quoted

quoted

quoted

+ * found. Once one is found the page is locked and the scan can be 

terminated.

quoted

quoted

quoted

quoted

+ */

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?

quoted

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.

quoted

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.