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

[PATCH v2 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!

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: remove unnecessary VM_BUG_ON_PAGE on
    putback_movable_page()
  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()
  mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP
    case

 mm/migrate.c | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

-- 
2.19.1

[PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()

[Miaohe Lin]

The !PageLocked() check is implicitly done in PageMovable(). Remove this
explicit one.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/migrate.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 47df0df8f21a..facec65c7374 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -145,7 +145,6 @@ 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);
 
-- 
2.19.1

[PATCH v2 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>
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 facec65c7374..97da1fabdf72 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1374,7 +1374,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 v2 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 97da1fabdf72..d372be3da9b2 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2972,6 +2972,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

[PATCH v2 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 d372be3da9b2..5357a8527ca2 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2315,44 +2315,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 v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case

[Miaohe Lin]

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.

Fixes: c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA balancing")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/migrate.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 5357a8527ca2..68bfa1625898 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2192,9 +2192,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);
@@ -2306,7 +2303,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 
 out_unlock:
 	unlock_page(page);
-out:
 	put_page(page);
 	return 0;
 }
-- 
2.19.1

Re: [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()

[David Hildenbrand]

On 23.03.21 14:54, Miaohe Lin wrote:
> The !PageLocked() check is implicitly done in PageMovable(). Remove this
> explicit one.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   mm/migrate.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 47df0df8f21a..facec65c7374 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -145,7 +145,6 @@ 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);
>   
> 

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

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case

[Yang Shi]

On Tue, Mar 23, 2021 at 6:55 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> 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.

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.

Your fix is correct, and please add the above justification to your commit log.

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

>
> Fixes: c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA balancing")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/migrate.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 5357a8527ca2..68bfa1625898 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2192,9 +2192,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);
> @@ -2306,7 +2303,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>
>  out_unlock:
>         unlock_page(page);
> -out:
>         put_page(page);
>         return 0;
>  }
> --
> 2.19.1
>

Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case

[Yang Shi]

On Tue, Mar 23, 2021 at 6:55 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> 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.

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.

Your fix is correct, and please add the above justification to your commit log.

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

>
> Fixes: c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA balancing")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/migrate.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 5357a8527ca2..68bfa1625898 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2192,9 +2192,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);
> @@ -2306,7 +2303,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>
>  out_unlock:
>         unlock_page(page);
> -out:
>         put_page(page);
>         return 0;
>  }
> --
> 2.19.1
>

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

[Yang Shi]

On Tue, Mar 23, 2021 at 6:54 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> 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: Yang Shi <shy828301@gmail.com>

>
> Reviewed-by: David Hildenbrand <david@redhat.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 facec65c7374..97da1fabdf72 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1374,7 +1374,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
>

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

[Yang Shi]

On Tue, Mar 23, 2021 at 6:54 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> 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: Yang Shi <shy828301@gmail.com>

>
> Reviewed-by: David Hildenbrand <david@redhat.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 facec65c7374..97da1fabdf72 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1374,7 +1374,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
>

Re: [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()

[Yang Shi]

On Tue, Mar 23, 2021 at 6:54 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> The !PageLocked() check is implicitly done in PageMovable(). Remove this
> explicit one.

TBH, I'm a little bit reluctant to have this kind change. If "locked"
check is necessary we'd better make it explicit otherwise just remove
it.

And why not just remove all the 3 VM_BUG_ON_PAGE since
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().

And you also could make putback_movable_page() static.

> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/migrate.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 47df0df8f21a..facec65c7374 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -145,7 +145,6 @@ 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);
>
> --
> 2.19.1
>

Re: [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()

[Yang Shi]

On Tue, Mar 23, 2021 at 6:54 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> The !PageLocked() check is implicitly done in PageMovable(). Remove this
> explicit one.

TBH, I'm a little bit reluctant to have this kind change. If "locked"
check is necessary we'd better make it explicit otherwise just remove
it.

And why not just remove all the 3 VM_BUG_ON_PAGE since
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().

And you also could make putback_movable_page() static.

> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/migrate.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 47df0df8f21a..facec65c7374 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -145,7 +145,6 @@ 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);
>
> --
> 2.19.1
>

Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case

[Yang Shi]

On Tue, Mar 23, 2021 at 10:17 AM Yang Shi <shy828301@gmail.com> wrote:
>
> On Tue, Mar 23, 2021 at 6:55 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
> >
> > 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.
>
> 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.
>
> Your fix is correct, and please add the above justification to your commit log.

BTW, I think you can just undo or revert commit c77c5cbafe54 ("mm:
migrate: skip shared exec THP for NUMA balancing").

Thanks,
Yang

>
> Reviewed-by: Yang Shi <shy828301@gmail.com>
>
> >
> > Fixes: c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA balancing")
> > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> > ---
> >  mm/migrate.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 5357a8527ca2..68bfa1625898 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -2192,9 +2192,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);
> > @@ -2306,7 +2303,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
> >
> >  out_unlock:
> >         unlock_page(page);
> > -out:
> >         put_page(page);
> >         return 0;
> >  }
> > --
> > 2.19.1
> >

Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case

[Yang Shi]

On Tue, Mar 23, 2021 at 10:17 AM Yang Shi <shy828301@gmail.com> wrote:
>
> On Tue, Mar 23, 2021 at 6:55 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
> >
> > 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.
>
> 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.
>
> Your fix is correct, and please add the above justification to your commit log.

BTW, I think you can just undo or revert commit c77c5cbafe54 ("mm:
migrate: skip shared exec THP for NUMA balancing").

Thanks,
Yang

>
> Reviewed-by: Yang Shi <shy828301@gmail.com>
>
> >
> > Fixes: c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA balancing")
> > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> > ---
> >  mm/migrate.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 5357a8527ca2..68bfa1625898 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -2192,9 +2192,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);
> > @@ -2306,7 +2303,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
> >
> >  out_unlock:
> >         unlock_page(page);
> > -out:
> >         put_page(page);
> >         return 0;
> >  }
> > --
> > 2.19.1
> >

Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case

[Miaohe Lin]

On 2021/3/24 1:17, Yang Shi wrote:
> On Tue, Mar 23, 2021 at 6:55 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> 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.
> 
> 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.
> 
> Your fix is correct, and please add the above justification to your commit log.
> 

Will do. Many thanks for your review!

> Reviewed-by: Yang Shi <shy828301@gmail.com>
> 
>>
>> Fixes: c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA balancing")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/migrate.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 5357a8527ca2..68bfa1625898 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -2192,9 +2192,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);
>> @@ -2306,7 +2303,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>>
>>  out_unlock:
>>         unlock_page(page);
>> -out:
>>         put_page(page);
>>         return 0;
>>  }
>> --
>> 2.19.1
>>
> .
> 

Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case

[Miaohe Lin]

On 2021/3/24 9:16, Yang Shi wrote:
> On Tue, Mar 23, 2021 at 10:17 AM Yang Shi <shy828301@gmail.com> wrote:
>>
>> On Tue, Mar 23, 2021 at 6:55 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>>>
>>> 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.
>>
>> 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.
>>
>> Your fix is correct, and please add the above justification to your commit log.
> 
> BTW, I think you can just undo or revert commit c77c5cbafe54 ("mm:
> migrate: skip shared exec THP for NUMA balancing").
> 

Yep, we can revert this commit. I thought it handle the shared exec base page too.
Will do it and with the above justification to the commit log.
Many Thanks!

> Thanks,
> Yang
> 
>>
>> Reviewed-by: Yang Shi <shy828301@gmail.com>
>>
>>>
>>> Fixes: c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA balancing")
>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>> ---
>>>  mm/migrate.c | 4 ----
>>>  1 file changed, 4 deletions(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index 5357a8527ca2..68bfa1625898 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -2192,9 +2192,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);
>>> @@ -2306,7 +2303,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>>>
>>>  out_unlock:
>>>         unlock_page(page);
>>> -out:
>>>         put_page(page);
>>>         return 0;
>>>  }
>>> --
>>> 2.19.1
>>>
> .
> 

Re: [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()

[Miaohe Lin]

On 2021/3/24 1:58, Yang Shi wrote:
> On Tue, Mar 23, 2021 at 6:54 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> The !PageLocked() check is implicitly done in PageMovable(). Remove this
>> explicit one.
> 
> TBH, I'm a little bit reluctant to have this kind change. If "locked"
> check is necessary we'd better make it explicit otherwise just remove
> it.
> 
> And why not just remove all the 3 VM_BUG_ON_PAGE since
> 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().
> 
> And you also could make putback_movable_page() static.
> 

Sounds good! Many thanks for your advice!

>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/migrate.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 47df0df8f21a..facec65c7374 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -145,7 +145,6 @@ 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);
>>
>> --
>> 2.19.1
>>
> .
> 

Re: [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()

[Miaohe Lin]

On 2021/3/23 22:27, David Hildenbrand wrote:
> On 23.03.21 14:54, Miaohe Lin wrote:
>> The !PageLocked() check is implicitly done in PageMovable(). Remove this
>> explicit one.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>   mm/migrate.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 47df0df8f21a..facec65c7374 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -145,7 +145,6 @@ 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);
>>  
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>

Many thanks for your review. But I'am going to change this patch, so this Reviewed-by tag may not
applies any more. Sorry about it!

>