[PATCH 1/2] mm: thp: fix wrong cache flush in remove_migration_pmd()

[PATCH 1/2] mm: thp: fix wrong cache flush in remove_migration_pmd()

[Muchun Song]

The flush_cache_page() is supposed to be justified only if the page
is already placed in process page table, and that is done right after
flush_cache_page(). So using this interface is wrong. And there is
no need to invalite cache since it was non-present before in
remove_migration_pmd(). So just to remove it.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/huge_memory.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index f58524394dc1..45ede45b11f5 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3207,7 +3207,6 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
 	if (pmd_swp_uffd_wp(*pvmw->pmd))
 		pmde = pmd_wrprotect(pmd_mkuffd_wp(pmde));
 
-	flush_cache_range(vma, mmun_start, mmun_start + HPAGE_PMD_SIZE);
 	if (PageAnon(new))
 		page_add_anon_rmap(new, vma, mmun_start, true);
 	else
@@ -3215,6 +3214,8 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
 	set_pmd_at(mm, mmun_start, pvmw->pmd, pmde);
 	if ((vma->vm_flags & VM_LOCKED) && !PageDoubleMap(new))
 		mlock_vma_page(new);
+
+	/* No need to invalidate - it was non-present before */
 	update_mmu_cache_pmd(vma, address, pvmw->pmd);
 }
 #endif
-- 
2.11.0

[PATCH 2/2] mm: fix missing cache flush for all tail pages of THP

[Muchun Song]

The D-cache maintenance inside move_to_new_page() only consider one page,
there is still D-cache maintenance issue for tail pages of THP. Fix this
by using flush_dcache_folio().

Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/migrate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index c9296d63878d..daf2b3508670 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -934,8 +934,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
 			page->mapping = NULL;
 
 		if (likely(!is_zone_device_page(newpage)))
-			flush_dcache_page(newpage);
-
+			flush_dcache_folio(page_folio(newpage));
 	}
 out:
 	return rc;
-- 
2.11.0

Re: [PATCH 1/2] mm: thp: fix wrong cache flush in remove_migration_pmd()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

On 21 Jan 2022, at 3:13, Muchun Song wrote:

> The flush_cache_page() is supposed to be justified only if the page
> is already placed in process page table, and that is done right after
> flush_cache_page(). So using this interface is wrong. And there is
> no need to invalite cache since it was non-present before in
> remove_migration_pmd(). So just to remove it.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/huge_memory.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index f58524394dc1..45ede45b11f5 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3207,7 +3207,6 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
>  	if (pmd_swp_uffd_wp(*pvmw->pmd))
>  		pmde = pmd_wrprotect(pmd_mkuffd_wp(pmde));
>
> -	flush_cache_range(vma, mmun_start, mmun_start + HPAGE_PMD_SIZE);
>  	if (PageAnon(new))
>  		page_add_anon_rmap(new, vma, mmun_start, true);
>  	else
> @@ -3215,6 +3214,8 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
>  	set_pmd_at(mm, mmun_start, pvmw->pmd, pmde);
>  	if ((vma->vm_flags & VM_LOCKED) && !PageDoubleMap(new))
>  		mlock_vma_page(new);
> +
> +	/* No need to invalidate - it was non-present before */
>  	update_mmu_cache_pmd(vma, address, pvmw->pmd);
>  }
>  #endif

LGTM. Thanks. Reviewed-by: Zi Yan <ziy@nvidia.com>

--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 2/2] mm: fix missing cache flush for all tail pages of THP

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On 21 Jan 2022, at 3:13, 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 THP. Fix this
> by using flush_dcache_folio().
>
> Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/migrate.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index c9296d63878d..daf2b3508670 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -934,8 +934,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>  			page->mapping = NULL;
>
>  		if (likely(!is_zone_device_page(newpage)))
> -			flush_dcache_page(newpage);
> -
> +			flush_dcache_folio(page_folio(newpage));
>  	}
>  out:
>  	return rc;
> -- 
> 2.11.0

Yes, the entire THP should be flushed. But it is better
to use a for loop instead of the folio variant, so that the patch
can be ported easily to the stable trees. The for loop can be
converted later when the whole function is converted to use folio.

Thanks.

--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 2/2] mm: fix missing cache flush for all tail pages of THP

[Muchun Song]

On Fri, Jan 21, 2022 at 10:59 PM Zi Yan <ziy@nvidia.com> wrote:
>
> On 21 Jan 2022, at 3:13, 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 THP. Fix this
> > by using flush_dcache_folio().
> >
> > Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path")
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  mm/migrate.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index c9296d63878d..daf2b3508670 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -934,8 +934,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
> >                       page->mapping = NULL;
> >
> >               if (likely(!is_zone_device_page(newpage)))
> > -                     flush_dcache_page(newpage);
> > -
> > +                     flush_dcache_folio(page_folio(newpage));
> >       }
> >  out:
> >       return rc;
> > --
> > 2.11.0
>
> Yes, the entire THP should be flushed. But it is better
> to use a for loop instead of the folio variant, so that the patch
> can be ported easily to the stable trees. The for loop can be
> converted later when the whole function is converted to use folio.
>

Agree. Will do. Thanks for your review.