linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Cleanup and fixup for mm/migrate.c
@ 2021-03-23 13:54 Miaohe Lin
  2021-03-23 13:54 ` [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page() Miaohe Lin
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Miaohe Lin @ 2021-03-23 13:54 UTC (permalink / raw)
  To: akpm
  Cc: jglisse, shy828301, aquini, david, apopple, linux-kernel,
	linux-mm, linmiaohe

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()
  2021-03-23 13:54 [PATCH v2 0/5] Cleanup and fixup for mm/migrate.c Miaohe Lin
@ 2021-03-23 13:54 ` Miaohe Lin
  2021-03-23 14:27   ` David Hildenbrand
  2021-03-23 17:58   ` Yang Shi
  2021-03-23 13:54 ` [PATCH v2 2/5] mm/migrate.c: remove unnecessary rc != MIGRATEPAGE_SUCCESS check in 'else' case Miaohe Lin
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 15+ messages in thread
From: Miaohe Lin @ 2021-03-23 13:54 UTC (permalink / raw)
  To: akpm
  Cc: jglisse, shy828301, aquini, david, apopple, linux-kernel,
	linux-mm, linmiaohe

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/5] mm/migrate.c: remove unnecessary rc != MIGRATEPAGE_SUCCESS check in 'else' case
  2021-03-23 13:54 [PATCH v2 0/5] Cleanup and fixup for mm/migrate.c Miaohe Lin
  2021-03-23 13:54 ` [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page() Miaohe Lin
@ 2021-03-23 13:54 ` Miaohe Lin
  2021-03-23 17:46   ` Yang Shi
  2021-03-23 13:54 ` [PATCH v2 3/5] mm/migrate.c: fix potential indeterminate pte entry in migrate_vma_insert_page() Miaohe Lin
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Miaohe Lin @ 2021-03-23 13:54 UTC (permalink / raw)
  To: akpm
  Cc: jglisse, shy828301, aquini, david, apopple, linux-kernel,
	linux-mm, linmiaohe

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 3/5] mm/migrate.c: fix potential indeterminate pte entry in migrate_vma_insert_page()
  2021-03-23 13:54 [PATCH v2 0/5] Cleanup and fixup for mm/migrate.c Miaohe Lin
  2021-03-23 13:54 ` [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page() Miaohe Lin
  2021-03-23 13:54 ` [PATCH v2 2/5] mm/migrate.c: remove unnecessary rc != MIGRATEPAGE_SUCCESS check in 'else' case Miaohe Lin
@ 2021-03-23 13:54 ` Miaohe Lin
  2021-03-23 13:54 ` [PATCH v2 4/5] mm/migrate.c: use helper migrate_vma_collect_skip() in migrate_vma_collect_hole() Miaohe Lin
  2021-03-23 13:54 ` [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case Miaohe Lin
  4 siblings, 0 replies; 15+ messages in thread
From: Miaohe Lin @ 2021-03-23 13:54 UTC (permalink / raw)
  To: akpm
  Cc: jglisse, shy828301, aquini, david, apopple, linux-kernel,
	linux-mm, linmiaohe

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 4/5] mm/migrate.c: use helper migrate_vma_collect_skip() in migrate_vma_collect_hole()
  2021-03-23 13:54 [PATCH v2 0/5] Cleanup and fixup for mm/migrate.c Miaohe Lin
                   ` (2 preceding siblings ...)
  2021-03-23 13:54 ` [PATCH v2 3/5] mm/migrate.c: fix potential indeterminate pte entry in migrate_vma_insert_page() Miaohe Lin
@ 2021-03-23 13:54 ` Miaohe Lin
  2021-03-23 13:54 ` [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case Miaohe Lin
  4 siblings, 0 replies; 15+ messages in thread
From: Miaohe Lin @ 2021-03-23 13:54 UTC (permalink / raw)
  To: akpm
  Cc: jglisse, shy828301, aquini, david, apopple, linux-kernel,
	linux-mm, linmiaohe

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case
  2021-03-23 13:54 [PATCH v2 0/5] Cleanup and fixup for mm/migrate.c Miaohe Lin
                   ` (3 preceding siblings ...)
  2021-03-23 13:54 ` [PATCH v2 4/5] mm/migrate.c: use helper migrate_vma_collect_skip() in migrate_vma_collect_hole() Miaohe Lin
@ 2021-03-23 13:54 ` Miaohe Lin
  2021-03-23 17:17   ` Yang Shi
  4 siblings, 1 reply; 15+ messages in thread
From: Miaohe Lin @ 2021-03-23 13:54 UTC (permalink / raw)
  To: akpm
  Cc: jglisse, shy828301, aquini, david, apopple, linux-kernel,
	linux-mm, linmiaohe

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()
  2021-03-23 13:54 ` [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page() Miaohe Lin
@ 2021-03-23 14:27   ` David Hildenbrand
  2021-03-24  2:40     ` Miaohe Lin
  2021-03-23 17:58   ` Yang Shi
  1 sibling, 1 reply; 15+ messages in thread
From: David Hildenbrand @ 2021-03-23 14:27 UTC (permalink / raw)
  To: Miaohe Lin, akpm
  Cc: jglisse, shy828301, aquini, apopple, linux-kernel, linux-mm

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case
  2021-03-23 13:54 ` [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case Miaohe Lin
@ 2021-03-23 17:17   ` Yang Shi
  2021-03-24  1:16     ` Yang Shi
  2021-03-24  2:03     ` Miaohe Lin
  0 siblings, 2 replies; 15+ messages in thread
From: Yang Shi @ 2021-03-23 17:17 UTC (permalink / raw)
  To: Miaohe Lin
  Cc: Andrew Morton, Jerome Glisse, Rafael Aquini, David Hildenbrand,
	Alistair Popple, Linux Kernel Mailing List, Linux MM

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
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/5] mm/migrate.c: remove unnecessary rc != MIGRATEPAGE_SUCCESS check in 'else' case
  2021-03-23 13:54 ` [PATCH v2 2/5] mm/migrate.c: remove unnecessary rc != MIGRATEPAGE_SUCCESS check in 'else' case Miaohe Lin
@ 2021-03-23 17:46   ` Yang Shi
  0 siblings, 0 replies; 15+ messages in thread
From: Yang Shi @ 2021-03-23 17:46 UTC (permalink / raw)
  To: Miaohe Lin
  Cc: Andrew Morton, Jerome Glisse, Rafael Aquini, David Hildenbrand,
	Alistair Popple, Linux Kernel Mailing List, Linux MM

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
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()
  2021-03-23 13:54 ` [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page() Miaohe Lin
  2021-03-23 14:27   ` David Hildenbrand
@ 2021-03-23 17:58   ` Yang Shi
  2021-03-24  2:37     ` Miaohe Lin
  1 sibling, 1 reply; 15+ messages in thread
From: Yang Shi @ 2021-03-23 17:58 UTC (permalink / raw)
  To: Miaohe Lin
  Cc: Andrew Morton, Jerome Glisse, Rafael Aquini, David Hildenbrand,
	Alistair Popple, Linux Kernel Mailing List, Linux MM

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
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case
  2021-03-23 17:17   ` Yang Shi
@ 2021-03-24  1:16     ` Yang Shi
  2021-03-24  2:14       ` Miaohe Lin
  2021-03-24  2:03     ` Miaohe Lin
  1 sibling, 1 reply; 15+ messages in thread
From: Yang Shi @ 2021-03-24  1:16 UTC (permalink / raw)
  To: Miaohe Lin
  Cc: Andrew Morton, Jerome Glisse, Rafael Aquini, David Hildenbrand,
	Alistair Popple, Linux Kernel Mailing List, Linux MM

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
> >


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case
  2021-03-23 17:17   ` Yang Shi
  2021-03-24  1:16     ` Yang Shi
@ 2021-03-24  2:03     ` Miaohe Lin
  1 sibling, 0 replies; 15+ messages in thread
From: Miaohe Lin @ 2021-03-24  2:03 UTC (permalink / raw)
  To: Yang Shi
  Cc: Andrew Morton, Jerome Glisse, Rafael Aquini, David Hildenbrand,
	Alistair Popple, Linux Kernel Mailing List, Linux MM

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
>>
> .
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case
  2021-03-24  1:16     ` Yang Shi
@ 2021-03-24  2:14       ` Miaohe Lin
  0 siblings, 0 replies; 15+ messages in thread
From: Miaohe Lin @ 2021-03-24  2:14 UTC (permalink / raw)
  To: Yang Shi
  Cc: Andrew Morton, Jerome Glisse, Rafael Aquini, David Hildenbrand,
	Alistair Popple, Linux Kernel Mailing List, Linux MM

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
>>>
> .
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()
  2021-03-23 17:58   ` Yang Shi
@ 2021-03-24  2:37     ` Miaohe Lin
  0 siblings, 0 replies; 15+ messages in thread
From: Miaohe Lin @ 2021-03-24  2:37 UTC (permalink / raw)
  To: Yang Shi
  Cc: Andrew Morton, Jerome Glisse, Rafael Aquini, David Hildenbrand,
	Alistair Popple, Linux Kernel Mailing List, Linux MM

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
>>
> .
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page()
  2021-03-23 14:27   ` David Hildenbrand
@ 2021-03-24  2:40     ` Miaohe Lin
  0 siblings, 0 replies; 15+ messages in thread
From: Miaohe Lin @ 2021-03-24  2:40 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: jglisse, shy828301, aquini, apopple, linux-kernel, linux-mm,
	Andrew Morton

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!

> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-03-24  2:40 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 13:54 [PATCH v2 0/5] Cleanup and fixup for mm/migrate.c Miaohe Lin
2021-03-23 13:54 ` [PATCH v2 1/5] mm/migrate.c: remove unnecessary VM_BUG_ON_PAGE on putback_movable_page() Miaohe Lin
2021-03-23 14:27   ` David Hildenbrand
2021-03-24  2:40     ` Miaohe Lin
2021-03-23 17:58   ` Yang Shi
2021-03-24  2:37     ` Miaohe Lin
2021-03-23 13:54 ` [PATCH v2 2/5] mm/migrate.c: remove unnecessary rc != MIGRATEPAGE_SUCCESS check in 'else' case Miaohe Lin
2021-03-23 17:46   ` Yang Shi
2021-03-23 13:54 ` [PATCH v2 3/5] mm/migrate.c: fix potential indeterminate pte entry in migrate_vma_insert_page() Miaohe Lin
2021-03-23 13:54 ` [PATCH v2 4/5] mm/migrate.c: use helper migrate_vma_collect_skip() in migrate_vma_collect_hole() Miaohe Lin
2021-03-23 13:54 ` [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case Miaohe Lin
2021-03-23 17:17   ` Yang Shi
2021-03-24  1:16     ` Yang Shi
2021-03-24  2:14       ` Miaohe Lin
2021-03-24  2:03     ` Miaohe Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).