* [PATCH v2 1/2] mm: thp: fix wrong cache flush in remove_migration_pmd() @ 2022-01-24 5:17 Muchun Song 2022-01-24 5:17 ` [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP Muchun Song 0 siblings, 1 reply; 13+ messages in thread From: Muchun Song @ 2022-01-24 5:17 UTC (permalink / raw) To: akpm, zi.yan, kirill.shutemov Cc: linux-mm, linux-kernel, duanxiongchun, Muchun Song, Zi Yan 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> Reviewed-by: Zi Yan <ziy@nvidia.com> --- Changes in v2: - Collect Reviewed-by tag. Thanks Zi. 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-24 5:17 [PATCH v2 1/2] mm: thp: fix wrong cache flush in remove_migration_pmd() Muchun Song @ 2022-01-24 5:17 ` Muchun Song 2022-01-24 16:07 ` Zi Yan 2022-01-24 18:11 ` David Rientjes 0 siblings, 2 replies; 13+ messages in thread From: Muchun Song @ 2022-01-24 5:17 UTC (permalink / raw) To: akpm, zi.yan, kirill.shutemov Cc: linux-mm, linux-kernel, duanxiongchun, 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 not using flush_dcache_folio() since it is not backportable. Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path") Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- Changes in v2: - Using a for loop instead of the folio variant for backportable. 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); + } } out: return rc; -- 2.11.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-24 5:17 ` [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP Muchun Song @ 2022-01-24 16:07 ` Zi Yan 2022-01-24 18:11 ` David Rientjes 1 sibling, 0 replies; 13+ messages in thread From: Zi Yan @ 2022-01-24 16:07 UTC (permalink / raw) To: Muchun Song; +Cc: akpm, kirill.shutemov, linux-mm, linux-kernel, duanxiongchun [-- Attachment #1: Type: text/plain, Size: 1265 bytes --] On 24 Jan 2022, at 0:17, 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 not using flush_dcache_folio() since it is not backportable. > > Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > Changes in v2: > - Using a for loop instead of the folio variant for backportable. > > 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); > + } > } > out: > return rc; > -- > 2.11.0 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-24 5:17 ` [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP Muchun Song 2022-01-24 16:07 ` Zi Yan @ 2022-01-24 18:11 ` David Rientjes 2022-01-24 19:22 ` Zi Yan 1 sibling, 1 reply; 13+ messages in thread From: David Rientjes @ 2022-01-24 18:11 UTC (permalink / raw) To: Muchun Song Cc: akpm, zi.yan, kirill.shutemov, linux-mm, linux-kernel, duanxiongchun On Mon, 24 Jan 2022, 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 not using flush_dcache_folio() since it is not backportable. > The mention of being backportable suggests that we should backport this, likely to 4.14+. So should it be marked as stable? That aside, should there be a follow-up patch that converts to using flush_dcache_folio()? > Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > Changes in v2: > - Using a for loop instead of the folio variant for backportable. > > 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); > + } > } > out: > return rc; > -- > 2.11.0 > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-24 18:11 ` David Rientjes @ 2022-01-24 19:22 ` Zi Yan 2022-01-25 0:41 ` David Rientjes 2022-01-25 1:55 ` Muchun Song 0 siblings, 2 replies; 13+ messages in thread From: Zi Yan @ 2022-01-24 19:22 UTC (permalink / raw) To: David Rientjes, Muchun Song Cc: akpm, kirill.shutemov, linux-mm, linux-kernel, duanxiongchun, Lars Persson [-- Attachment #1: Type: text/plain, Size: 2335 bytes --] On 24 Jan 2022, at 13:11, David Rientjes wrote: > On Mon, 24 Jan 2022, 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 not using flush_dcache_folio() since it is not backportable. >> > > The mention of being backportable suggests that we should backport this, > likely to 4.14+. So should it be marked as stable? Hmm, after more digging, I am not sure if the bug exists. For THP migration, flush_cache_range() is used in remove_migration_pmd(). The flush_dcache_page() was added by Lars Persson (cc’d) to solve the data corruption on MIPS[1], but THP migration is only enabled on x86_64, PPC_BOOK3S_64, and ARM64. To make code more consistent, I guess flush_cache_range() in remove_migration_pmd() can be removed, since it is superseded by the flush_dcache_page() below. The Fixes can be dropped. Let me know if I miss anything. > > That aside, should there be a follow-up patch that converts to using > flush_dcache_folio()? Are you suggesting to convert just this code or the entire move_to_new_page() to use folio? The latter might be more desirable, since the code will be more consistent. [1] https://lore.kernel.org/all/20190315083502.11849-1-larper@axis.com/T/#u > >> Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path") >> Signed-off-by: Muchun Song <songmuchun@bytedance.com> >> --- >> Changes in v2: >> - Using a for loop instead of the folio variant for backportable. >> >> 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); >> + } >> } >> out: >> return rc; >> -- >> 2.11.0 >> >> >> -- Best Regards, Yan, Zi [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 854 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-24 19:22 ` Zi Yan @ 2022-01-25 0:41 ` David Rientjes 2022-01-25 1:55 ` Muchun Song 1 sibling, 0 replies; 13+ messages in thread From: David Rientjes @ 2022-01-25 0:41 UTC (permalink / raw) To: Zi Yan Cc: Muchun Song, akpm, kirill.shutemov, linux-mm, linux-kernel, duanxiongchun, Lars Persson [-- Attachment #1: Type: text/plain, Size: 1153 bytes --] On Mon, 24 Jan 2022, Zi Yan 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 not using flush_dcache_folio() since it is not backportable. > >> > > > > The mention of being backportable suggests that we should backport this, > > likely to 4.14+. So should it be marked as stable? > > Hmm, after more digging, I am not sure if the bug exists. For THP migration, > flush_cache_range() is used in remove_migration_pmd(). The flush_dcache_page() > was added by Lars Persson (cc’d) to solve the data corruption on MIPS[1], > but THP migration is only enabled on x86_64, PPC_BOOK3S_64, and ARM64. > > To make code more consistent, I guess flush_cache_range() in remove_migration_pmd() > can be removed, since it is superseded by the flush_dcache_page() below. > > The Fixes can be dropped. Let me know if I miss anything. > Yeah, I don't think the Fixes needs to exist here because there doesn't appear to be an issue today. We likely need to choose one of the two paths from above to handle the flush only in a single place. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-24 19:22 ` Zi Yan 2022-01-25 0:41 ` David Rientjes @ 2022-01-25 1:55 ` Muchun Song 2022-01-25 2:42 ` Zi Yan 1 sibling, 1 reply; 13+ messages in thread From: Muchun Song @ 2022-01-25 1:55 UTC (permalink / raw) To: Zi Yan Cc: David Rientjes, Andrew Morton, Kirill A. Shutemov, Linux Memory Management List, LKML, Xiongchun duan, Lars Persson On Tue, Jan 25, 2022 at 3:22 AM Zi Yan <ziy@nvidia.com> wrote: > > On 24 Jan 2022, at 13:11, David Rientjes wrote: > > > On Mon, 24 Jan 2022, 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 not using flush_dcache_folio() since it is not backportable. > >> > > > > The mention of being backportable suggests that we should backport this, > > likely to 4.14+. So should it be marked as stable? > > Hmm, after more digging, I am not sure if the bug exists. For THP migration, > flush_cache_range() is used in remove_migration_pmd(). The flush_dcache_page() > was added by Lars Persson (cc’d) to solve the data corruption on MIPS[1], > but THP migration is only enabled on x86_64, PPC_BOOK3S_64, and ARM64. I only mention the THP case. After some more thinking, I think the HugeTLB should also be considered, Right? The HugeTLB is enabled on arm, arm64, mips, parisc, powerpc, riscv, s390 and sh. > > To make code more consistent, I guess flush_cache_range() in remove_migration_pmd() > can be removed, since it is superseded by the flush_dcache_page() below. From my point of view, flush_cache_range() in remove_migration_pmd() is a wrong usage, which cannot replace flush_dcache_page(). I think the commit c2cc499c5bcf ("mm compaction: fix of improper cache flush in migration code") , which is similar to the situation here, can offer more infos. > > The Fixes can be dropped. Let me know if I miss anything. > > > > > That aside, should there be a follow-up patch that converts to using > > flush_dcache_folio()? > > Are you suggesting to convert just this code or the entire move_to_new_page() > to use folio? The latter might be more desirable, since the code will be > more consistent. > > > [1] https://lore.kernel.org/all/20190315083502.11849-1-larper@axis.com/T/#u > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-25 1:55 ` Muchun Song @ 2022-01-25 2:42 ` Zi Yan 2022-01-25 6:01 ` Muchun Song 0 siblings, 1 reply; 13+ messages in thread From: Zi Yan @ 2022-01-25 2:42 UTC (permalink / raw) To: Muchun Song Cc: David Rientjes, Andrew Morton, Kirill A. Shutemov, Linux Memory Management List, LKML, Xiongchun duan, Lars Persson, Mike Kravetz [-- Attachment #1: Type: text/plain, Size: 3442 bytes --] On 24 Jan 2022, at 20:55, Muchun Song wrote: > On Tue, Jan 25, 2022 at 3:22 AM Zi Yan <ziy@nvidia.com> wrote: >> >> On 24 Jan 2022, at 13:11, David Rientjes wrote: >> >>> On Mon, 24 Jan 2022, 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 not using flush_dcache_folio() since it is not backportable. >>>> >>> >>> The mention of being backportable suggests that we should backport this, >>> likely to 4.14+. So should it be marked as stable? >> >> Hmm, after more digging, I am not sure if the bug exists. For THP migration, >> flush_cache_range() is used in remove_migration_pmd(). The flush_dcache_page() >> was added by Lars Persson (cc’d) to solve the data corruption on MIPS[1], >> but THP migration is only enabled on x86_64, PPC_BOOK3S_64, and ARM64. > > I only mention the THP case. After some more thinking, I think the HugeTLB > should also be considered, Right? The HugeTLB is enabled on arm, arm64, > mips, parisc, powerpc, riscv, s390 and sh. > +Mike for HugeTLB If HugeTLB page migration also misses flush_dcache_page() on its tail pages, you will need a different patch for the commit introducing hugetlb page migration. >> >> To make code more consistent, I guess flush_cache_range() in remove_migration_pmd() >> can be removed, since it is superseded by the flush_dcache_page() below. > > From my point of view, flush_cache_range() in remove_migration_pmd() is > a wrong usage, which cannot replace flush_dcache_page(). I think the commit > c2cc499c5bcf ("mm compaction: fix of improper cache flush in migration code") > , which is similar to the situation here, can offer more infos. > Thanks for the information. That helps. But remove_migration_pmd() did not cause any issue at the commit pointed by Fixes but at the commit which enabled THP migration on IBM and ARM64, whichever came first. IIUC, there will be different versions of the fix targeting different stable trees: 1. pre-4.14, THP migration did not exist: you will need to fix the use of flush_dcache_page() at that time for HugeTLB page migration. Both flushing dcache page for all subpages and moving flush_dcache_page from remove_migration_pte() to move_to_new_page(). 4.9 and 4.4 are affected. But EOL of 4.4 is next month, so you might skip it. 2. 4.14 to before device public page is removed: your current fix will not apply directly, but the for loop works. flush_cache_range() in remove_migration_pmd() should be removed, since it is dead code based on the commit you mentioned. It might not be worth the effort to find when IBM and ARM64 enable THP migration. 3. after device public page is removed: your current fix will apply cleanly and the removal of flush_cache_range() in remove_migration_pmd() should be added. Let me know if it makes sense. >> >> The Fixes can be dropped. Let me know if I miss anything. >> >>> >>> That aside, should there be a follow-up patch that converts to using >>> flush_dcache_folio()? >> >> Are you suggesting to convert just this code or the entire move_to_new_page() >> to use folio? The latter might be more desirable, since the code will be >> more consistent. >> >> >> [1] https://lore.kernel.org/all/20190315083502.11849-1-larper@axis.com/T/#u >> -- Best Regards, Yan, Zi [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 854 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-25 2:42 ` Zi Yan @ 2022-01-25 6:01 ` Muchun Song 2022-01-25 21:24 ` Mike Kravetz 0 siblings, 1 reply; 13+ messages in thread From: Muchun Song @ 2022-01-25 6:01 UTC (permalink / raw) To: Zi Yan Cc: David Rientjes, Andrew Morton, Kirill A. Shutemov, Linux Memory Management List, LKML, Xiongchun duan, Lars Persson, Mike Kravetz On Tue, Jan 25, 2022 at 10:42 AM Zi Yan <ziy@nvidia.com> wrote: > > On 24 Jan 2022, at 20:55, Muchun Song wrote: > > > On Tue, Jan 25, 2022 at 3:22 AM Zi Yan <ziy@nvidia.com> wrote: > >> > >> On 24 Jan 2022, at 13:11, David Rientjes wrote: > >> > >>> On Mon, 24 Jan 2022, 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 not using flush_dcache_folio() since it is not backportable. > >>>> > >>> > >>> The mention of being backportable suggests that we should backport this, > >>> likely to 4.14+. So should it be marked as stable? > >> > >> Hmm, after more digging, I am not sure if the bug exists. For THP migration, > >> flush_cache_range() is used in remove_migration_pmd(). The flush_dcache_page() > >> was added by Lars Persson (cc’d) to solve the data corruption on MIPS[1], > >> but THP migration is only enabled on x86_64, PPC_BOOK3S_64, and ARM64. > > > > I only mention the THP case. After some more thinking, I think the HugeTLB > > should also be considered, Right? The HugeTLB is enabled on arm, arm64, > > mips, parisc, powerpc, riscv, s390 and sh. > > > > +Mike for HugeTLB > > If HugeTLB page migration also misses flush_dcache_page() on its tail pages, > you will need a different patch for the commit introducing hugetlb page migration. Agree. I think arm (see the following commit) has handled this issue, while most others do not. commit 0b19f93351dd ("ARM: mm: Add support for flushing HugeTLB pages.") But I do not have any real devices to test if this issue exists on other archs. In theory, it exists. > > >> > >> To make code more consistent, I guess flush_cache_range() in remove_migration_pmd() > >> can be removed, since it is superseded by the flush_dcache_page() below. > > > > From my point of view, flush_cache_range() in remove_migration_pmd() is > > a wrong usage, which cannot replace flush_dcache_page(). I think the commit > > c2cc499c5bcf ("mm compaction: fix of improper cache flush in migration code") > > , which is similar to the situation here, can offer more infos. > > > > Thanks for the information. That helps. But remove_migration_pmd() did not cause > any issue at the commit pointed by Fixes but at the commit which enabled THP > migration on IBM and ARM64, whichever came first. > > IIUC, there will be different versions of the fix targeting different stable > trees: > > 1. pre-4.14, THP migration did not exist: you will need to fix the use of > flush_dcache_page() at that time for HugeTLB page migration. Both flushing > dcache page for all subpages and moving flush_dcache_page from > remove_migration_pte() to move_to_new_page(). 4.9 and 4.4 are affected. > But EOL of 4.4 is next month, so you might skip it. > > 2. 4.14 to before device public page is removed: your current fix will not > apply directly, but the for loop works. flush_cache_range() in > remove_migration_pmd() should be removed, since it is dead code based on > the commit you mentioned. It might not be worth the effort to find when > IBM and ARM64 enable THP migration. > > 3. after device public page is removed: your current fix will apply cleanly > and the removal of flush_cache_range() in remove_migration_pmd() should > be added. > > Let me know if it makes sense. Make sense. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-25 6:01 ` Muchun Song @ 2022-01-25 21:24 ` Mike Kravetz 2022-01-26 3:29 ` Muchun Song 0 siblings, 1 reply; 13+ messages in thread From: Mike Kravetz @ 2022-01-25 21:24 UTC (permalink / raw) To: Muchun Song, Zi Yan Cc: David Rientjes, Andrew Morton, Kirill A. Shutemov, Linux Memory Management List, LKML, Xiongchun duan, Lars Persson On 1/24/22 22:01, Muchun Song wrote: > On Tue, Jan 25, 2022 at 10:42 AM Zi Yan <ziy@nvidia.com> wrote: >> >> On 24 Jan 2022, at 20:55, Muchun Song wrote: >> >>> On Tue, Jan 25, 2022 at 3:22 AM Zi Yan <ziy@nvidia.com> wrote: >>>> >>>> On 24 Jan 2022, at 13:11, David Rientjes wrote: >>>> >>>>> On Mon, 24 Jan 2022, 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 not using flush_dcache_folio() since it is not backportable. >>>>>> >>>>> >>>>> The mention of being backportable suggests that we should backport this, >>>>> likely to 4.14+. So should it be marked as stable? >>>> >>>> Hmm, after more digging, I am not sure if the bug exists. For THP migration, >>>> flush_cache_range() is used in remove_migration_pmd(). The flush_dcache_page() >>>> was added by Lars Persson (cc’d) to solve the data corruption on MIPS[1], >>>> but THP migration is only enabled on x86_64, PPC_BOOK3S_64, and ARM64. >>> >>> I only mention the THP case. After some more thinking, I think the HugeTLB >>> should also be considered, Right? The HugeTLB is enabled on arm, arm64, >>> mips, parisc, powerpc, riscv, s390 and sh. >>> >> >> +Mike for HugeTLB >> >> If HugeTLB page migration also misses flush_dcache_page() on its tail pages, >> you will need a different patch for the commit introducing hugetlb page migration. > > Agree. I think arm (see the following commit) has handled this issue, while most > others do not. > > commit 0b19f93351dd ("ARM: mm: Add support for flushing HugeTLB pages.") > > But I do not have any real devices to test if this issue exists on other archs. > In theory, it exists. > Thanks for adding me to the discussion. I agree that this issue exists at least in theory for hugetlb pages as well. This made me look at other places with similar code for hugetlb. i.e. Allocating a new page, copying data to new page and then establishing a mapping (pte) to the new page. - hugetlb_cow calls copy_user_huge_page() which ends up calling copy_user_highpage that includes dcache flushing of the target for some architectures, but not all. - userfaultfd calls copy_huge_page_from_user which does not appear to do any dcache flushing for the target page. Do you think these code paths have the same potential issue? -- Mike Kravetz ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-25 21:24 ` Mike Kravetz @ 2022-01-26 3:29 ` Muchun Song 2022-01-26 23:26 ` Mike Kravetz 0 siblings, 1 reply; 13+ messages in thread From: Muchun Song @ 2022-01-26 3:29 UTC (permalink / raw) To: Mike Kravetz Cc: Zi Yan, David Rientjes, Andrew Morton, Kirill A. Shutemov, Linux Memory Management List, LKML, Xiongchun duan, Lars Persson On Wed, Jan 26, 2022 at 5:24 AM Mike Kravetz <mike.kravetz@oracle.com> wrote: > > On 1/24/22 22:01, Muchun Song wrote: > > On Tue, Jan 25, 2022 at 10:42 AM Zi Yan <ziy@nvidia.com> wrote: > >> > >> On 24 Jan 2022, at 20:55, Muchun Song wrote: > >> > >>> On Tue, Jan 25, 2022 at 3:22 AM Zi Yan <ziy@nvidia.com> wrote: > >>>> > >>>> On 24 Jan 2022, at 13:11, David Rientjes wrote: > >>>> > >>>>> On Mon, 24 Jan 2022, 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 not using flush_dcache_folio() since it is not backportable. > >>>>>> > >>>>> > >>>>> The mention of being backportable suggests that we should backport this, > >>>>> likely to 4.14+. So should it be marked as stable? > >>>> > >>>> Hmm, after more digging, I am not sure if the bug exists. For THP migration, > >>>> flush_cache_range() is used in remove_migration_pmd(). The flush_dcache_page() > >>>> was added by Lars Persson (cc’d) to solve the data corruption on MIPS[1], > >>>> but THP migration is only enabled on x86_64, PPC_BOOK3S_64, and ARM64. > >>> > >>> I only mention the THP case. After some more thinking, I think the HugeTLB > >>> should also be considered, Right? The HugeTLB is enabled on arm, arm64, > >>> mips, parisc, powerpc, riscv, s390 and sh. > >>> > >> > >> +Mike for HugeTLB > >> > >> If HugeTLB page migration also misses flush_dcache_page() on its tail pages, > >> you will need a different patch for the commit introducing hugetlb page migration. > > > > Agree. I think arm (see the following commit) has handled this issue, while most > > others do not. > > > > commit 0b19f93351dd ("ARM: mm: Add support for flushing HugeTLB pages.") > > > > But I do not have any real devices to test if this issue exists on other archs. > > In theory, it exists. > > > > Thanks for adding me to the discussion. > > I agree that this issue exists at least in theory for hugetlb pages as well. > This made me look at other places with similar code for hugetlb. i.e. > Allocating a new page, copying data to new page and then establishing a > mapping (pte) to the new page. Hi Mike, Thanks for looking at this. > > - hugetlb_cow calls copy_user_huge_page() which ends up calling > copy_user_highpage that includes dcache flushing of the target for some > architectures, but not all. copy_user_page() inside copy_user_highpage() is already considering the cache maintenance on different architectures, which is documented in Documentation/core-api/cachetlb.rst. So there are no problems in this case. > - userfaultfd calls copy_huge_page_from_user which does not appear to do > any dcache flushing for the target page. Right. The new page should be flushed before setting up the mapping to the user space. > Do you think these code paths have the same potential issue? The latter does have the issue, the former does not. The fixes may look like the following: diff --git a/mm/hugetlb.c b/mm/hugetlb.c index a1baa198519a..828240aee3f9 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -5819,6 +5819,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, goto out; } folio_copy(page_folio(page), page_folio(*pagep)); + flush_dcache_folio(page_folio(page)); put_page(*pagep); *pagep = NULL; } diff --git a/mm/memory.c b/mm/memory.c index e8ce066be5f2..ff6f48cdcc48 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -5400,6 +5400,7 @@ long copy_huge_page_from_user(struct page *dst_page, kunmap(subpage); else kunmap_atomic(page_kaddr); + flush_dcache_page(subpage); ret_val -= (PAGE_SIZE - rc); if (rc) Thanks. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-26 3:29 ` Muchun Song @ 2022-01-26 23:26 ` Mike Kravetz 2022-01-27 1:55 ` Muchun Song 0 siblings, 1 reply; 13+ messages in thread From: Mike Kravetz @ 2022-01-26 23:26 UTC (permalink / raw) To: Muchun Song Cc: Zi Yan, David Rientjes, Andrew Morton, Kirill A. Shutemov, Linux Memory Management List, LKML, Xiongchun duan, Lars Persson On 1/25/22 19:29, Muchun Song wrote: > On Wed, Jan 26, 2022 at 5:24 AM Mike Kravetz <mike.kravetz@oracle.com> wrote: >> >> On 1/24/22 22:01, Muchun Song wrote: >>> On Tue, Jan 25, 2022 at 10:42 AM Zi Yan <ziy@nvidia.com> wrote: >>>> >>>> On 24 Jan 2022, at 20:55, Muchun Song wrote: >>>> >>>>> On Tue, Jan 25, 2022 at 3:22 AM Zi Yan <ziy@nvidia.com> wrote: >>>>>> >>>>>> On 24 Jan 2022, at 13:11, David Rientjes wrote: >>>>>> >>>>>>> On Mon, 24 Jan 2022, 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 not using flush_dcache_folio() since it is not backportable. >>>>>>>> >>>>>>> >>>>>>> The mention of being backportable suggests that we should backport this, >>>>>>> likely to 4.14+. So should it be marked as stable? >>>>>> >>>>>> Hmm, after more digging, I am not sure if the bug exists. For THP migration, >>>>>> flush_cache_range() is used in remove_migration_pmd(). The flush_dcache_page() >>>>>> was added by Lars Persson (cc’d) to solve the data corruption on MIPS[1], >>>>>> but THP migration is only enabled on x86_64, PPC_BOOK3S_64, and ARM64. >>>>> >>>>> I only mention the THP case. After some more thinking, I think the HugeTLB >>>>> should also be considered, Right? The HugeTLB is enabled on arm, arm64, >>>>> mips, parisc, powerpc, riscv, s390 and sh. >>>>> >>>> >>>> +Mike for HugeTLB >>>> >>>> If HugeTLB page migration also misses flush_dcache_page() on its tail pages, >>>> you will need a different patch for the commit introducing hugetlb page migration. >>> >>> Agree. I think arm (see the following commit) has handled this issue, while most >>> others do not. >>> >>> commit 0b19f93351dd ("ARM: mm: Add support for flushing HugeTLB pages.") >>> >>> But I do not have any real devices to test if this issue exists on other archs. >>> In theory, it exists. >>> >> >> Thanks for adding me to the discussion. >> >> I agree that this issue exists at least in theory for hugetlb pages as well. >> This made me look at other places with similar code for hugetlb. i.e. >> Allocating a new page, copying data to new page and then establishing a >> mapping (pte) to the new page. > > Hi Mike, > > Thanks for looking at this. > >> >> - hugetlb_cow calls copy_user_huge_page() which ends up calling >> copy_user_highpage that includes dcache flushing of the target for some >> architectures, but not all. > > copy_user_page() inside copy_user_highpage() is already considering > the cache maintenance on different architectures, which is documented > in Documentation/core-api/cachetlb.rst. So there are no problems in this > case. > Thanks! That cleared up some of my confusion. >> - userfaultfd calls copy_huge_page_from_user which does not appear to do >> any dcache flushing for the target page. > > Right. The new page should be flushed before setting up the mapping > to the user space. > >> Do you think these code paths have the same potential issue? > > The latter does have the issue, the former does not. The fixes may > look like the following: > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index a1baa198519a..828240aee3f9 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -5819,6 +5819,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > goto out; > } > folio_copy(page_folio(page), page_folio(*pagep)); > + flush_dcache_folio(page_folio(page)); > put_page(*pagep); > *pagep = NULL; > } > diff --git a/mm/memory.c b/mm/memory.c > index e8ce066be5f2..ff6f48cdcc48 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -5400,6 +5400,7 @@ long copy_huge_page_from_user(struct page *dst_page, > kunmap(subpage); > else > kunmap_atomic(page_kaddr); > + flush_dcache_page(subpage); > > ret_val -= (PAGE_SIZE - rc); > if (rc) > That looks good to me. Do you plan to include this in the next version of this series? -- Mike Kravetz ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP 2022-01-26 23:26 ` Mike Kravetz @ 2022-01-27 1:55 ` Muchun Song 0 siblings, 0 replies; 13+ messages in thread From: Muchun Song @ 2022-01-27 1:55 UTC (permalink / raw) To: Mike Kravetz Cc: Zi Yan, David Rientjes, Andrew Morton, Kirill A. Shutemov, Linux Memory Management List, LKML, Xiongchun duan, Lars Persson On Thu, Jan 27, 2022 at 7:27 AM Mike Kravetz <mike.kravetz@oracle.com> wrote: > > On 1/25/22 19:29, Muchun Song wrote: > > On Wed, Jan 26, 2022 at 5:24 AM Mike Kravetz <mike.kravetz@oracle.com> wrote: > >> > >> On 1/24/22 22:01, Muchun Song wrote: > >>> On Tue, Jan 25, 2022 at 10:42 AM Zi Yan <ziy@nvidia.com> wrote: > >>>> > >>>> On 24 Jan 2022, at 20:55, Muchun Song wrote: > >>>> > >>>>> On Tue, Jan 25, 2022 at 3:22 AM Zi Yan <ziy@nvidia.com> wrote: > >>>>>> > >>>>>> On 24 Jan 2022, at 13:11, David Rientjes wrote: > >>>>>> > >>>>>>> On Mon, 24 Jan 2022, 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 not using flush_dcache_folio() since it is not backportable. > >>>>>>>> > >>>>>>> > >>>>>>> The mention of being backportable suggests that we should backport this, > >>>>>>> likely to 4.14+. So should it be marked as stable? > >>>>>> > >>>>>> Hmm, after more digging, I am not sure if the bug exists. For THP migration, > >>>>>> flush_cache_range() is used in remove_migration_pmd(). The flush_dcache_page() > >>>>>> was added by Lars Persson (cc’d) to solve the data corruption on MIPS[1], > >>>>>> but THP migration is only enabled on x86_64, PPC_BOOK3S_64, and ARM64. > >>>>> > >>>>> I only mention the THP case. After some more thinking, I think the HugeTLB > >>>>> should also be considered, Right? The HugeTLB is enabled on arm, arm64, > >>>>> mips, parisc, powerpc, riscv, s390 and sh. > >>>>> > >>>> > >>>> +Mike for HugeTLB > >>>> > >>>> If HugeTLB page migration also misses flush_dcache_page() on its tail pages, > >>>> you will need a different patch for the commit introducing hugetlb page migration. > >>> > >>> Agree. I think arm (see the following commit) has handled this issue, while most > >>> others do not. > >>> > >>> commit 0b19f93351dd ("ARM: mm: Add support for flushing HugeTLB pages.") > >>> > >>> But I do not have any real devices to test if this issue exists on other archs. > >>> In theory, it exists. > >>> > >> > >> Thanks for adding me to the discussion. > >> > >> I agree that this issue exists at least in theory for hugetlb pages as well. > >> This made me look at other places with similar code for hugetlb. i.e. > >> Allocating a new page, copying data to new page and then establishing a > >> mapping (pte) to the new page. > > > > Hi Mike, > > > > Thanks for looking at this. > > > >> > >> - hugetlb_cow calls copy_user_huge_page() which ends up calling > >> copy_user_highpage that includes dcache flushing of the target for some > >> architectures, but not all. > > > > copy_user_page() inside copy_user_highpage() is already considering > > the cache maintenance on different architectures, which is documented > > in Documentation/core-api/cachetlb.rst. So there are no problems in this > > case. > > > > Thanks! That cleared up some of my confusion. > > > >> - userfaultfd calls copy_huge_page_from_user which does not appear to do > >> any dcache flushing for the target page. > > > > Right. The new page should be flushed before setting up the mapping > > to the user space. > > > >> Do you think these code paths have the same potential issue? > > > > The latter does have the issue, the former does not. The fixes may > > look like the following: > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index a1baa198519a..828240aee3f9 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -5819,6 +5819,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > > goto out; > > } > > folio_copy(page_folio(page), page_folio(*pagep)); > > + flush_dcache_folio(page_folio(page)); > > put_page(*pagep); > > *pagep = NULL; > > } > > diff --git a/mm/memory.c b/mm/memory.c > > index e8ce066be5f2..ff6f48cdcc48 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -5400,6 +5400,7 @@ long copy_huge_page_from_user(struct page *dst_page, > > kunmap(subpage); > > else > > kunmap_atomic(page_kaddr); > > + flush_dcache_page(subpage); > > > > ret_val -= (PAGE_SIZE - rc); > > if (rc) > > > > That looks good to me. Do you plan to include this in the next version > of this series? Yes, will do. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-01-27 1:55 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-24 5:17 [PATCH v2 1/2] mm: thp: fix wrong cache flush in remove_migration_pmd() Muchun Song 2022-01-24 5:17 ` [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of THP Muchun Song 2022-01-24 16:07 ` Zi Yan 2022-01-24 18:11 ` David Rientjes 2022-01-24 19:22 ` Zi Yan 2022-01-25 0:41 ` David Rientjes 2022-01-25 1:55 ` Muchun Song 2022-01-25 2:42 ` Zi Yan 2022-01-25 6:01 ` Muchun Song 2022-01-25 21:24 ` Mike Kravetz 2022-01-26 3:29 ` Muchun Song 2022-01-26 23:26 ` Mike Kravetz 2022-01-27 1:55 ` Muchun Song
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.