Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210323135405.65059-1-linmiaohe@huawei.com> <20210323135405.65059-6-linmiaohe@huawei.com>
In-Reply-To: <20210323135405.65059-6-linmiaohe@huawei.com>
From:   Yang Shi <shy828301@gmail.com>
Date:   Tue, 23 Mar 2021 10:17:48 -0700
Message-ID: <CAHbLzkoSsPWSdyZQBR03NbU8i3AG_DTL4P-efYULvuYmWzyYbg@mail.gmail.com>
Subject: Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA
 balancing shared exec THP case
To:     Miaohe Lin <linmiaohe@huawei.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Jerome Glisse <jglisse@redhat.com>,
        Rafael Aquini <aquini@redhat.com>,
        David Hildenbrand <david@redhat.com>,
        Alistair Popple <apopple@nvidia.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux MM <linux-mm@kvack.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>