| Message ID | 20210407084238.20443-4-apopple@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for SVM atomics in Nouveau | expand |
* Alistair Popple <apopple@nvidia.com> [210407 04:43]: > The behaviour of try_to_unmap_one() is difficult to follow because it > performs different operations based on a fairly large set of flags used > in different combinations. > > TTU_MUNLOCK is one such flag. However it is exclusively used by > try_to_munlock() which specifies no other flags. Therefore rather than > overload try_to_unmap_one() with unrelated behaviour split this out into > it's own function and remove the flag. > > Signed-off-by: Alistair Popple <apopple@nvidia.com> > Reviewed-by: Ralph Campbell <rcampbell@nvidia.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > > --- > > v8: > * Renamed try_to_munlock to page_mlock to better reflect what the > function actually does. > * Removed the TODO from the documentation that this patch addresses. > > v7: > * Added Christoph's Reviewed-by > > v4: > * Removed redundant check for VM_LOCKED > --- > Documentation/vm/unevictable-lru.rst | 33 ++++++++----------- > include/linux/rmap.h | 3 +- > mm/mlock.c | 10 +++--- > mm/rmap.c | 48 +++++++++++++++++++++------- > 4 files changed, 55 insertions(+), 39 deletions(-) > > diff --git a/Documentation/vm/unevictable-lru.rst b/Documentation/vm/unevictable-lru.rst > index 0e1490524f53..eae3af17f2d9 100644 > --- a/Documentation/vm/unevictable-lru.rst > +++ b/Documentation/vm/unevictable-lru.rst > @@ -389,14 +389,14 @@ mlocked, munlock_vma_page() updates that zone statistics for the number of > mlocked pages. Note, however, that at this point we haven't checked whether > the page is mapped by other VM_LOCKED VMAs. > > -We can't call try_to_munlock(), the function that walks the reverse map to > +We can't call page_mlock(), the function that walks the reverse map to > check for other VM_LOCKED VMAs, without first isolating the page from the LRU. > -try_to_munlock() is a variant of try_to_unmap() and thus requires that the page > +page_mlock() is a variant of try_to_unmap() and thus requires that the page > not be on an LRU list [more on these below]. However, the call to > -isolate_lru_page() could fail, in which case we couldn't try_to_munlock(). So, > +isolate_lru_page() could fail, in which case we can't call page_mlock(). So, > we go ahead and clear PG_mlocked up front, as this might be the only chance we > -have. If we can successfully isolate the page, we go ahead and > -try_to_munlock(), which will restore the PG_mlocked flag and update the zone > +have. If we can successfully isolate the page, we go ahead and call > +page_mlock(), which will restore the PG_mlocked flag and update the zone > page statistics if it finds another VMA holding the page mlocked. If we fail > to isolate the page, we'll have left a potentially mlocked page on the LRU. > This is fine, because we'll catch it later if and if vmscan tries to reclaim > @@ -545,31 +545,24 @@ munlock or munmap system calls, mm teardown (munlock_vma_pages_all), reclaim, > holepunching, and truncation of file pages and their anonymous COWed pages. > > > -try_to_munlock() Reverse Map Scan > +page_mlock() Reverse Map Scan > --------------------------------- > > -.. warning:: > - [!] TODO/FIXME: a better name might be page_mlocked() - analogous to the > - page_referenced() reverse map walker. > - > When munlock_vma_page() [see section :ref:`munlock()/munlockall() System Call > Handling <munlock_munlockall_handling>` above] tries to munlock a > page, it needs to determine whether or not the page is mapped by any > VM_LOCKED VMA without actually attempting to unmap all PTEs from the > page. For this purpose, the unevictable/mlock infrastructure > -introduced a variant of try_to_unmap() called try_to_munlock(). > +introduced a variant of try_to_unmap() called page_mlock(). > > -try_to_munlock() calls the same functions as try_to_unmap() for anonymous and > -mapped file and KSM pages with a flag argument specifying unlock versus unmap > -processing. Again, these functions walk the respective reverse maps looking > -for VM_LOCKED VMAs. When such a VMA is found, as in the try_to_unmap() case, > -the functions mlock the page via mlock_vma_page() and return SWAP_MLOCK. This > -undoes the pre-clearing of the page's PG_mlocked done by munlock_vma_page. > +page_mlock() walks the respective reverse maps looking for VM_LOCKED VMAs. When > +such a VMA is found the page is mlocked via mlock_vma_page(). This undoes the > +pre-clearing of the page's PG_mlocked done by munlock_vma_page. > > -Note that try_to_munlock()'s reverse map walk must visit every VMA in a page's > +Note that page_mlock()'s reverse map walk must visit every VMA in a page's > reverse map to determine that a page is NOT mapped into any VM_LOCKED VMA. > However, the scan can terminate when it encounters a VM_LOCKED VMA. > -Although try_to_munlock() might be called a great many times when munlocking a > +Although page_mlock() might be called a great many times when munlocking a > large region or tearing down a large address space that has been mlocked via > mlockall(), overall this is a fairly rare event. > > @@ -602,7 +595,7 @@ inactive lists to the appropriate node's unevictable list. > shrink_inactive_list() should only see SHM_LOCK'd pages that became SHM_LOCK'd > after shrink_active_list() had moved them to the inactive list, or pages mapped > into VM_LOCKED VMAs that munlock_vma_page() couldn't isolate from the LRU to > -recheck via try_to_munlock(). shrink_inactive_list() won't notice the latter, > +recheck via page_mlock(). shrink_inactive_list() won't notice the latter, > but will pass on to shrink_page_list(). > > shrink_page_list() again culls obviously unevictable pages that it could > diff --git a/include/linux/rmap.h b/include/linux/rmap.h > index def5c62c93b3..38a746787c2f 100644 > --- a/include/linux/rmap.h > +++ b/include/linux/rmap.h > @@ -87,7 +87,6 @@ struct anon_vma_chain { > > enum ttu_flags { > TTU_MIGRATION = 0x1, /* migration mode */ > - TTU_MUNLOCK = 0x2, /* munlock mode */ > > TTU_SPLIT_HUGE_PMD = 0x4, /* split huge PMD if any */ > TTU_IGNORE_MLOCK = 0x8, /* ignore mlock */ > @@ -239,7 +238,7 @@ int page_mkclean(struct page *); > * called in munlock()/munmap() path to check for other vmas holding > * the page mlocked. > */ > -void try_to_munlock(struct page *); > +void page_mlock(struct page *page); > > void remove_migration_ptes(struct page *old, struct page *new, bool locked); > > diff --git a/mm/mlock.c b/mm/mlock.c > index f8f8cc32d03d..9b8b82cfbbff 100644 > --- a/mm/mlock.c > +++ b/mm/mlock.c > @@ -108,7 +108,7 @@ void mlock_vma_page(struct page *page) > /* > * Finish munlock after successful page isolation > * > - * Page must be locked. This is a wrapper for try_to_munlock() > + * Page must be locked. This is a wrapper for page_mlock() > * and putback_lru_page() with munlock accounting. > */ > static void __munlock_isolated_page(struct page *page) > @@ -118,7 +118,7 @@ static void __munlock_isolated_page(struct page *page) > * and we don't need to check all the other vmas. > */ > if (page_mapcount(page) > 1) > - try_to_munlock(page); > + page_mlock(page); > > /* Did try_to_unlock() succeed or punt? */ > if (!PageMlocked(page)) > @@ -158,7 +158,7 @@ static void __munlock_isolation_failed(struct page *page) > * munlock()ed or munmap()ed, we want to check whether other vmas hold the > * page locked so that we can leave it on the unevictable lru list and not > * bother vmscan with it. However, to walk the page's rmap list in > - * try_to_munlock() we must isolate the page from the LRU. If some other > + * page_mlock() we must isolate the page from the LRU. If some other > * task has removed the page from the LRU, we won't be able to do that. > * So we clear the PageMlocked as we might not get another chance. If we > * can't isolate the page, we leave it for putback_lru_page() and vmscan > @@ -168,7 +168,7 @@ unsigned int munlock_vma_page(struct page *page) > { > int nr_pages; > > - /* For try_to_munlock() and to serialize with page migration */ > + /* For page_mlock() and to serialize with page migration */ > BUG_ON(!PageLocked(page)); > VM_BUG_ON_PAGE(PageTail(page), page); > > @@ -205,7 +205,7 @@ static int __mlock_posix_error_return(long retval) > * > * The fast path is available only for evictable pages with single mapping. > * Then we can bypass the per-cpu pvec and get better performance. > - * when mapcount > 1 we need try_to_munlock() which can fail. > + * when mapcount > 1 we need page_mlock() which can fail. > * when !page_evictable(), we need the full redo logic of putback_lru_page to > * avoid leaving evictable page in unevictable list. > * > diff --git a/mm/rmap.c b/mm/rmap.c > index 977e70803ed8..f09d522725b9 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1405,10 +1405,6 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, > struct mmu_notifier_range range; > enum ttu_flags flags = (enum ttu_flags)(long)arg; > > - /* munlock has nothing to gain from examining un-locked vmas */ > - if ((flags & TTU_MUNLOCK) && !(vma->vm_flags & VM_LOCKED)) > - return true; > - > if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) && > is_zone_device_page(page) && !is_device_private_page(page)) > return true; > @@ -1469,8 +1465,6 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, > page_vma_mapped_walk_done(&pvmw); > break; > } > - if (flags & TTU_MUNLOCK) > - continue; > } > > /* Unexpected PMD-mapped THP? */ > @@ -1784,8 +1778,39 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags) > return !page_mapcount(page) ? true : false; > } > Please add a comment here, especially around locking. > +static bool page_mlock_one(struct page *page, struct vm_area_struct *vma, > + unsigned long address, void *arg) > +{ > + struct page_vma_mapped_walk pvmw = { > + .page = page, > + .vma = vma, > + .address = address, > + }; > + > + /* munlock has nothing to gain from examining un-locked vmas */ > + if (!(vma->vm_flags & VM_LOCKED)) > + return true; The logic here doesn't make sense. You called page_mlock_one() on a VMA that isn't locked and it returns true? Is this a check to see if the VMA has zero mlock'ed pages? > + > + while (page_vma_mapped_walk(&pvmw)) { > + /* PTE-mapped THP are never mlocked */ > + if (!PageTransCompound(page)) { > + /* > + * Holding pte lock, we do *not* need > + * mmap_lock here > + */ Are you sure? I think you at least need to hold the mmap lock for reading to ensure there's no race here? mlock_vma_page() eludes to such a scenario when lazy mlocking. The mmap_lock is held for writing in the scenarios I have checked. > + mlock_vma_page(page); > + } > + page_vma_mapped_walk_done(&pvmw); > + > + /* found a mlocked page, no point continuing munlock check */ I think you need to check_pte() to be sure it is mapped? > + return false; > + } > + > + return true; Again, I don't get the return values. If page_mlock_one() returns true, I'd expect for my page to now be locked. This isn't the case here, page_mlock_one() returns true if there are no pages present for a locked VMA, correct? > +} > + > /** > - * try_to_munlock - try to munlock a page > + * page_mlock - try to munlock a page Is this an mlock or an munlock? I'm not confident it's either, but more of a check to see if there are pages mapped in a locked VMA? > * @page: the page to be munlocked > * > * Called from munlock code. Checks all of the VMAs mapping the page > @@ -1793,11 +1818,10 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags) > * returned with PG_mlocked cleared if no other vmas have it mlocked. > */ > > -void try_to_munlock(struct page *page) > +void page_mlock(struct page *page) > { > struct rmap_walk_control rwc = { > - .rmap_one = try_to_unmap_one, > - .arg = (void *)TTU_MUNLOCK, > + .rmap_one = page_mlock_one, > .done = page_not_mapped, > .anon_lock = page_lock_anon_vma_read, > > @@ -1849,7 +1873,7 @@ static struct anon_vma *rmap_walk_anon_lock(struct page *page, > * Find all the mappings of a page using the mapping pointer and the vma chains > * contained in the anon_vma struct it points to. > * > - * When called from try_to_munlock(), the mmap_lock of the mm containing the vma > + * When called from page_mlock(), the mmap_lock of the mm containing the vma > * where the page was found will be held for write. So, we won't recheck > * vm_flags for that VMA. That should be OK, because that vma shouldn't be > * LOCKED. > @@ -1901,7 +1925,7 @@ static void rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc, > * Find all the mappings of a page using the mapping pointer and the vma chains > * contained in the address_space struct it points to. > * > - * When called from try_to_munlock(), the mmap_lock of the mm containing the vma > + * When called from page_mlock(), the mmap_lock of the mm containing the vma > * where the page was found will be held for write. So, we won't recheck Above it is stated that the lock does not need to be held, but this comment says it is already held for writing - which is it? > * vm_flags for that VMA. That should be OK, because that vma shouldn't be > * LOCKED. > -- > 2.20.1 > munlock_vma_pages_range() comments references try_to_{munlock|unmap}
On Wednesday, 19 May 2021 6:04:51 AM AEST Liam Howlett wrote: > External email: Use caution opening links or attachments > > * Alistair Popple <apopple@nvidia.com> [210407 04:43]: > > The behaviour of try_to_unmap_one() is difficult to follow because it > > performs different operations based on a fairly large set of flags used > > in different combinations. > > > > TTU_MUNLOCK is one such flag. However it is exclusively used by > > try_to_munlock() which specifies no other flags. Therefore rather than > > overload try_to_unmap_one() with unrelated behaviour split this out into > > it's own function and remove the flag. > > > > Signed-off-by: Alistair Popple <apopple@nvidia.com> > > Reviewed-by: Ralph Campbell <rcampbell@nvidia.com> > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > --- > > > > v8: > > * Renamed try_to_munlock to page_mlock to better reflect what the > > > > function actually does. > > > > * Removed the TODO from the documentation that this patch addresses. > > > > v7: > > * Added Christoph's Reviewed-by > > > > v4: > > * Removed redundant check for VM_LOCKED > > --- > > > > Documentation/vm/unevictable-lru.rst | 33 ++++++++----------- > > include/linux/rmap.h | 3 +- > > mm/mlock.c | 10 +++--- > > mm/rmap.c | 48 +++++++++++++++++++++------- > > 4 files changed, 55 insertions(+), 39 deletions(-) > > > > diff --git a/Documentation/vm/unevictable-lru.rst > > b/Documentation/vm/unevictable-lru.rst index 0e1490524f53..eae3af17f2d9 > > 100644 > > --- a/Documentation/vm/unevictable-lru.rst > > +++ b/Documentation/vm/unevictable-lru.rst > > @@ -389,14 +389,14 @@ mlocked, munlock_vma_page() updates that zone > > statistics for the number of> > > mlocked pages. Note, however, that at this point we haven't checked > > whether the page is mapped by other VM_LOCKED VMAs. > > > > -We can't call try_to_munlock(), the function that walks the reverse map > > to > > +We can't call page_mlock(), the function that walks the reverse map to > > > > check for other VM_LOCKED VMAs, without first isolating the page from the > > LRU.> > > -try_to_munlock() is a variant of try_to_unmap() and thus requires that > > the page +page_mlock() is a variant of try_to_unmap() and thus requires > > that the page> > > not be on an LRU list [more on these below]. However, the call to > > > > -isolate_lru_page() could fail, in which case we couldn't > > try_to_munlock(). So, +isolate_lru_page() could fail, in which case we > > can't call page_mlock(). So,> > > we go ahead and clear PG_mlocked up front, as this might be the only > > chance we> > > -have. If we can successfully isolate the page, we go ahead and > > -try_to_munlock(), which will restore the PG_mlocked flag and update the > > zone +have. If we can successfully isolate the page, we go ahead and > > call +page_mlock(), which will restore the PG_mlocked flag and update the > > zone> > > page statistics if it finds another VMA holding the page mlocked. If we > > fail to isolate the page, we'll have left a potentially mlocked page on > > the LRU. This is fine, because we'll catch it later if and if vmscan > > tries to reclaim> > > @@ -545,31 +545,24 @@ munlock or munmap system calls, mm teardown > > (munlock_vma_pages_all), reclaim,> > > holepunching, and truncation of file pages and their anonymous COWed > > pages. > > > > -try_to_munlock() Reverse Map Scan > > +page_mlock() Reverse Map Scan > > > > --------------------------------- > > > > -.. warning:: > > - [!] TODO/FIXME: a better name might be page_mlocked() - analogous to > > the - page_referenced() reverse map walker. > > - > > > > When munlock_vma_page() [see section :ref:`munlock()/munlockall() System > > Call Handling <munlock_munlockall_handling>` above] tries to munlock a > > page, it needs to determine whether or not the page is mapped by any > > VM_LOCKED VMA without actually attempting to unmap all PTEs from the > > page. For this purpose, the unevictable/mlock infrastructure > > > > -introduced a variant of try_to_unmap() called try_to_munlock(). > > +introduced a variant of try_to_unmap() called page_mlock(). > > > > -try_to_munlock() calls the same functions as try_to_unmap() for anonymous > > and -mapped file and KSM pages with a flag argument specifying unlock > > versus unmap -processing. Again, these functions walk the respective > > reverse maps looking -for VM_LOCKED VMAs. When such a VMA is found, as > > in the try_to_unmap() case, -the functions mlock the page via > > mlock_vma_page() and return SWAP_MLOCK. This -undoes the pre-clearing of > > the page's PG_mlocked done by munlock_vma_page. +page_mlock() walks the > > respective reverse maps looking for VM_LOCKED VMAs. When +such a VMA is > > found the page is mlocked via mlock_vma_page(). This undoes the > > +pre-clearing of the page's PG_mlocked done by munlock_vma_page. > > > > -Note that try_to_munlock()'s reverse map walk must visit every VMA in a > > page's +Note that page_mlock()'s reverse map walk must visit every VMA in > > a page's> > > reverse map to determine that a page is NOT mapped into any VM_LOCKED > > VMA. > > However, the scan can terminate when it encounters a VM_LOCKED VMA. > > > > -Although try_to_munlock() might be called a great many times when > > munlocking a +Although page_mlock() might be called a great many times > > when munlocking a> > > large region or tearing down a large address space that has been mlocked > > via mlockall(), overall this is a fairly rare event. > > > > @@ -602,7 +595,7 @@ inactive lists to the appropriate node's unevictable > > list.> > > shrink_inactive_list() should only see SHM_LOCK'd pages that became > > SHM_LOCK'd after shrink_active_list() had moved them to the inactive > > list, or pages mapped into VM_LOCKED VMAs that munlock_vma_page() > > couldn't isolate from the LRU to> > > -recheck via try_to_munlock(). shrink_inactive_list() won't notice the > > latter, +recheck via page_mlock(). shrink_inactive_list() won't notice > > the latter,> > > but will pass on to shrink_page_list(). > > > > shrink_page_list() again culls obviously unevictable pages that it could > > > > diff --git a/include/linux/rmap.h b/include/linux/rmap.h > > index def5c62c93b3..38a746787c2f 100644 > > --- a/include/linux/rmap.h > > +++ b/include/linux/rmap.h > > @@ -87,7 +87,6 @@ struct anon_vma_chain { > > > > enum ttu_flags { > > > > TTU_MIGRATION = 0x1, /* migration mode */ > > > > - TTU_MUNLOCK = 0x2, /* munlock mode */ > > > > TTU_SPLIT_HUGE_PMD = 0x4, /* split huge PMD if any */ > > TTU_IGNORE_MLOCK = 0x8, /* ignore mlock */ > > > > @@ -239,7 +238,7 @@ int page_mkclean(struct page *); > > > > * called in munlock()/munmap() path to check for other vmas holding > > * the page mlocked. > > */ > > > > -void try_to_munlock(struct page *); > > +void page_mlock(struct page *page); > > > > void remove_migration_ptes(struct page *old, struct page *new, bool > > locked);> > > diff --git a/mm/mlock.c b/mm/mlock.c > > index f8f8cc32d03d..9b8b82cfbbff 100644 > > --- a/mm/mlock.c > > +++ b/mm/mlock.c > > @@ -108,7 +108,7 @@ void mlock_vma_page(struct page *page) > > > > /* > > > > * Finish munlock after successful page isolation > > * > > > > - * Page must be locked. This is a wrapper for try_to_munlock() > > + * Page must be locked. This is a wrapper for page_mlock() > > > > * and putback_lru_page() with munlock accounting. > > */ > > > > static void __munlock_isolated_page(struct page *page) > > > > @@ -118,7 +118,7 @@ static void __munlock_isolated_page(struct page *page) > > > > * and we don't need to check all the other vmas. > > */ > > > > if (page_mapcount(page) > 1) > > > > - try_to_munlock(page); > > + page_mlock(page); > > > > /* Did try_to_unlock() succeed or punt? */ > > if (!PageMlocked(page)) > > > > @@ -158,7 +158,7 @@ static void __munlock_isolation_failed(struct page > > *page)> > > * munlock()ed or munmap()ed, we want to check whether other vmas hold > > the > > * page locked so that we can leave it on the unevictable lru list and > > not > > * bother vmscan with it. However, to walk the page's rmap list in > > > > - * try_to_munlock() we must isolate the page from the LRU. If some other > > + * page_mlock() we must isolate the page from the LRU. If some other > > > > * task has removed the page from the LRU, we won't be able to do that. > > * So we clear the PageMlocked as we might not get another chance. If we > > * can't isolate the page, we leave it for putback_lru_page() and vmscan > > > > @@ -168,7 +168,7 @@ unsigned int munlock_vma_page(struct page *page) > > > > { > > > > int nr_pages; > > > > - /* For try_to_munlock() and to serialize with page migration */ > > + /* For page_mlock() and to serialize with page migration */ > > > > BUG_ON(!PageLocked(page)); > > VM_BUG_ON_PAGE(PageTail(page), page); > > > > @@ -205,7 +205,7 @@ static int __mlock_posix_error_return(long retval) > > > > * > > * The fast path is available only for evictable pages with single > > mapping. > > * Then we can bypass the per-cpu pvec and get better performance. > > > > - * when mapcount > 1 we need try_to_munlock() which can fail. > > + * when mapcount > 1 we need page_mlock() which can fail. > > > > * when !page_evictable(), we need the full redo logic of > > putback_lru_page to * avoid leaving evictable page in unevictable list. > > * > > > > diff --git a/mm/rmap.c b/mm/rmap.c > > index 977e70803ed8..f09d522725b9 100644 > > --- a/mm/rmap.c > > +++ b/mm/rmap.c > > @@ -1405,10 +1405,6 @@ static bool try_to_unmap_one(struct page *page, > > struct vm_area_struct *vma,> > > struct mmu_notifier_range range; > > enum ttu_flags flags = (enum ttu_flags)(long)arg; > > > > - /* munlock has nothing to gain from examining un-locked vmas */ > > - if ((flags & TTU_MUNLOCK) && !(vma->vm_flags & VM_LOCKED)) > > - return true; > > - > > > > if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) && > > > > is_zone_device_page(page) && !is_device_private_page(page)) > > > > return true; > > > > @@ -1469,8 +1465,6 @@ static bool try_to_unmap_one(struct page *page, > > struct vm_area_struct *vma,> > > page_vma_mapped_walk_done(&pvmw); > > break; > > > > } > > > > - if (flags & TTU_MUNLOCK) > > - continue; > > > > } > > > > /* Unexpected PMD-mapped THP? */ > > > > @@ -1784,8 +1778,39 @@ bool try_to_unmap(struct page *page, enum ttu_flags > > flags)> > > return !page_mapcount(page) ? true : false; > > > > } > > Please add a comment here, especially around locking. > > > +static bool page_mlock_one(struct page *page, struct vm_area_struct *vma, > > + unsigned long address, void *arg) > > +{ > > + struct page_vma_mapped_walk pvmw = { > > + .page = page, > > + .vma = vma, > > + .address = address, > > + }; > > + > > + /* munlock has nothing to gain from examining un-locked vmas */ > > + if (!(vma->vm_flags & VM_LOCKED)) > > + return true; > > The logic here doesn't make sense. You called page_mlock_one() on a VMA > that isn't locked and it returns true? Is this a check to see if the > VMA has zero mlock'ed pages? I'm pretty sure the logic is correct. This is used for an rmap_walk, so we return true to continue the page table scan to see if other VMAs have the page locked. > > + > > + while (page_vma_mapped_walk(&pvmw)) { > > + /* PTE-mapped THP are never mlocked */ > > + if (!PageTransCompound(page)) { > > + /* > > + * Holding pte lock, we do *not* need > > + * mmap_lock here > > + */ > > Are you sure? I think you at least need to hold the mmap lock for > reading to ensure there's no race here? mlock_vma_page() eludes to such > a scenario when lazy mlocking. Not really. I don't claim to be an mlock expert but as this is a clean-up for try_to_unmap() the intent was to not change existing behaviour. However presenting the function in this simplified form did raise this and some other questions during previous reviews - see https://lore.kernel.org/ dri-devel/20210331115746.GA1463678@nvidia.com/ for the previous discussion. To answer the questions around locking though I did do some git sha1 mining. The best explanation seemed to be contained in https://git.kernel.org/pub/scm/ linux/kernel/git/torvalds/linux.git/commit/? id=b87537d9e2feb30f6a962f27eb32768682698d3b from Hugh (whom I've added again here in case he can help answer some of these). > The mmap_lock is held for writing in the scenarios I have checked. > > > + mlock_vma_page(page); > > + } > > + page_vma_mapped_walk_done(&pvmw); > > + > > + /* found a mlocked page, no point continuing munlock check > > */ > > I think you need to check_pte() to be sure it is mapped? > > > + return false; > > + } > > + > > + return true; > > Again, I don't get the return values. If page_mlock_one() returns true, > I'd expect for my page to now be locked. This isn't the case here, > page_mlock_one() returns true if there are no pages present for a locked > VMA, correct? This is used for an rmap walk, if we were able to mlock the page we return false to stop the walk as there is no need to examine other VMAs if the page has already been mlocked. > > +} > > + > > > > /** > > > > - * try_to_munlock - try to munlock a page > > + * page_mlock - try to munlock a page > > Is this an mlock or an munlock? I'm not confident it's either, but more > of a check to see if there are pages mapped in a locked VMA? It is called (only) from the munlock code to check a page does not need to be mlocked, or to mlock it if it does. > > * @page: the page to be munlocked > > * > > * Called from munlock code. Checks all of the VMAs mapping the page > > > > @@ -1793,11 +1818,10 @@ bool try_to_unmap(struct page *page, enum > > ttu_flags flags)> > > * returned with PG_mlocked cleared if no other vmas have it mlocked. > > */ > > > > -void try_to_munlock(struct page *page) > > +void page_mlock(struct page *page) > > > > { > > > > struct rmap_walk_control rwc = { > > > > - .rmap_one = try_to_unmap_one, > > - .arg = (void *)TTU_MUNLOCK, > > + .rmap_one = page_mlock_one, > > > > .done = page_not_mapped, > > .anon_lock = page_lock_anon_vma_read, > > > > @@ -1849,7 +1873,7 @@ static struct anon_vma *rmap_walk_anon_lock(struct > > page *page,> > > * Find all the mappings of a page using the mapping pointer and the vma > > chains * contained in the anon_vma struct it points to. > > * > > > > - * When called from try_to_munlock(), the mmap_lock of the mm containing > > the vma + * When called from page_mlock(), the mmap_lock of the mm > > containing the vma> > > * where the page was found will be held for write. So, we won't recheck > > * vm_flags for that VMA. That should be OK, because that vma shouldn't > > be > > * LOCKED. > > > > @@ -1901,7 +1925,7 @@ static void rmap_walk_anon(struct page *page, struct > > rmap_walk_control *rwc,> > > * Find all the mappings of a page using the mapping pointer and the vma > > chains * contained in the address_space struct it points to. > > * > > > > - * When called from try_to_munlock(), the mmap_lock of the mm containing > > the vma + * When called from page_mlock(), the mmap_lock of the mm > > containing the vma> > > * where the page was found will be held for write. So, we won't recheck > > Above it is stated that the lock does not need to be held, but this > comment says it is already held for writing - which is it? I will have to check. > > * vm_flags for that VMA. That should be OK, because that vma shouldn't > > be > > * LOCKED. > > > > -- > > 2.20.1 > > munlock_vma_pages_range() comments references try_to_{munlock|unmap} Thanks, I noticed that too when I was rereading it just now. Will fix that up. - Alistair
* Alistair Popple <apopple@nvidia.com> [210519 08:38]: > On Wednesday, 19 May 2021 6:04:51 AM AEST Liam Howlett wrote: > > External email: Use caution opening links or attachments > > > > * Alistair Popple <apopple@nvidia.com> [210407 04:43]: > > > The behaviour of try_to_unmap_one() is difficult to follow because it > > > performs different operations based on a fairly large set of flags used > > > in different combinations. > > > > > > TTU_MUNLOCK is one such flag. However it is exclusively used by > > > try_to_munlock() which specifies no other flags. Therefore rather than > > > overload try_to_unmap_one() with unrelated behaviour split this out into > > > it's own function and remove the flag. > > > > > > Signed-off-by: Alistair Popple <apopple@nvidia.com> > > > Reviewed-by: Ralph Campbell <rcampbell@nvidia.com> > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > > > --- > > > > > > v8: > > > * Renamed try_to_munlock to page_mlock to better reflect what the > > > > > > function actually does. > > > > > > * Removed the TODO from the documentation that this patch addresses. > > > > > > v7: > > > * Added Christoph's Reviewed-by > > > > > > v4: > > > * Removed redundant check for VM_LOCKED > > > --- > > > > > > Documentation/vm/unevictable-lru.rst | 33 ++++++++----------- > > > include/linux/rmap.h | 3 +- > > > mm/mlock.c | 10 +++--- > > > mm/rmap.c | 48 +++++++++++++++++++++------- > > > 4 files changed, 55 insertions(+), 39 deletions(-) > > > > > > diff --git a/Documentation/vm/unevictable-lru.rst > > > b/Documentation/vm/unevictable-lru.rst index 0e1490524f53..eae3af17f2d9 > > > 100644 > > > --- a/Documentation/vm/unevictable-lru.rst > > > +++ b/Documentation/vm/unevictable-lru.rst > > > @@ -389,14 +389,14 @@ mlocked, munlock_vma_page() updates that zone > > > statistics for the number of> > > > mlocked pages. Note, however, that at this point we haven't checked > > > whether the page is mapped by other VM_LOCKED VMAs. > > > > > > -We can't call try_to_munlock(), the function that walks the reverse map > > > to > > > +We can't call page_mlock(), the function that walks the reverse map to > > > > > > check for other VM_LOCKED VMAs, without first isolating the page from the > > > LRU.> > > > -try_to_munlock() is a variant of try_to_unmap() and thus requires that > > > the page +page_mlock() is a variant of try_to_unmap() and thus requires > > > that the page> > > > not be on an LRU list [more on these below]. However, the call to > > > > > > -isolate_lru_page() could fail, in which case we couldn't > > > try_to_munlock(). So, +isolate_lru_page() could fail, in which case we > > > can't call page_mlock(). So,> > > > we go ahead and clear PG_mlocked up front, as this might be the only > > > chance we> > > > -have. If we can successfully isolate the page, we go ahead and > > > -try_to_munlock(), which will restore the PG_mlocked flag and update the > > > zone +have. If we can successfully isolate the page, we go ahead and > > > call +page_mlock(), which will restore the PG_mlocked flag and update the > > > zone> > > > page statistics if it finds another VMA holding the page mlocked. If we > > > fail to isolate the page, we'll have left a potentially mlocked page on > > > the LRU. This is fine, because we'll catch it later if and if vmscan > > > tries to reclaim> > > > @@ -545,31 +545,24 @@ munlock or munmap system calls, mm teardown > > > (munlock_vma_pages_all), reclaim,> > > > holepunching, and truncation of file pages and their anonymous COWed > > > pages. > > > > > > -try_to_munlock() Reverse Map Scan > > > +page_mlock() Reverse Map Scan > > > > > > --------------------------------- > > > > > > -.. warning:: > > > - [!] TODO/FIXME: a better name might be page_mlocked() - analogous to > > > the - page_referenced() reverse map walker. > > > - > > > > > > When munlock_vma_page() [see section :ref:`munlock()/munlockall() System > > > Call Handling <munlock_munlockall_handling>` above] tries to munlock a > > > page, it needs to determine whether or not the page is mapped by any > > > VM_LOCKED VMA without actually attempting to unmap all PTEs from the > > > page. For this purpose, the unevictable/mlock infrastructure > > > > > > -introduced a variant of try_to_unmap() called try_to_munlock(). > > > +introduced a variant of try_to_unmap() called page_mlock(). > > > > > > -try_to_munlock() calls the same functions as try_to_unmap() for anonymous > > > and -mapped file and KSM pages with a flag argument specifying unlock > > > versus unmap -processing. Again, these functions walk the respective > > > reverse maps looking -for VM_LOCKED VMAs. When such a VMA is found, as > > > in the try_to_unmap() case, -the functions mlock the page via > > > mlock_vma_page() and return SWAP_MLOCK. This -undoes the pre-clearing of > > > the page's PG_mlocked done by munlock_vma_page. +page_mlock() walks the > > > respective reverse maps looking for VM_LOCKED VMAs. When +such a VMA is > > > found the page is mlocked via mlock_vma_page(). This undoes the > > > +pre-clearing of the page's PG_mlocked done by munlock_vma_page. > > > > > > -Note that try_to_munlock()'s reverse map walk must visit every VMA in a > > > page's +Note that page_mlock()'s reverse map walk must visit every VMA in > > > a page's> > > > reverse map to determine that a page is NOT mapped into any VM_LOCKED > > > VMA. > > > However, the scan can terminate when it encounters a VM_LOCKED VMA. > > > > > > -Although try_to_munlock() might be called a great many times when > > > munlocking a +Although page_mlock() might be called a great many times > > > when munlocking a> > > > large region or tearing down a large address space that has been mlocked > > > via mlockall(), overall this is a fairly rare event. > > > > > > @@ -602,7 +595,7 @@ inactive lists to the appropriate node's unevictable > > > list.> > > > shrink_inactive_list() should only see SHM_LOCK'd pages that became > > > SHM_LOCK'd after shrink_active_list() had moved them to the inactive > > > list, or pages mapped into VM_LOCKED VMAs that munlock_vma_page() > > > couldn't isolate from the LRU to> > > > -recheck via try_to_munlock(). shrink_inactive_list() won't notice the > > > latter, +recheck via page_mlock(). shrink_inactive_list() won't notice > > > the latter,> > > > but will pass on to shrink_page_list(). > > > > > > shrink_page_list() again culls obviously unevictable pages that it could > > > > > > diff --git a/include/linux/rmap.h b/include/linux/rmap.h > > > index def5c62c93b3..38a746787c2f 100644 > > > --- a/include/linux/rmap.h > > > +++ b/include/linux/rmap.h > > > @@ -87,7 +87,6 @@ struct anon_vma_chain { > > > > > > enum ttu_flags { > > > > > > TTU_MIGRATION = 0x1, /* migration mode */ > > > > > > - TTU_MUNLOCK = 0x2, /* munlock mode */ > > > > > > TTU_SPLIT_HUGE_PMD = 0x4, /* split huge PMD if any */ > > > TTU_IGNORE_MLOCK = 0x8, /* ignore mlock */ > > > > > > @@ -239,7 +238,7 @@ int page_mkclean(struct page *); > > > > > > * called in munlock()/munmap() path to check for other vmas holding > > > * the page mlocked. > > > */ > > > > > > -void try_to_munlock(struct page *); > > > +void page_mlock(struct page *page); > > > > > > void remove_migration_ptes(struct page *old, struct page *new, bool > > > locked);> > > > diff --git a/mm/mlock.c b/mm/mlock.c > > > index f8f8cc32d03d..9b8b82cfbbff 100644 > > > --- a/mm/mlock.c > > > +++ b/mm/mlock.c > > > @@ -108,7 +108,7 @@ void mlock_vma_page(struct page *page) > > > > > > /* > > > > > > * Finish munlock after successful page isolation > > > * > > > > > > - * Page must be locked. This is a wrapper for try_to_munlock() > > > + * Page must be locked. This is a wrapper for page_mlock() > > > > > > * and putback_lru_page() with munlock accounting. > > > */ > > > > > > static void __munlock_isolated_page(struct page *page) > > > > > > @@ -118,7 +118,7 @@ static void __munlock_isolated_page(struct page *page) > > > > > > * and we don't need to check all the other vmas. > > > */ > > > > > > if (page_mapcount(page) > 1) > > > > > > - try_to_munlock(page); > > > + page_mlock(page); > > > > > > /* Did try_to_unlock() succeed or punt? */ > > > if (!PageMlocked(page)) > > > > > > @@ -158,7 +158,7 @@ static void __munlock_isolation_failed(struct page > > > *page)> > > > * munlock()ed or munmap()ed, we want to check whether other vmas hold > > > the > > > * page locked so that we can leave it on the unevictable lru list and > > > not > > > * bother vmscan with it. However, to walk the page's rmap list in > > > > > > - * try_to_munlock() we must isolate the page from the LRU. If some other > > > + * page_mlock() we must isolate the page from the LRU. If some other > > > > > > * task has removed the page from the LRU, we won't be able to do that. > > > * So we clear the PageMlocked as we might not get another chance. If we > > > * can't isolate the page, we leave it for putback_lru_page() and vmscan > > > > > > @@ -168,7 +168,7 @@ unsigned int munlock_vma_page(struct page *page) > > > > > > { > > > > > > int nr_pages; > > > > > > - /* For try_to_munlock() and to serialize with page migration */ > > > + /* For page_mlock() and to serialize with page migration */ > > > > > > BUG_ON(!PageLocked(page)); > > > VM_BUG_ON_PAGE(PageTail(page), page); > > > > > > @@ -205,7 +205,7 @@ static int __mlock_posix_error_return(long retval) > > > > > > * > > > * The fast path is available only for evictable pages with single > > > mapping. > > > * Then we can bypass the per-cpu pvec and get better performance. > > > > > > - * when mapcount > 1 we need try_to_munlock() which can fail. > > > + * when mapcount > 1 we need page_mlock() which can fail. > > > > > > * when !page_evictable(), we need the full redo logic of > > > putback_lru_page to * avoid leaving evictable page in unevictable list. > > > * > > > > > > diff --git a/mm/rmap.c b/mm/rmap.c > > > index 977e70803ed8..f09d522725b9 100644 > > > --- a/mm/rmap.c > > > +++ b/mm/rmap.c > > > @@ -1405,10 +1405,6 @@ static bool try_to_unmap_one(struct page *page, > > > struct vm_area_struct *vma,> > > > struct mmu_notifier_range range; > > > enum ttu_flags flags = (enum ttu_flags)(long)arg; > > > > > > - /* munlock has nothing to gain from examining un-locked vmas */ > > > - if ((flags & TTU_MUNLOCK) && !(vma->vm_flags & VM_LOCKED)) > > > - return true; > > > - > > > > > > if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) && > > > > > > is_zone_device_page(page) && !is_device_private_page(page)) > > > > > > return true; > > > > > > @@ -1469,8 +1465,6 @@ static bool try_to_unmap_one(struct page *page, > > > struct vm_area_struct *vma,> > > > page_vma_mapped_walk_done(&pvmw); > > > break; > > > > > > } > > > > > > - if (flags & TTU_MUNLOCK) > > > - continue; > > > > > > } > > > > > > /* Unexpected PMD-mapped THP? */ > > > > > > @@ -1784,8 +1778,39 @@ bool try_to_unmap(struct page *page, enum ttu_flags > > > flags)> > > > return !page_mapcount(page) ? true : false; > > > > > > } > > > > Please add a comment here, especially around locking. Did you miss this comment? I think the name confusion alone means this needs some documentation. It's also worth mentioning arg is unused. > > > > > +static bool page_mlock_one(struct page *page, struct vm_area_struct *vma, > > > + unsigned long address, void *arg) > > > +{ > > > + struct page_vma_mapped_walk pvmw = { > > > + .page = page, > > > + .vma = vma, > > > + .address = address, > > > + }; > > > + > > > + /* munlock has nothing to gain from examining un-locked vmas */ > > > + if (!(vma->vm_flags & VM_LOCKED)) > > > + return true; > > > > The logic here doesn't make sense. You called page_mlock_one() on a VMA > > that isn't locked and it returns true? Is this a check to see if the > > VMA has zero mlock'ed pages? > > I'm pretty sure the logic is correct. This is used for an rmap_walk, so we > return true to continue the page table scan to see if other VMAs have the page > locked. yes, sorry. The logic is correct but doesn't read as though it does. I cannot see what is going on easily and there are no comments stating what is happening. > > > > + > > > + while (page_vma_mapped_walk(&pvmw)) { > > > + /* PTE-mapped THP are never mlocked */ > > > + if (!PageTransCompound(page)) { > > > + /* > > > + * Holding pte lock, we do *not* need > > > + * mmap_lock here > > > + */ > > > > Are you sure? I think you at least need to hold the mmap lock for > > reading to ensure there's no race here? mlock_vma_page() eludes to such > > a scenario when lazy mlocking. > > Not really. I don't claim to be an mlock expert but as this is a clean-up for > try_to_unmap() the intent was to not change existing behaviour. > > However presenting the function in this simplified form did raise this and > some other questions during previous reviews - see https://lore.kernel.org/ > dri-devel/20210331115746.GA1463678@nvidia.com/ for the previous discussion. 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() > > To answer the questions around locking though I did do some git sha1 mining. > The best explanation seemed to be contained in https://git.kernel.org/pub/scm/ > linux/kernel/git/torvalds/linux.git/commit/? > id=b87537d9e2feb30f6a962f27eb32768682698d3b from Hugh (whom I've added again > here in case he can help answer some of these). Thanks for the pointer. That race doesn't make the lock unnecessary. It is the exception to the rule because the exit of the process will correct what has happened - and in all other cases, the mmap_lock is held already (or ought to be) at a higher level. I think this comment is just wrong and should be removed as it is confusing. I see that this is a copy from elsewhere, but it's at least wrong in this context. > > > The mmap_lock is held for writing in the scenarios I have checked. > > > > > + mlock_vma_page(page); > > > + } > > > + page_vma_mapped_walk_done(&pvmw); > > > + > > > + /* found a mlocked page, no point continuing munlock check > > > */ > > > > I think you need to check_pte() to be sure it is mapped? On second thought, my comment here is wrong. > > > > > + return false; > > > + } > > > + > > > + return true; > > > > Again, I don't get the return values. If page_mlock_one() returns true, > > I'd expect for my page to now be locked. This isn't the case here, > > page_mlock_one() returns true if there are no pages present for a locked > > VMA, correct? > > This is used for an rmap walk, if we were able to mlock the page we return > false to stop the walk as there is no need to examine other VMAs if the page > has already been mlocked. My confusion is about the function name. The code is not self-documenting as it is written and there are no comments. Maybe add a comment block saying what it returns and why? > > > > +} > > > + > > > > > > /** > > > > > > - * try_to_munlock - try to munlock a page > > > + * page_mlock - try to munlock a page > > > > Is this an mlock or an munlock? I'm not confident it's either, but more > > of a check to see if there are pages mapped in a locked VMA? > > It is called (only) from the munlock code to check a page does not need to be > mlocked, or to mlock it if it does. I don't want to re-open the discussion on the name, but I just might with this comment: Do you think it is correct to have a comment for page_mlock that says "try to munlock a page"? Those seem like opposite functions to me. The @page also references the munlock'ing. This is ensuring a locked page that is also locked elsewhere will not be removed, right? > > > > * @page: the page to be munlocked > > > * > > > * Called from munlock code. Checks all of the VMAs mapping the page > > > > > > @@ -1793,11 +1818,10 @@ bool try_to_unmap(struct page *page, enum > > > ttu_flags flags)> > > > * returned with PG_mlocked cleared if no other vmas have it mlocked. > > > */ > > > > > > -void try_to_munlock(struct page *page) > > > +void page_mlock(struct page *page) > > > > > > { > > > > > > struct rmap_walk_control rwc = { > > > > > > - .rmap_one = try_to_unmap_one, > > > - .arg = (void *)TTU_MUNLOCK, > > > + .rmap_one = page_mlock_one, > > > > > > .done = page_not_mapped, > > > .anon_lock = page_lock_anon_vma_read, > > > > > > @@ -1849,7 +1873,7 @@ static struct anon_vma *rmap_walk_anon_lock(struct > > > page *page,> > > > * Find all the mappings of a page using the mapping pointer and the vma > > > chains * contained in the anon_vma struct it points to. > > > * > > > > > > - * When called from try_to_munlock(), the mmap_lock of the mm containing > > > the vma + * When called from page_mlock(), the mmap_lock of the mm > > > containing the vma> > > > * where the page was found will be held for write. So, we won't recheck > > > * vm_flags for that VMA. That should be OK, because that vma shouldn't > > > be > > > * LOCKED. > > > > > > @@ -1901,7 +1925,7 @@ static void rmap_walk_anon(struct page *page, struct > > > rmap_walk_control *rwc,> > > > * Find all the mappings of a page using the mapping pointer and the vma > > > chains * contained in the address_space struct it points to. > > > * > > > > > > - * When called from try_to_munlock(), the mmap_lock of the mm containing > > > the vma + * When called from page_mlock(), the mmap_lock of the mm > > > containing the vma> > > > * where the page was found will be held for write. So, we won't recheck > > > > Above it is stated that the lock does not need to be held, but this > > comment says it is already held for writing - which is it? > > I will have to check. > > > > * vm_flags for that VMA. That should be OK, because that vma shouldn't > > > be > > > * LOCKED. > > > > > > -- > > > 2.20.1 > > > > munlock_vma_pages_range() comments references try_to_{munlock|unmap} > > Thanks, I noticed that too when I was rereading it just now. Will fix that up. > > - Alistair > > >
On Friday, 21 May 2021 6:24:27 AM AEST Liam Howlett wrote: [...] > > > > diff --git a/mm/rmap.c b/mm/rmap.c > > > > index 977e70803ed8..f09d522725b9 100644 > > > > --- a/mm/rmap.c > > > > +++ b/mm/rmap.c > > > > @@ -1405,10 +1405,6 @@ static bool try_to_unmap_one(struct page *page, > > > > struct vm_area_struct *vma,> > > > > > > > > struct mmu_notifier_range range; > > > > enum ttu_flags flags = (enum ttu_flags)(long)arg; > > > > > > > > - /* munlock has nothing to gain from examining un-locked vmas */ > > > > - if ((flags & TTU_MUNLOCK) && !(vma->vm_flags & VM_LOCKED)) > > > > - return true; > > > > - > > > > > > > > if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) && > > > > > > > > is_zone_device_page(page) && !is_device_private_page(page)) > > > > > > > > return true; > > > > > > > > @@ -1469,8 +1465,6 @@ static bool try_to_unmap_one(struct page *page, > > > > struct vm_area_struct *vma,> > > > > > > > > page_vma_mapped_walk_done(&pvmw); > > > > break; > > > > > > > > } > > > > > > > > - if (flags & TTU_MUNLOCK) > > > > - continue; > > > > > > > > } > > > > > > > > /* Unexpected PMD-mapped THP? */ > > > > > > > > @@ -1784,8 +1778,39 @@ bool try_to_unmap(struct page *page, enum > > > > ttu_flags > > > > flags)> > > > > > > > > return !page_mapcount(page) ? true : false; > > > > > > > > } > > > > > > Please add a comment here, especially around locking. > > Did you miss this comment? I think the name confusion alone means this > needs some documentation. It's also worth mentioning arg is unused. Ack. Was meant to come back to that after discussing some of the locking questions below. The other side effect of splitting this code out is it leaves space for more specific documentation which is only a good thing. I will try and summarise some of the discussion below into a comment here. > > > > +static bool page_mlock_one(struct page *page, struct vm_area_struct > > > > *vma, > > > > + unsigned long address, void *arg) > > > > +{ > > > > + struct page_vma_mapped_walk pvmw = { > > > > + .page = page, > > > > + .vma = vma, > > > > + .address = address, > > > > + }; > > > > + > > > > + /* munlock has nothing to gain from examining un-locked vmas */ > > > > + if (!(vma->vm_flags & VM_LOCKED)) > > > > + return true; > > > > > > The logic here doesn't make sense. You called page_mlock_one() on a VMA > > > that isn't locked and it returns true? Is this a check to see if the > > > VMA has zero mlock'ed pages? > > > > I'm pretty sure the logic is correct. This is used for an rmap_walk, so we > > return true to continue the page table scan to see if other VMAs have the > > page locked. > > yes, sorry. The logic is correct but doesn't read as though it does. > I cannot see what is going on easily and there are no comments stating > what is happening. Thanks for confirming. The documentation in Documentation/vm/unevictable- lru.rst is helpful for higher level context but I will put some comments here around the logic. > > > > + > > > > + while (page_vma_mapped_walk(&pvmw)) { > > > > + /* PTE-mapped THP are never mlocked */ > > > > + if (!PageTransCompound(page)) { > > > > + /* > > > > + * Holding pte lock, we do *not* need > > > > + * mmap_lock here > > > > + */ > > > > > > Are you sure? I think you at least need to hold the mmap lock for > > > reading to ensure there's no race here? mlock_vma_page() eludes to such > > > a scenario when lazy mlocking. > > > > Not really. I don't claim to be an mlock expert but as this is a clean-up > > for try_to_unmap() the intent was to not change existing behaviour. > > > > However presenting the function in this simplified form did raise this and > > some other questions during previous reviews - see > > https://lore.kernel.org/ > > dri-devel/20210331115746.GA1463678@nvidia.com/ for the previous > > discussion. > > 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() > > > To answer the questions around locking though I did do some git sha1 > > mining. The best explanation seemed to be contained in > > https://git.kernel.org/pub/scm/ > > linux/kernel/git/torvalds/linux.git/commit/? > > id=b87537d9e2feb30f6a962f27eb32768682698d3b from Hugh (whom I've added > > again here in case he can help answer some of these). > > Thanks for the pointer. That race doesn't make the lock unnecessary. > It is the exception to the rule because the exit of the process will > correct what has happened - and in all other cases, the mmap_lock is > held already (or ought to be) at a higher level. I think this comment > is just wrong and should be removed as it is confusing. I see that this > is a copy from elsewhere, but it's at least wrong in this context. Ha, thanks. I had just copied the comment without checking to see if mmap_lock was always held by callers of page_mlock() or not. From what I can tell it is which confirms what you are saying. Thanks. > > > The mmap_lock is held for writing in the scenarios I have checked. > > > > > > > + mlock_vma_page(page); > > > > + } > > > > + page_vma_mapped_walk_done(&pvmw); > > > > + > > > > + /* found a mlocked page, no point continuing munlock > > > > check > > > > */ > > > > > > I think you need to check_pte() to be sure it is mapped? > > On second thought, my comment here is wrong. Thanks, I had meant to question that in my last reply. > > > > + return false; > > > > + } > > > > + > > > > + return true; > > > > > > Again, I don't get the return values. If page_mlock_one() returns true, > > > I'd expect for my page to now be locked. This isn't the case here, > > > page_mlock_one() returns true if there are no pages present for a locked > > > VMA, correct? > > > > This is used for an rmap walk, if we were able to mlock the page we return > > false to stop the walk as there is no need to examine other VMAs if the > > page has already been mlocked. > > My confusion is about the function name. The code is not > self-documenting as it is written and there are no comments. Maybe add > a comment block saying what it returns and why? Fair enough, I agree it isn't obvious from just reading the code so will incorporate a summary from the unevictable-lru documentation (which would have been helpful to have had referenced originally anyway). > > > > +} > > > > + > > > > > > > > /** > > > > > > > > - * try_to_munlock - try to munlock a page > > > > + * page_mlock - try to munlock a page > > > > > > Is this an mlock or an munlock? I'm not confident it's either, but more > > > of a check to see if there are pages mapped in a locked VMA? > > > > It is called (only) from the munlock code to check a page does not need to > > be mlocked, or to mlock it if it does. > > I don't want to re-open the discussion on the name, but I just might > with this comment: Do you think it is correct to have a comment for > page_mlock that says "try to munlock a page"? Those seem like opposite > functions to me. The @page also references the munlock'ing. This is > ensuring a locked page that is also locked elsewhere will not be > removed, right? No, I don't think that is correct. Sorry for the confusion. I updated the name with a script but obviously missed some of the comments. When reviewing this for rebasing I noticed a few other places in the code still incorrectly referencing this as an munlock so will go through and improve the comments more generally. I think a better description of what this function does and why might help with the inevitable confusion that arises from munlock calling a function to mlock pages. > > > > * @page: the page to be munlocked > > > > * > > > > * Called from munlock code. Checks all of the VMAs mapping the page > > > > > > > > @@ -1793,11 +1818,10 @@ bool try_to_unmap(struct page *page, enum > > > > ttu_flags flags)> > > > > > > > > * returned with PG_mlocked cleared if no other vmas have it mlocked. > > > > */ > > > > > > > > -void try_to_munlock(struct page *page) > > > > +void page_mlock(struct page *page) > > > > > > > > { > > > > > > > > struct rmap_walk_control rwc = { > > > > > > > > - .rmap_one = try_to_unmap_one, > > > > - .arg = (void *)TTU_MUNLOCK, > > > > + .rmap_one = page_mlock_one, > > > > > > > > .done = page_not_mapped, > > > > .anon_lock = page_lock_anon_vma_read, > > > > > > > > @@ -1849,7 +1873,7 @@ static struct anon_vma > > > > *rmap_walk_anon_lock(struct > > > > page *page,> > > > > > > > > * Find all the mappings of a page using the mapping pointer and the > > > > vma > > > > chains * contained in the anon_vma struct it points to. > > > > * > > > > > > > > - * When called from try_to_munlock(), the mmap_lock of the mm > > > > containing > > > > the vma + * When called from page_mlock(), the mmap_lock of the mm > > > > containing the vma> > > > > > > > > * where the page was found will be held for write. So, we won't > > > > recheck > > > > * vm_flags for that VMA. That should be OK, because that vma > > > > shouldn't > > > > be > > > > * LOCKED. > > > > > > > > @@ -1901,7 +1925,7 @@ static void rmap_walk_anon(struct page *page, > > > > struct > > > > rmap_walk_control *rwc,> > > > > > > > > * Find all the mappings of a page using the mapping pointer and the > > > > vma > > > > chains * contained in the address_space struct it points to. > > > > * > > > > > > > > - * When called from try_to_munlock(), the mmap_lock of the mm > > > > containing > > > > the vma + * When called from page_mlock(), the mmap_lock of the mm > > > > containing the vma> > > > > > > > > * where the page was found will be held for write. So, we won't > > > > recheck > > > > > > Above it is stated that the lock does not need to be held, but this > > > comment says it is already held for writing - which is it? > > > > I will have to check. > > > > > > * vm_flags for that VMA. That should be OK, because that vma > > > > shouldn't > > > > be > > > > * LOCKED. > > > > > > > > -- > > > > 2.20.1 > > > > > > munlock_vma_pages_range() comments references try_to_{munlock|unmap} > > > > Thanks, I noticed that too when I was rereading it just now. Will fix that > > up.> > > - Alistair
diff --git a/Documentation/vm/unevictable-lru.rst b/Documentation/vm/unevictable-lru.rst index 0e1490524f53..eae3af17f2d9 100644 --- a/Documentation/vm/unevictable-lru.rst +++ b/Documentation/vm/unevictable-lru.rst @@ -389,14 +389,14 @@ mlocked, munlock_vma_page() updates that zone statistics for the number of mlocked pages. Note, however, that at this point we haven't checked whether the page is mapped by other VM_LOCKED VMAs. -We can't call try_to_munlock(), the function that walks the reverse map to +We can't call page_mlock(), the function that walks the reverse map to check for other VM_LOCKED VMAs, without first isolating the page from the LRU. -try_to_munlock() is a variant of try_to_unmap() and thus requires that the page +page_mlock() is a variant of try_to_unmap() and thus requires that the page not be on an LRU list [more on these below]. However, the call to -isolate_lru_page() could fail, in which case we couldn't try_to_munlock(). So, +isolate_lru_page() could fail, in which case we can't call page_mlock(). So, we go ahead and clear PG_mlocked up front, as this might be the only chance we -have. If we can successfully isolate the page, we go ahead and -try_to_munlock(), which will restore the PG_mlocked flag and update the zone +have. If we can successfully isolate the page, we go ahead and call +page_mlock(), which will restore the PG_mlocked flag and update the zone page statistics if it finds another VMA holding the page mlocked. If we fail to isolate the page, we'll have left a potentially mlocked page on the LRU. This is fine, because we'll catch it later if and if vmscan tries to reclaim @@ -545,31 +545,24 @@ munlock or munmap system calls, mm teardown (munlock_vma_pages_all), reclaim, holepunching, and truncation of file pages and their anonymous COWed pages. -try_to_munlock() Reverse Map Scan +page_mlock() Reverse Map Scan --------------------------------- -.. warning:: - [!] TODO/FIXME: a better name might be page_mlocked() - analogous to the - page_referenced() reverse map walker. - When munlock_vma_page() [see section :ref:`munlock()/munlockall() System Call Handling <munlock_munlockall_handling>` above] tries to munlock a page, it needs to determine whether or not the page is mapped by any VM_LOCKED VMA without actually attempting to unmap all PTEs from the page. For this purpose, the unevictable/mlock infrastructure -introduced a variant of try_to_unmap() called try_to_munlock(). +introduced a variant of try_to_unmap() called page_mlock(). -try_to_munlock() calls the same functions as try_to_unmap() for anonymous and -mapped file and KSM pages with a flag argument specifying unlock versus unmap -processing. Again, these functions walk the respective reverse maps looking -for VM_LOCKED VMAs. When such a VMA is found, as in the try_to_unmap() case, -the functions mlock the page via mlock_vma_page() and return SWAP_MLOCK. This -undoes the pre-clearing of the page's PG_mlocked done by munlock_vma_page. +page_mlock() walks the respective reverse maps looking for VM_LOCKED VMAs. When +such a VMA is found the page is mlocked via mlock_vma_page(). This undoes the +pre-clearing of the page's PG_mlocked done by munlock_vma_page. -Note that try_to_munlock()'s reverse map walk must visit every VMA in a page's +Note that page_mlock()'s reverse map walk must visit every VMA in a page's reverse map to determine that a page is NOT mapped into any VM_LOCKED VMA. However, the scan can terminate when it encounters a VM_LOCKED VMA. -Although try_to_munlock() might be called a great many times when munlocking a +Although page_mlock() might be called a great many times when munlocking a large region or tearing down a large address space that has been mlocked via mlockall(), overall this is a fairly rare event. @@ -602,7 +595,7 @@ inactive lists to the appropriate node's unevictable list. shrink_inactive_list() should only see SHM_LOCK'd pages that became SHM_LOCK'd after shrink_active_list() had moved them to the inactive list, or pages mapped into VM_LOCKED VMAs that munlock_vma_page() couldn't isolate from the LRU to -recheck via try_to_munlock(). shrink_inactive_list() won't notice the latter, +recheck via page_mlock(). shrink_inactive_list() won't notice the latter, but will pass on to shrink_page_list(). shrink_page_list() again culls obviously unevictable pages that it could diff --git a/include/linux/rmap.h b/include/linux/rmap.h index def5c62c93b3..38a746787c2f 100644 --- a/include/linux/rmap.h +++ b/include/linux/rmap.h @@ -87,7 +87,6 @@ struct anon_vma_chain { enum ttu_flags { TTU_MIGRATION = 0x1, /* migration mode */ - TTU_MUNLOCK = 0x2, /* munlock mode */ TTU_SPLIT_HUGE_PMD = 0x4, /* split huge PMD if any */ TTU_IGNORE_MLOCK = 0x8, /* ignore mlock */ @@ -239,7 +238,7 @@ int page_mkclean(struct page *); * called in munlock()/munmap() path to check for other vmas holding * the page mlocked. */ -void try_to_munlock(struct page *); +void page_mlock(struct page *page); void remove_migration_ptes(struct page *old, struct page *new, bool locked); diff --git a/mm/mlock.c b/mm/mlock.c index f8f8cc32d03d..9b8b82cfbbff 100644 --- a/mm/mlock.c +++ b/mm/mlock.c @@ -108,7 +108,7 @@ void mlock_vma_page(struct page *page) /* * Finish munlock after successful page isolation * - * Page must be locked. This is a wrapper for try_to_munlock() + * Page must be locked. This is a wrapper for page_mlock() * and putback_lru_page() with munlock accounting. */ static void __munlock_isolated_page(struct page *page) @@ -118,7 +118,7 @@ static void __munlock_isolated_page(struct page *page) * and we don't need to check all the other vmas. */ if (page_mapcount(page) > 1) - try_to_munlock(page); + page_mlock(page); /* Did try_to_unlock() succeed or punt? */ if (!PageMlocked(page)) @@ -158,7 +158,7 @@ static void __munlock_isolation_failed(struct page *page) * munlock()ed or munmap()ed, we want to check whether other vmas hold the * page locked so that we can leave it on the unevictable lru list and not * bother vmscan with it. However, to walk the page's rmap list in - * try_to_munlock() we must isolate the page from the LRU. If some other + * page_mlock() we must isolate the page from the LRU. If some other * task has removed the page from the LRU, we won't be able to do that. * So we clear the PageMlocked as we might not get another chance. If we * can't isolate the page, we leave it for putback_lru_page() and vmscan @@ -168,7 +168,7 @@ unsigned int munlock_vma_page(struct page *page) { int nr_pages; - /* For try_to_munlock() and to serialize with page migration */ + /* For page_mlock() and to serialize with page migration */ BUG_ON(!PageLocked(page)); VM_BUG_ON_PAGE(PageTail(page), page); @@ -205,7 +205,7 @@ static int __mlock_posix_error_return(long retval) * * The fast path is available only for evictable pages with single mapping. * Then we can bypass the per-cpu pvec and get better performance. - * when mapcount > 1 we need try_to_munlock() which can fail. + * when mapcount > 1 we need page_mlock() which can fail. * when !page_evictable(), we need the full redo logic of putback_lru_page to * avoid leaving evictable page in unevictable list. * diff --git a/mm/rmap.c b/mm/rmap.c index 977e70803ed8..f09d522725b9 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1405,10 +1405,6 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, struct mmu_notifier_range range; enum ttu_flags flags = (enum ttu_flags)(long)arg; - /* munlock has nothing to gain from examining un-locked vmas */ - if ((flags & TTU_MUNLOCK) && !(vma->vm_flags & VM_LOCKED)) - return true; - if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) && is_zone_device_page(page) && !is_device_private_page(page)) return true; @@ -1469,8 +1465,6 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, page_vma_mapped_walk_done(&pvmw); break; } - if (flags & TTU_MUNLOCK) - continue; } /* Unexpected PMD-mapped THP? */ @@ -1784,8 +1778,39 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags) return !page_mapcount(page) ? true : false; } +static bool page_mlock_one(struct page *page, struct vm_area_struct *vma, + unsigned long address, void *arg) +{ + struct page_vma_mapped_walk pvmw = { + .page = page, + .vma = vma, + .address = address, + }; + + /* munlock has nothing to gain from examining un-locked vmas */ + if (!(vma->vm_flags & VM_LOCKED)) + return true; + + while (page_vma_mapped_walk(&pvmw)) { + /* PTE-mapped THP are never mlocked */ + if (!PageTransCompound(page)) { + /* + * Holding pte lock, we do *not* need + * mmap_lock here + */ + mlock_vma_page(page); + } + page_vma_mapped_walk_done(&pvmw); + + /* found a mlocked page, no point continuing munlock check */ + return false; + } + + return true; +} + /** - * try_to_munlock - try to munlock a page + * page_mlock - try to munlock a page * @page: the page to be munlocked * * Called from munlock code. Checks all of the VMAs mapping the page @@ -1793,11 +1818,10 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags) * returned with PG_mlocked cleared if no other vmas have it mlocked. */ -void try_to_munlock(struct page *page) +void page_mlock(struct page *page) { struct rmap_walk_control rwc = { - .rmap_one = try_to_unmap_one, - .arg = (void *)TTU_MUNLOCK, + .rmap_one = page_mlock_one, .done = page_not_mapped, .anon_lock = page_lock_anon_vma_read, @@ -1849,7 +1873,7 @@ static struct anon_vma *rmap_walk_anon_lock(struct page *page, * Find all the mappings of a page using the mapping pointer and the vma chains * contained in the anon_vma struct it points to. * - * When called from try_to_munlock(), the mmap_lock of the mm containing the vma + * When called from page_mlock(), the mmap_lock of the mm containing the vma * where the page was found will be held for write. So, we won't recheck * vm_flags for that VMA. That should be OK, because that vma shouldn't be * LOCKED. @@ -1901,7 +1925,7 @@ static void rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc, * Find all the mappings of a page using the mapping pointer and the vma chains * contained in the address_space struct it points to. * - * When called from try_to_munlock(), the mmap_lock of the mm containing the vma + * When called from page_mlock(), the mmap_lock of the mm containing the vma * where the page was found will be held for write. So, we won't recheck * vm_flags for that VMA. That should be OK, because that vma shouldn't be * LOCKED.