Re: [PATCH v3 2/5] mm: fix missing cache flush for all tail pages of compound page

[Muchun Song <songmuchun@xxxxxxxxxxxxx>]

On Wed, Feb 2, 2022 at 7:33 AM Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
>
> On 1/31/22 08:02, Muchun Song wrote:
> > The D-cache maintenance inside move_to_new_page() only consider one page,
> > there is still D-cache maintenance issue for tail pages of compound page
> > (e.g. THP or HugeTLB).  THP migration is only enabled on x86_64, ARM64
> > and powerpc, while powerpc and arm64 need to maintain the consistency
> > between I-Cache and D-Cache, which depends on flush_dcache_page() to
> > maintain the consistency between I-Cache and D-Cache.  In theory, the
> > issue can be found on arm64 and powerpc.  HugeTLB migration is enabled
> > on arm, arm64, mips, parisc, powerpc, riscv, s390 and sh, while arm
> > has handled the compound page cache flush in flush_dcache_page(), but
> > most others do not.  In theory, the issue exists on many architectures.
> > Fix this by not using flush_dcache_folio() since it is not backportable.
> >
> > Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path")
> > Fixes: 290408d4a250 ("hugetlb: hugepage migration core")
> > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> > Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>
> > ---
> >  mm/migrate.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index c9296d63878d..c418e8d92b9c 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -933,9 +933,12 @@ static int move_to_new_page(struct page *newpage, struct page *page,
> >               if (!PageMappingFlags(page))
> >                       page->mapping = NULL;
> >
> > -             if (likely(!is_zone_device_page(newpage)))
> > -                     flush_dcache_page(newpage);
> > +             if (likely(!is_zone_device_page(newpage))) {
> > +                     int i, nr = compound_nr(newpage);
> >
> > +                     for (i = 0; i < nr; i++)
> > +                             flush_dcache_page(newpage + i);
> > +             }
>
> As you have already noted elsewhere, the arm64 version of flush_dcache_page
> seems to handle this and flush the entire compound_page.  Is this going to
> flush the entire compound page compound_nr times?
>

Not flush cache compound_nr times but set PG_dcache_clean
on page->flags compound_nr times.  The definition of
flush_dcache_page() on arm64 is as follows.

void flush_dcache_page(struct page *page)
{
        if (test_bit(PG_dcache_clean, &page->flags))
                clear_bit(PG_dcache_clean, &page->flags);
}

So I have a plan to improve this, e.g. implement flush_dcache_folio()
on arm64 and replace those multiple dcache flush with
flush_dcache_folio().

void flush_dcache_folio(struct folio *folio)
{
       if (test_bit(PG_dcache_clean, &folio->flags))
               clear_bit(PG_dcache_clean, &folio->flags);
}

Thanks.