[PATCH v3 0/5] Cleanup and fixup for mm/migrate.c

[PATCH v3 0/5] Cleanup and fixup for mm/migrate.c

[Miaohe Lin]

Hi all,
This series contains cleanups to remove unnecessary VM_BUG_ON_PAGE and
rc != MIGRATEPAGE_SUCCESS check. Also use helper function to remove some
duplicated codes. What's more, this fixes potential deadlock in NUMA
balancing shared exec THP case and so on. More details can be found in
the respective changelogs. Thanks!

v2->v3:
Collect Reviewed-by tag
Revert commit c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA
balancing") per Yang
Remove all 3 VM_BUG_ON_PAGE and make putback_movable_page static per Yang

v1->v2:
Fix removing the wrong assertion per Rafael.
Use pr_warn_once() instead per David.
Collect Reviewed-by tag.

Miaohe Lin (5):
  mm/migrate.c: make putback_movable_page() static
  mm/migrate.c: remove unnecessary rc != MIGRATEPAGE_SUCCESS check in
    'else' case
  mm/migrate.c: fix potential indeterminate pte entry in
    migrate_vma_insert_page()
  mm/migrate.c: use helper migrate_vma_collect_skip() in
    migrate_vma_collect_hole()
  Revert "mm: migrate: skip shared exec THP for NUMA balancing"

 include/linux/migrate.h |  1 -
 mm/migrate.c            | 62 +++++++++++++++--------------------------
 2 files changed, 22 insertions(+), 41 deletions(-)

-- 
2.19.1

[PATCH v3 1/5] mm/migrate.c: make putback_movable_page() static

[Miaohe Lin]

The putback_movable_page() is just called by putback_movable_pages() and
we know the page is locked and both PageMovable() and PageIsolated() is
checked right before calling putback_movable_page(). So we make it static
and remove all the 3 VM_BUG_ON_PAGE().

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 include/linux/migrate.h | 1 -
 mm/migrate.c            | 7 +------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index fdf65f23acec..1d8095069b1c 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -44,7 +44,6 @@ extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
 		unsigned long private, enum migrate_mode mode, int reason);
 extern struct page *alloc_migration_target(struct page *page, unsigned long private);
 extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
-extern void putback_movable_page(struct page *page);
 
 extern void migrate_prep(void);
 extern void migrate_prep_local(void);
diff --git a/mm/migrate.c b/mm/migrate.c
index 47df0df8f21a..61e7f848b554 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -140,15 +140,10 @@ int isolate_movable_page(struct page *page, isolate_mode_t mode)
 	return -EBUSY;
 }
 
-/* It should be called on page which is PG_movable */
-void putback_movable_page(struct page *page)
+static void putback_movable_page(struct page *page)
 {
 	struct address_space *mapping;
 
-	VM_BUG_ON_PAGE(!PageLocked(page), page);
-	VM_BUG_ON_PAGE(!PageMovable(page), page);
-	VM_BUG_ON_PAGE(!PageIsolated(page), page);
-
 	mapping = page_mapping(page);
 	mapping->a_ops->putback_page(page);
 	__ClearPageIsolated(page);
-- 
2.19.1

Re: [PATCH v3 1/5] mm/migrate.c: make putback_movable_page() static

[David Hildenbrand]

On 25.03.21 14:15, Miaohe Lin wrote:
> The putback_movable_page() is just called by putback_movable_pages() and
> we know the page is locked and both PageMovable() and PageIsolated() is
> checked right before calling putback_movable_page(). So we make it static
> and remove all the 3 VM_BUG_ON_PAGE().
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   include/linux/migrate.h | 1 -
>   mm/migrate.c            | 7 +------
>   2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index fdf65f23acec..1d8095069b1c 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -44,7 +44,6 @@ extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
>   		unsigned long private, enum migrate_mode mode, int reason);
>   extern struct page *alloc_migration_target(struct page *page, unsigned long private);
>   extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
> -extern void putback_movable_page(struct page *page);
>   
>   extern void migrate_prep(void);
>   extern void migrate_prep_local(void);
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 47df0df8f21a..61e7f848b554 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -140,15 +140,10 @@ int isolate_movable_page(struct page *page, isolate_mode_t mode)
>   	return -EBUSY;
>   }
>   
> -/* It should be called on page which is PG_movable */
> -void putback_movable_page(struct page *page)
> +static void putback_movable_page(struct page *page)
>   {
>   	struct address_space *mapping;
>   
> -	VM_BUG_ON_PAGE(!PageLocked(page), page);
> -	VM_BUG_ON_PAGE(!PageMovable(page), page);
> -	VM_BUG_ON_PAGE(!PageIsolated(page), page);
> -
>   	mapping = page_mapping(page);
>   	mapping->a_ops->putback_page(page);
>   	__ClearPageIsolated(page);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/5] mm/migrate.c: make putback_movable_page() static

[Yang Shi]

On Thu, Mar 25, 2021 at 6:16 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> The putback_movable_page() is just called by putback_movable_pages() and
> we know the page is locked and both PageMovable() and PageIsolated() is
> checked right before calling putback_movable_page(). So we make it static
> and remove all the 3 VM_BUG_ON_PAGE().

Reviewed-by: Yang Shi <shy828301@gmail.com>

>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  include/linux/migrate.h | 1 -
>  mm/migrate.c            | 7 +------
>  2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index fdf65f23acec..1d8095069b1c 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -44,7 +44,6 @@ extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
>                 unsigned long private, enum migrate_mode mode, int reason);
>  extern struct page *alloc_migration_target(struct page *page, unsigned long private);
>  extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
> -extern void putback_movable_page(struct page *page);
>
>  extern void migrate_prep(void);
>  extern void migrate_prep_local(void);
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 47df0df8f21a..61e7f848b554 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -140,15 +140,10 @@ int isolate_movable_page(struct page *page, isolate_mode_t mode)
>         return -EBUSY;
>  }
>
> -/* It should be called on page which is PG_movable */
> -void putback_movable_page(struct page *page)
> +static void putback_movable_page(struct page *page)
>  {
>         struct address_space *mapping;
>
> -       VM_BUG_ON_PAGE(!PageLocked(page), page);
> -       VM_BUG_ON_PAGE(!PageMovable(page), page);
> -       VM_BUG_ON_PAGE(!PageIsolated(page), page);
> -
>         mapping = page_mapping(page);
>         mapping->a_ops->putback_page(page);
>         __ClearPageIsolated(page);
> --
> 2.19.1
>

[PATCH v3 2/5] mm/migrate.c: remove unnecessary rc != MIGRATEPAGE_SUCCESS check in 'else' case

[Miaohe Lin]

It's guaranteed that in the 'else' case of the rc == MIGRATEPAGE_SUCCESS
check, rc does not equal to MIGRATEPAGE_SUCCESS. Remove this unnecessary
check.

Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Yang Shi <shy828301@gmail.com>
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/migrate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 61e7f848b554..dacbdc9710ac 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1370,7 +1370,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 out:
 	if (rc == MIGRATEPAGE_SUCCESS)
 		putback_active_hugepage(hpage);
-	else if (rc != -EAGAIN && rc != MIGRATEPAGE_SUCCESS)
+	else if (rc != -EAGAIN)
 		list_move_tail(&hpage->lru, ret);
 
 	/*
-- 
2.19.1

[PATCH v3 3/5] mm/migrate.c: fix potential indeterminate pte entry in migrate_vma_insert_page()

[Miaohe Lin]

If the zone device page does not belong to un-addressable device memory,
the variable entry will be uninitialized and lead to indeterminate pte
entry ultimately. Fix this unexpected case and warn about it.

Fixes: df6ad69838fc ("mm/device-public-memory: device memory cache coherent with CPU")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/migrate.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mm/migrate.c b/mm/migrate.c
index dacbdc9710ac..87bbad578127 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2968,6 +2968,13 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
 
 			swp_entry = make_device_private_entry(page, vma->vm_flags & VM_WRITE);
 			entry = swp_entry_to_pte(swp_entry);
+		} else {
+			/*
+			 * For now we only support migrating to un-addressable
+			 * device memory.
+			 */
+			pr_warn_once("Unsupported ZONE_DEVICE page type.\n");
+			goto abort;
 		}
 	} else {
 		entry = mk_pte(page, vma->vm_page_prot);
-- 
2.19.1

Re: [PATCH v3 3/5] mm/migrate.c: fix potential indeterminate pte entry in migrate_vma_insert_page()

[David Hildenbrand]

On 25.03.21 14:15, Miaohe Lin wrote:
> If the zone device page does not belong to un-addressable device memory,
> the variable entry will be uninitialized and lead to indeterminate pte
> entry ultimately. Fix this unexpected case and warn about it.
> 
> Fixes: df6ad69838fc ("mm/device-public-memory: device memory cache coherent with CPU")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   mm/migrate.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index dacbdc9710ac..87bbad578127 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2968,6 +2968,13 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
>   
>   			swp_entry = make_device_private_entry(page, vma->vm_flags & VM_WRITE);
>   			entry = swp_entry_to_pte(swp_entry);
> +		} else {
> +			/*
> +			 * For now we only support migrating to un-addressable
> +			 * device memory.
> +			 */
> +			pr_warn_once("Unsupported ZONE_DEVICE page type.\n");
> +			goto abort;
>   		}
>   	} else {
>   		entry = mk_pte(page, vma->vm_page_prot);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v3 4/5] mm/migrate.c: use helper migrate_vma_collect_skip() in migrate_vma_collect_hole()

[Miaohe Lin]

It's more recommended to use helper function migrate_vma_collect_skip() to
skip the unexpected case and it also helps remove some duplicated codes.
Move migrate_vma_collect_skip() above migrate_vma_collect_hole() to avoid
compiler warning.

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/migrate.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 87bbad578127..c621c8f6fb7d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2311,44 +2311,38 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 #endif /* CONFIG_NUMA */
 
 #ifdef CONFIG_DEVICE_PRIVATE
-static int migrate_vma_collect_hole(unsigned long start,
+static int migrate_vma_collect_skip(unsigned long start,
 				    unsigned long end,
-				    __always_unused int depth,
 				    struct mm_walk *walk)
 {
 	struct migrate_vma *migrate = walk->private;
 	unsigned long addr;
 
-	/* Only allow populating anonymous memory. */
-	if (!vma_is_anonymous(walk->vma)) {
-		for (addr = start; addr < end; addr += PAGE_SIZE) {
-			migrate->src[migrate->npages] = 0;
-			migrate->dst[migrate->npages] = 0;
-			migrate->npages++;
-		}
-		return 0;
-	}
-
 	for (addr = start; addr < end; addr += PAGE_SIZE) {
-		migrate->src[migrate->npages] = MIGRATE_PFN_MIGRATE;
 		migrate->dst[migrate->npages] = 0;
-		migrate->npages++;
-		migrate->cpages++;
+		migrate->src[migrate->npages++] = 0;
 	}
 
 	return 0;
 }
 
-static int migrate_vma_collect_skip(unsigned long start,
+static int migrate_vma_collect_hole(unsigned long start,
 				    unsigned long end,
+				    __always_unused int depth,
 				    struct mm_walk *walk)
 {
 	struct migrate_vma *migrate = walk->private;
 	unsigned long addr;
 
+	/* Only allow populating anonymous memory. */
+	if (!vma_is_anonymous(walk->vma))
+		return migrate_vma_collect_skip(start, end, walk);
+
 	for (addr = start; addr < end; addr += PAGE_SIZE) {
+		migrate->src[migrate->npages] = MIGRATE_PFN_MIGRATE;
 		migrate->dst[migrate->npages] = 0;
-		migrate->src[migrate->npages++] = 0;
+		migrate->npages++;
+		migrate->cpages++;
 	}
 
 	return 0;
-- 
2.19.1

[PATCH v3 5/5] Revert "mm: migrate: skip shared exec THP for NUMA balancing"

[Miaohe Lin]

This reverts commit c77c5cbafe549eb330e8909861a3e16cbda2c848.

Since commit c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA
balancing"), the NUMA balancing would skip shared exec transhuge page.
But this enhancement is not suitable for transhuge page. Because it's
required that page_mapcount() must be 1 due to no migration pte dance
is done here. On the other hand, the shared exec transhuge page will
leave the migrate_misplaced_page() with pte entry untouched and page
locked. Thus pagefault for NUMA will be triggered again and deadlock
occurs when we start waiting for the page lock held by ourselves.

Yang Shi said:

 "Thanks for catching this. By relooking the code I think the other
  important reason for removing this is
  migrate_misplaced_transhuge_page() actually can't see shared exec
  file THP at all since page_lock_anon_vma_read() is called before
  and if page is not anonymous page it will just restore the PMD
  without migrating anything.
  The pages for private mapped file vma may be anonymous pages due to
  COW but they can't be THP so it won't trigger THP numa fault at all. I
  think this is why no bug was reported. I overlooked this in the first
  place."

Reviewed-by: Yang Shi <shy828301@gmail.com>
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/migrate.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index c621c8f6fb7d..51190759e6dd 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2105,17 +2105,6 @@ bool pmd_trans_migrating(pmd_t pmd)
 	return PageLocked(page);
 }
 
-static inline bool is_shared_exec_page(struct vm_area_struct *vma,
-				       struct page *page)
-{
-	if (page_mapcount(page) != 1 &&
-	    (page_is_file_lru(page) || vma_is_shmem(vma)) &&
-	    (vma->vm_flags & VM_EXEC))
-		return true;
-
-	return false;
-}
-
 /*
  * Attempt to migrate a misplaced page to the specified destination
  * node. Caller is expected to have an elevated reference count on
@@ -2133,7 +2122,8 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
 	 * Don't migrate file pages that are mapped in multiple processes
 	 * with execute permissions as they are probably shared libraries.
 	 */
-	if (is_shared_exec_page(vma, page))
+	if (page_mapcount(page) != 1 && page_is_file_lru(page) &&
+	    (vma->vm_flags & VM_EXEC))
 		goto out;
 
 	/*
@@ -2188,9 +2178,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	int page_lru = page_is_file_lru(page);
 	unsigned long start = address & HPAGE_PMD_MASK;
 
-	if (is_shared_exec_page(vma, page))
-		goto out;
-
 	new_page = alloc_pages_node(node,
 		(GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
 		HPAGE_PMD_ORDER);
@@ -2302,7 +2289,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 
 out_unlock:
 	unlock_page(page);
-out:
 	put_page(page);
 	return 0;
 }
-- 
2.19.1