Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220513191705.457775-1-shy828301@gmail.com> <YoSEsa2zvqylYuZC@FVFYT0MHHV2J.usts.net>
In-Reply-To: <YoSEsa2zvqylYuZC@FVFYT0MHHV2J.usts.net>
From:   Yang Shi <shy828301@gmail.com>
Date:   Wed, 18 May 2022 11:45:14 -0700
Message-ID: <CAHbLzkrXvJn6FbCNUTMi3c85nnMvOda58V2DDSb9W7_9_YTjSg@mail.gmail.com>
Subject: Re: [v2 PATCH] mm: pvmw: check possible huge PMD map by transhuge_vma_suitable()
To:     Muchun Song <songmuchun@bytedance.com>
Cc:     Matthew Wilcox <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 17, 2022 at 10:31 PM Muchun Song <songmuchun@bytedance.com> wrote:
>
> On Fri, May 13, 2022 at 12:17:05PM -0700, Yang Shi wrote:
> > IIUC PVMW checks if the vma is possibly huge PMD mapped by
> > transparent_hugepage_active() and "pvmw->nr_pages >= HPAGE_PMD_NR".
> >
> > Actually pvmw->nr_pages is returned by compound_nr() or
> > folio_nr_pages(), so the page should be THP as long as "pvmw->nr_pages
> > >= HPAGE_PMD_NR".  And it is guaranteed THP is allocated for valid VMA
> > in the first place.  But it may be not PMD mapped if the VMA is file
> > VMA and it is not properly aligned.  The transhuge_vma_suitable()
> > is used to do such check, so replace transparent_hugepage_active() to
> > it, which is too heavy and overkilling.
> >
> > Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> > Cc: Muchun Song <songmuchun@bytedance.com>
> > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > ---
> > v2: * Fixed build error for !CONFIG_TRANSPARENT_HUGEPAGE
> >     * Removed fixes tag per Willy
> >
> >  include/linux/huge_mm.h | 8 ++++++--
> >  mm/page_vma_mapped.c    | 2 +-
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index fbf36bb1be22..c2826b1f4069 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -117,8 +117,10 @@ extern struct kobj_attribute shmem_enabled_attr;
> >  extern unsigned long transparent_hugepage_flags;
> >
> >  static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
> > -             unsigned long haddr)
> > +             unsigned long addr)
> >  {
> > +     unsigned long haddr;
> > +
> >       /* Don't have to check pgoff for anonymous vma */
> >       if (!vma_is_anonymous(vma)) {
> >               if (!IS_ALIGNED((vma->vm_start >> PAGE_SHIFT) - vma->vm_pgoff,
> > @@ -126,6 +128,8 @@ static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
> >                       return false;
> >       }
> >
> > +     haddr = addr & HPAGE_PMD_MASK;
> > +
> >       if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
> >               return false;
> >       return true;
> > @@ -328,7 +332,7 @@ static inline bool transparent_hugepage_active(struct vm_area_struct *vma)
> >  }
> >
> >  static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
> > -             unsigned long haddr)
> > +             unsigned long addr)
> >  {
> >       return false;
> >  }
> > diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> > index c10f839fc410..e971a467fcdf 100644
> > --- a/mm/page_vma_mapped.c
> > +++ b/mm/page_vma_mapped.c
> > @@ -243,7 +243,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
> >                        * cleared *pmd but not decremented compound_mapcount().
> >                        */
> >                       if ((pvmw->flags & PVMW_SYNC) &&
> > -                         transparent_hugepage_active(vma) &&
> > +                         transhuge_vma_suitable(vma, pvmw->address) &&
>
> How about the following diff? Then we do not need to change
> transhuge_vma_suitable().  All the users of transhuge_vma_suitable()
> are already do the alignment by themselves.

Thanks for the suggestion. But TBH I don't think this is a better way.
I did think about this before proposing v2, but I don't prefer to
pollute the code with IS_ENABLED(CONFIG_xxx) since the definition of
transhuge_vma_suitable() is already protected by #ifdef. Rounding the
address in transhuge_vma_suitable() seems neater and more readable to
me IMHO.

Some callers of transhuge_vma_suitable() do round the address before
calling it, but the rounded address is used by other codes in the
callers too.

>
> Thanks.
>
> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> index c10f839fc410..0aed5ca60c67 100644
> --- a/mm/page_vma_mapped.c
> +++ b/mm/page_vma_mapped.c
> @@ -243,7 +243,8 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
>                          * cleared *pmd but not decremented compound_mapcount().
>                          */
>                         if ((pvmw->flags & PVMW_SYNC) &&
> -                           transparent_hugepage_active(vma) &&
> +                           IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
> +                           transhuge_vma_suitable(vma, pvmw->address & HPAGE_PMD_MASK) &&
>                             (pvmw->nr_pages >= HPAGE_PMD_NR)) {
>                                 spinlock_t *ptl = pmd_lock(mm, pvmw->pmd);
>
> >                           (pvmw->nr_pages >= HPAGE_PMD_NR)) {
> >                               spinlock_t *ptl = pmd_lock(mm, pvmw->pmd);
> >
> > --
> > 2.26.3
> >
> >