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: <20210427133214.2270207-1-linmiaohe@huawei.com> <20210427133214.2270207-4-linmiaohe@huawei.com>
In-Reply-To: <20210427133214.2270207-4-linmiaohe@huawei.com>
From:   Yang Shi <shy828301@gmail.com>
Date:   Tue, 27 Apr 2021 14:03:05 -0700
Message-ID: <CAHbLzkrBAtTM8aE_pM4ASQ6cGyfPcs7_7HPJLCd9T24VyqU5wQ@mail.gmail.com>
Subject: Re: [PATCH 3/5] mm/huge_memory.c: add missing read-only THP checking
 in transparent_hugepage_enabled()
To:     Miaohe Lin <linmiaohe@huawei.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, Zi Yan <ziy@nvidia.com>,
        william.kucharski@oracle.com, Matthew Wilcox <willy@infradead.org>,
        Yang Shi <yang.shi@linux.alibaba.com>,
        aneesh.kumar@linux.ibm.com, Ralph Campbell <rcampbell@nvidia.com>,
        Song Liu <songliubraving@fb.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Rik van Riel <riel@surriel.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Minchan Kim <minchan@kernel.org>,
        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, Apr 27, 2021 at 6:32 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> Since commit 99cb0dbd47a1 ("mm,thp: add read-only THP support for
> (non-shmem) FS"), read-only THP file mapping is supported. But it
> forgot to add checking for it in transparent_hugepage_enabled().
>
> Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/huge_memory.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 76ca1eb2a223..aa22a0ae9894 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -74,6 +74,9 @@ bool transparent_hugepage_enabled(struct vm_area_struct *vma)
>                 return __transparent_hugepage_enabled(vma);
>         if (vma_is_shmem(vma))
>                 return shmem_huge_enabled(vma);
> +       if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && vma->vm_file &&
> +           (vma->vm_flags & VM_DENYWRITE))
> +               return true;

I don't think this change is correct. This function is used to
indicate if allocating THP is eligible for the VMAs or not showed by
smap. And currently readonly FS THP is collapsed by khugepaged only.

So, you need check if the vma is suitable for khugepaged. Take a look
at what hugepage_vma_check() does.

And, the new patch
(https://lore.kernel.org/linux-mm/20210406000930.3455850-1-cfijalkovich@google.com/)
relax the constraints for readonly FS THP, it might be already in -mm
tree, so you need adopt the new condition as well.

>
>         return false;
>  }

> --
> 2.23.0
>
>