| Message ID | 20230518110727.2106156-3-ryan.roberts@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Encapsulate PTE contents from non-arch code | expand |
Hi Ryan, Nit. For consistency of DAMON commits subjects, I'd prefer something like "mm/damon/ops-common: atomically test and clear young on ptes and pmds". On Thu, 18 May 2023 12:07:24 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote: > It is racy to non-atomically read a pte, then clear the young bit, then > write it back as this could discard dirty information. Further, it is > bad practice to directly set a pte entry within a table. Instead > clearing young must go through the arch-provided helper, > ptep_test_and_clear_young() to ensure it is modified atomically and to > give the arch code visibility and allow it to check (and potentially > modify) the operation. > > Fixes: 46c3a0accdc4 ("mm/damon/vaddr: separate commonly usable functions") Nit. The commit is only for refactoring. The original code camde from commit 3f49584b262c ("mm/damon: implement primitives for the virtual memory address spaces"). > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > Reviewed-by: Zi Yan <ziy@nvidia.com> Other than above nits, Reviewed-by: SeongJae Park <sj@kernel.org> Thanks, SJ > --- > mm/damon/ops-common.c | 16 ++++++---------- > mm/damon/ops-common.h | 4 ++-- > mm/damon/paddr.c | 4 ++-- > mm/damon/vaddr.c | 4 ++-- > 4 files changed, 12 insertions(+), 16 deletions(-) > > diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c > index cc63cf953636..acc264b97903 100644 > --- a/mm/damon/ops-common.c > +++ b/mm/damon/ops-common.c > @@ -37,7 +37,7 @@ struct folio *damon_get_folio(unsigned long pfn) > return folio; > } > > -void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > +void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr) > { > bool referenced = false; > struct folio *folio = damon_get_folio(pte_pfn(*pte)); > @@ -45,13 +45,11 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > if (!folio) > return; > > - if (pte_young(*pte)) { > + if (ptep_test_and_clear_young(vma, addr, pte)) > referenced = true; > - *pte = pte_mkold(*pte); > - } > > #ifdef CONFIG_MMU_NOTIFIER > - if (mmu_notifier_clear_young(mm, addr, addr + PAGE_SIZE)) > + if (mmu_notifier_clear_young(vma->vm_mm, addr, addr + PAGE_SIZE)) > referenced = true; > #endif /* CONFIG_MMU_NOTIFIER */ > > @@ -62,7 +60,7 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > folio_put(folio); > } > > -void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr) > +void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr) > { > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > bool referenced = false; > @@ -71,13 +69,11 @@ void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr) > if (!folio) > return; > > - if (pmd_young(*pmd)) { > + if (pmdp_test_and_clear_young(vma, addr, pmd)) > referenced = true; > - *pmd = pmd_mkold(*pmd); > - } > > #ifdef CONFIG_MMU_NOTIFIER > - if (mmu_notifier_clear_young(mm, addr, addr + HPAGE_PMD_SIZE)) > + if (mmu_notifier_clear_young(vma->vm_mm, addr, addr + HPAGE_PMD_SIZE)) > referenced = true; > #endif /* CONFIG_MMU_NOTIFIER */ > > diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h > index 14f4bc69f29b..18d837d11bce 100644 > --- a/mm/damon/ops-common.h > +++ b/mm/damon/ops-common.h > @@ -9,8 +9,8 @@ > > struct folio *damon_get_folio(unsigned long pfn); > > -void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr); > -void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr); > +void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr); > +void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr); > > int damon_cold_score(struct damon_ctx *c, struct damon_region *r, > struct damos *s); > diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c > index 467b99166b43..5b3a3463d078 100644 > --- a/mm/damon/paddr.c > +++ b/mm/damon/paddr.c > @@ -24,9 +24,9 @@ static bool __damon_pa_mkold(struct folio *folio, struct vm_area_struct *vma, > while (page_vma_mapped_walk(&pvmw)) { > addr = pvmw.address; > if (pvmw.pte) > - damon_ptep_mkold(pvmw.pte, vma->vm_mm, addr); > + damon_ptep_mkold(pvmw.pte, vma, addr); > else > - damon_pmdp_mkold(pvmw.pmd, vma->vm_mm, addr); > + damon_pmdp_mkold(pvmw.pmd, vma, addr); > } > return true; > } > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c > index 1fec16d7263e..37994fb6120c 100644 > --- a/mm/damon/vaddr.c > +++ b/mm/damon/vaddr.c > @@ -311,7 +311,7 @@ static int damon_mkold_pmd_entry(pmd_t *pmd, unsigned long addr, > } > > if (pmd_trans_huge(*pmd)) { > - damon_pmdp_mkold(pmd, walk->mm, addr); > + damon_pmdp_mkold(pmd, walk->vma, addr); > spin_unlock(ptl); > return 0; > } > @@ -323,7 +323,7 @@ static int damon_mkold_pmd_entry(pmd_t *pmd, unsigned long addr, > pte = pte_offset_map_lock(walk->mm, pmd, addr, &ptl); > if (!pte_present(*pte)) > goto out; > - damon_ptep_mkold(pte, walk->mm, addr); > + damon_ptep_mkold(pte, walk->vma, addr); > out: > pte_unmap_unlock(pte, ptl); > return 0; > -- > 2.25.1 > >
On Thu, May 18, 2023 at 5:07 AM Ryan Roberts <ryan.roberts@arm.com> wrote: > > It is racy to non-atomically read a pte, then clear the young bit, then > write it back as this could discard dirty information. Further, it is > bad practice to directly set a pte entry within a table. Instead > clearing young must go through the arch-provided helper, > ptep_test_and_clear_young() to ensure it is modified atomically and to > give the arch code visibility and allow it to check (and potentially > modify) the operation. > > Fixes: 46c3a0accdc4 ("mm/damon/vaddr: separate commonly usable functions") This should be a separate patch, since it's independent from what the series tries to do. And that patch should cc stable, since it fixes user data corruption. > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > Reviewed-by: Zi Yan <ziy@nvidia.com> > --- > mm/damon/ops-common.c | 16 ++++++---------- > mm/damon/ops-common.h | 4 ++-- > mm/damon/paddr.c | 4 ++-- > mm/damon/vaddr.c | 4 ++-- > 4 files changed, 12 insertions(+), 16 deletions(-) > > diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c > index cc63cf953636..acc264b97903 100644 > --- a/mm/damon/ops-common.c > +++ b/mm/damon/ops-common.c > @@ -37,7 +37,7 @@ struct folio *damon_get_folio(unsigned long pfn) > return folio; > } > > -void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > +void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr) > { > bool referenced = false; > struct folio *folio = damon_get_folio(pte_pfn(*pte)); > @@ -45,13 +45,11 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > if (!folio) > return; > > - if (pte_young(*pte)) { > + if (ptep_test_and_clear_young(vma, addr, pte)) > referenced = true; > - *pte = pte_mkold(*pte); > - } > > #ifdef CONFIG_MMU_NOTIFIER > - if (mmu_notifier_clear_young(mm, addr, addr + PAGE_SIZE)) > + if (mmu_notifier_clear_young(vma->vm_mm, addr, addr + PAGE_SIZE)) > referenced = true; > #endif /* CONFIG_MMU_NOTIFIER */ Use ptep_clear_young_notify(). Similar below.
On 18/05/2023 18:13, SeongJae Park wrote: > Hi Ryan, > > > Nit. For consistency of DAMON commits subjects, I'd prefer something like > "mm/damon/ops-common: atomically test and clear young on ptes and pmds". > > On Thu, 18 May 2023 12:07:24 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote: > >> It is racy to non-atomically read a pte, then clear the young bit, then >> write it back as this could discard dirty information. Further, it is >> bad practice to directly set a pte entry within a table. Instead >> clearing young must go through the arch-provided helper, >> ptep_test_and_clear_young() to ensure it is modified atomically and to >> give the arch code visibility and allow it to check (and potentially >> modify) the operation. >> >> Fixes: 46c3a0accdc4 ("mm/damon/vaddr: separate commonly usable functions") > > Nit. The commit is only for refactoring. The original code camde from commit > 3f49584b262c ("mm/damon: implement primitives for the virtual memory address > spaces"). > >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >> Reviewed-by: Zi Yan <ziy@nvidia.com> > > Other than above nits, > > Reviewed-by: SeongJae Park <sj@kernel.org> Thanks for the reviews! I'll fix the nits you raised for v3. > > > Thanks, > SJ >
On 19/05/2023 00:19, Yu Zhao wrote: > On Thu, May 18, 2023 at 5:07 AM Ryan Roberts <ryan.roberts@arm.com> wrote: >> >> It is racy to non-atomically read a pte, then clear the young bit, then >> write it back as this could discard dirty information. Further, it is >> bad practice to directly set a pte entry within a table. Instead >> clearing young must go through the arch-provided helper, >> ptep_test_and_clear_young() to ensure it is modified atomically and to >> give the arch code visibility and allow it to check (and potentially >> modify) the operation. >> >> Fixes: 46c3a0accdc4 ("mm/damon/vaddr: separate commonly usable functions") > > This should be a separate patch, since it's independent from what the > series tries to do. > > And that patch should cc stable, since it fixes user data corruption. Fair point. The first 3 patches are fixes for issues I found during the conversion. So if you're ok with it, I'll split into 2 series; the first 3 patches in the first, and the conversion to ptep_deref() (or ptep_get(), as per your comments in the other thread) in the second. I guess the whole first series should go to stable. Let me know if you disagree. > >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >> Reviewed-by: Zi Yan <ziy@nvidia.com> >> --- >> mm/damon/ops-common.c | 16 ++++++---------- >> mm/damon/ops-common.h | 4 ++-- >> mm/damon/paddr.c | 4 ++-- >> mm/damon/vaddr.c | 4 ++-- >> 4 files changed, 12 insertions(+), 16 deletions(-) >> >> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c >> index cc63cf953636..acc264b97903 100644 >> --- a/mm/damon/ops-common.c >> +++ b/mm/damon/ops-common.c >> @@ -37,7 +37,7 @@ struct folio *damon_get_folio(unsigned long pfn) >> return folio; >> } >> >> -void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) >> +void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr) >> { >> bool referenced = false; >> struct folio *folio = damon_get_folio(pte_pfn(*pte)); >> @@ -45,13 +45,11 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) >> if (!folio) >> return; >> >> - if (pte_young(*pte)) { >> + if (ptep_test_and_clear_young(vma, addr, pte)) >> referenced = true; >> - *pte = pte_mkold(*pte); >> - } >> >> #ifdef CONFIG_MMU_NOTIFIER >> - if (mmu_notifier_clear_young(mm, addr, addr + PAGE_SIZE)) >> + if (mmu_notifier_clear_young(vma->vm_mm, addr, addr + PAGE_SIZE)) >> referenced = true; >> #endif /* CONFIG_MMU_NOTIFIER */ > > Use ptep_clear_young_notify(). Similar below. This looks sensible but I'd like clarification from SeongJae: Are you happy for me to do this refactoring as part of the patch? Thanks, Ryan
On Fri, 19 May 2023 10:02:48 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote: > On 19/05/2023 00:19, Yu Zhao wrote: > > On Thu, May 18, 2023 at 5:07 AM Ryan Roberts <ryan.roberts@arm.com> wrote: > >> > >> It is racy to non-atomically read a pte, then clear the young bit, then > >> write it back as this could discard dirty information. Further, it is > >> bad practice to directly set a pte entry within a table. Instead > >> clearing young must go through the arch-provided helper, > >> ptep_test_and_clear_young() to ensure it is modified atomically and to > >> give the arch code visibility and allow it to check (and potentially > >> modify) the operation. > >> > >> Fixes: 46c3a0accdc4 ("mm/damon/vaddr: separate commonly usable functions") > > > > This should be a separate patch, since it's independent from what the > > series tries to do. > > > > And that patch should cc stable, since it fixes user data corruption. > > Fair point. The first 3 patches are fixes for issues I found during the > conversion. So if you're ok with it, I'll split into 2 series; the first 3 > patches in the first, and the conversion to ptep_deref() (or ptep_get(), as per > your comments in the other thread) in the second. I guess the whole first series > should go to stable. > > Let me know if you disagree. > > > > >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > >> Reviewed-by: Zi Yan <ziy@nvidia.com> > >> --- > >> mm/damon/ops-common.c | 16 ++++++---------- > >> mm/damon/ops-common.h | 4 ++-- > >> mm/damon/paddr.c | 4 ++-- > >> mm/damon/vaddr.c | 4 ++-- > >> 4 files changed, 12 insertions(+), 16 deletions(-) > >> > >> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c > >> index cc63cf953636..acc264b97903 100644 > >> --- a/mm/damon/ops-common.c > >> +++ b/mm/damon/ops-common.c > >> @@ -37,7 +37,7 @@ struct folio *damon_get_folio(unsigned long pfn) > >> return folio; > >> } > >> > >> -void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > >> +void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr) > >> { > >> bool referenced = false; > >> struct folio *folio = damon_get_folio(pte_pfn(*pte)); > >> @@ -45,13 +45,11 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > >> if (!folio) > >> return; > >> > >> - if (pte_young(*pte)) { > >> + if (ptep_test_and_clear_young(vma, addr, pte)) > >> referenced = true; > >> - *pte = pte_mkold(*pte); > >> - } > >> > >> #ifdef CONFIG_MMU_NOTIFIER > >> - if (mmu_notifier_clear_young(mm, addr, addr + PAGE_SIZE)) > >> + if (mmu_notifier_clear_young(vma->vm_mm, addr, addr + PAGE_SIZE)) > >> referenced = true; > >> #endif /* CONFIG_MMU_NOTIFIER */ > > > > Use ptep_clear_young_notify(). Similar below. > > This looks sensible but I'd like clarification from SeongJae: Are you happy for > me to do this refactoring as part of the patch? Yes, I would be happy for that :) Nevertheless, because that's a refactoring rather than a fix, I'd be happier if you could do that with a separate patch. Thanks, SJ > > Thanks, > Ryan >
On 19/05/2023 20:54, SeongJae Park wrote: > On Fri, 19 May 2023 10:02:48 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote: > >> On 19/05/2023 00:19, Yu Zhao wrote: >>> On Thu, May 18, 2023 at 5:07 AM Ryan Roberts <ryan.roberts@arm.com> wrote: >>>> >>>> It is racy to non-atomically read a pte, then clear the young bit, then >>>> write it back as this could discard dirty information. Further, it is >>>> bad practice to directly set a pte entry within a table. Instead >>>> clearing young must go through the arch-provided helper, >>>> ptep_test_and_clear_young() to ensure it is modified atomically and to >>>> give the arch code visibility and allow it to check (and potentially >>>> modify) the operation. >>>> >>>> Fixes: 46c3a0accdc4 ("mm/damon/vaddr: separate commonly usable functions") >>> >>> This should be a separate patch, since it's independent from what the >>> series tries to do. >>> >>> And that patch should cc stable, since it fixes user data corruption. >> >> Fair point. The first 3 patches are fixes for issues I found during the >> conversion. So if you're ok with it, I'll split into 2 series; the first 3 >> patches in the first, and the conversion to ptep_deref() (or ptep_get(), as per >> your comments in the other thread) in the second. I guess the whole first series >> should go to stable. >> >> Let me know if you disagree. >> >>> >>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >>>> Reviewed-by: Zi Yan <ziy@nvidia.com> >>>> --- >>>> mm/damon/ops-common.c | 16 ++++++---------- >>>> mm/damon/ops-common.h | 4 ++-- >>>> mm/damon/paddr.c | 4 ++-- >>>> mm/damon/vaddr.c | 4 ++-- >>>> 4 files changed, 12 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c >>>> index cc63cf953636..acc264b97903 100644 >>>> --- a/mm/damon/ops-common.c >>>> +++ b/mm/damon/ops-common.c >>>> @@ -37,7 +37,7 @@ struct folio *damon_get_folio(unsigned long pfn) >>>> return folio; >>>> } >>>> >>>> -void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) >>>> +void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr) >>>> { >>>> bool referenced = false; >>>> struct folio *folio = damon_get_folio(pte_pfn(*pte)); >>>> @@ -45,13 +45,11 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) >>>> if (!folio) >>>> return; >>>> >>>> - if (pte_young(*pte)) { >>>> + if (ptep_test_and_clear_young(vma, addr, pte)) >>>> referenced = true; >>>> - *pte = pte_mkold(*pte); >>>> - } >>>> >>>> #ifdef CONFIG_MMU_NOTIFIER >>>> - if (mmu_notifier_clear_young(mm, addr, addr + PAGE_SIZE)) >>>> + if (mmu_notifier_clear_young(vma->vm_mm, addr, addr + PAGE_SIZE)) >>>> referenced = true; >>>> #endif /* CONFIG_MMU_NOTIFIER */ >>> >>> Use ptep_clear_young_notify(). Similar below. >> >> This looks sensible but I'd like clarification from SeongJae: Are you happy for >> me to do this refactoring as part of the patch? > > Yes, I would be happy for that :) Nevertheless, because that's a refactoring > rather than a fix, I'd be happier if you could do that with a separate patch. Great - I'll do it like this for v3. > > > Thanks, > SJ > >> >> Thanks, >> Ryan >>
On Thu, May 18, 2023 at 12:07:24PM +0100, Ryan Roberts wrote: > It is racy to non-atomically read a pte, then clear the young bit, then > write it back as this could discard dirty information. Further, it is > bad practice to directly set a pte entry within a table. Instead > clearing young must go through the arch-provided helper, > ptep_test_and_clear_young() to ensure it is modified atomically and to > give the arch code visibility and allow it to check (and potentially > modify) the operation. > > Fixes: 46c3a0accdc4 ("mm/damon/vaddr: separate commonly usable functions") > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > Reviewed-by: Zi Yan <ziy@nvidia.com> Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org> > --- > mm/damon/ops-common.c | 16 ++++++---------- > mm/damon/ops-common.h | 4 ++-- > mm/damon/paddr.c | 4 ++-- > mm/damon/vaddr.c | 4 ++-- > 4 files changed, 12 insertions(+), 16 deletions(-) > > diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c > index cc63cf953636..acc264b97903 100644 > --- a/mm/damon/ops-common.c > +++ b/mm/damon/ops-common.c > @@ -37,7 +37,7 @@ struct folio *damon_get_folio(unsigned long pfn) > return folio; > } > > -void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > +void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr) > { > bool referenced = false; > struct folio *folio = damon_get_folio(pte_pfn(*pte)); > @@ -45,13 +45,11 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > if (!folio) > return; > > - if (pte_young(*pte)) { > + if (ptep_test_and_clear_young(vma, addr, pte)) > referenced = true; > - *pte = pte_mkold(*pte); > - } > > #ifdef CONFIG_MMU_NOTIFIER > - if (mmu_notifier_clear_young(mm, addr, addr + PAGE_SIZE)) > + if (mmu_notifier_clear_young(vma->vm_mm, addr, addr + PAGE_SIZE)) > referenced = true; > #endif /* CONFIG_MMU_NOTIFIER */ > > @@ -62,7 +60,7 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > folio_put(folio); > } > > -void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr) > +void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr) > { > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > bool referenced = false; > @@ -71,13 +69,11 @@ void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr) > if (!folio) > return; > > - if (pmd_young(*pmd)) { > + if (pmdp_test_and_clear_young(vma, addr, pmd)) > referenced = true; > - *pmd = pmd_mkold(*pmd); > - } > > #ifdef CONFIG_MMU_NOTIFIER > - if (mmu_notifier_clear_young(mm, addr, addr + HPAGE_PMD_SIZE)) > + if (mmu_notifier_clear_young(vma->vm_mm, addr, addr + HPAGE_PMD_SIZE)) > referenced = true; > #endif /* CONFIG_MMU_NOTIFIER */ > > diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h > index 14f4bc69f29b..18d837d11bce 100644 > --- a/mm/damon/ops-common.h > +++ b/mm/damon/ops-common.h > @@ -9,8 +9,8 @@ > > struct folio *damon_get_folio(unsigned long pfn); > > -void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr); > -void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr); > +void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr); > +void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr); > > int damon_cold_score(struct damon_ctx *c, struct damon_region *r, > struct damos *s); > diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c > index 467b99166b43..5b3a3463d078 100644 > --- a/mm/damon/paddr.c > +++ b/mm/damon/paddr.c > @@ -24,9 +24,9 @@ static bool __damon_pa_mkold(struct folio *folio, struct vm_area_struct *vma, > while (page_vma_mapped_walk(&pvmw)) { > addr = pvmw.address; > if (pvmw.pte) > - damon_ptep_mkold(pvmw.pte, vma->vm_mm, addr); > + damon_ptep_mkold(pvmw.pte, vma, addr); > else > - damon_pmdp_mkold(pvmw.pmd, vma->vm_mm, addr); > + damon_pmdp_mkold(pvmw.pmd, vma, addr); > } > return true; > } > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c > index 1fec16d7263e..37994fb6120c 100644 > --- a/mm/damon/vaddr.c > +++ b/mm/damon/vaddr.c > @@ -311,7 +311,7 @@ static int damon_mkold_pmd_entry(pmd_t *pmd, unsigned long addr, > } > > if (pmd_trans_huge(*pmd)) { > - damon_pmdp_mkold(pmd, walk->mm, addr); > + damon_pmdp_mkold(pmd, walk->vma, addr); > spin_unlock(ptl); > return 0; > } > @@ -323,7 +323,7 @@ static int damon_mkold_pmd_entry(pmd_t *pmd, unsigned long addr, > pte = pte_offset_map_lock(walk->mm, pmd, addr, &ptl); > if (!pte_present(*pte)) > goto out; > - damon_ptep_mkold(pte, walk->mm, addr); > + damon_ptep_mkold(pte, walk->vma, addr); > out: > pte_unmap_unlock(pte, ptl); > return 0; > -- > 2.25.1 > >
diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c index cc63cf953636..acc264b97903 100644 --- a/mm/damon/ops-common.c +++ b/mm/damon/ops-common.c @@ -37,7 +37,7 @@ struct folio *damon_get_folio(unsigned long pfn) return folio; } -void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) +void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr) { bool referenced = false; struct folio *folio = damon_get_folio(pte_pfn(*pte)); @@ -45,13 +45,11 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) if (!folio) return; - if (pte_young(*pte)) { + if (ptep_test_and_clear_young(vma, addr, pte)) referenced = true; - *pte = pte_mkold(*pte); - } #ifdef CONFIG_MMU_NOTIFIER - if (mmu_notifier_clear_young(mm, addr, addr + PAGE_SIZE)) + if (mmu_notifier_clear_young(vma->vm_mm, addr, addr + PAGE_SIZE)) referenced = true; #endif /* CONFIG_MMU_NOTIFIER */ @@ -62,7 +60,7 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) folio_put(folio); } -void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr) +void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr) { #ifdef CONFIG_TRANSPARENT_HUGEPAGE bool referenced = false; @@ -71,13 +69,11 @@ void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr) if (!folio) return; - if (pmd_young(*pmd)) { + if (pmdp_test_and_clear_young(vma, addr, pmd)) referenced = true; - *pmd = pmd_mkold(*pmd); - } #ifdef CONFIG_MMU_NOTIFIER - if (mmu_notifier_clear_young(mm, addr, addr + HPAGE_PMD_SIZE)) + if (mmu_notifier_clear_young(vma->vm_mm, addr, addr + HPAGE_PMD_SIZE)) referenced = true; #endif /* CONFIG_MMU_NOTIFIER */ diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h index 14f4bc69f29b..18d837d11bce 100644 --- a/mm/damon/ops-common.h +++ b/mm/damon/ops-common.h @@ -9,8 +9,8 @@ struct folio *damon_get_folio(unsigned long pfn); -void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr); -void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr); +void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr); +void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr); int damon_cold_score(struct damon_ctx *c, struct damon_region *r, struct damos *s); diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c index 467b99166b43..5b3a3463d078 100644 --- a/mm/damon/paddr.c +++ b/mm/damon/paddr.c @@ -24,9 +24,9 @@ static bool __damon_pa_mkold(struct folio *folio, struct vm_area_struct *vma, while (page_vma_mapped_walk(&pvmw)) { addr = pvmw.address; if (pvmw.pte) - damon_ptep_mkold(pvmw.pte, vma->vm_mm, addr); + damon_ptep_mkold(pvmw.pte, vma, addr); else - damon_pmdp_mkold(pvmw.pmd, vma->vm_mm, addr); + damon_pmdp_mkold(pvmw.pmd, vma, addr); } return true; } diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c index 1fec16d7263e..37994fb6120c 100644 --- a/mm/damon/vaddr.c +++ b/mm/damon/vaddr.c @@ -311,7 +311,7 @@ static int damon_mkold_pmd_entry(pmd_t *pmd, unsigned long addr, } if (pmd_trans_huge(*pmd)) { - damon_pmdp_mkold(pmd, walk->mm, addr); + damon_pmdp_mkold(pmd, walk->vma, addr); spin_unlock(ptl); return 0; } @@ -323,7 +323,7 @@ static int damon_mkold_pmd_entry(pmd_t *pmd, unsigned long addr, pte = pte_offset_map_lock(walk->mm, pmd, addr, &ptl); if (!pte_present(*pte)) goto out; - damon_ptep_mkold(pte, walk->mm, addr); + damon_ptep_mkold(pte, walk->vma, addr); out: pte_unmap_unlock(pte, ptl); return 0;
It is racy to non-atomically read a pte, then clear the young bit, then write it back as this could discard dirty information. Further, it is bad practice to directly set a pte entry within a table. Instead clearing young must go through the arch-provided helper, ptep_test_and_clear_young() to ensure it is modified atomically and to give the arch code visibility and allow it to check (and potentially modify) the operation. Fixes: 46c3a0accdc4 ("mm/damon/vaddr: separate commonly usable functions") Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> Reviewed-by: Zi Yan <ziy@nvidia.com> --- mm/damon/ops-common.c | 16 ++++++---------- mm/damon/ops-common.h | 4 ++-- mm/damon/paddr.c | 4 ++-- mm/damon/vaddr.c | 4 ++-- 4 files changed, 12 insertions(+), 16 deletions(-)