| Message ID | 20240215121756.2734131-3-ryan.roberts@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Reduce cost of ptep_get_lockless on arm64 | expand |
On 15.02.24 13:17, Ryan Roberts wrote: > Gup needs to read ptes locklessly, so it uses ptep_get_lockless(). > However, the returned access and dirty bits are unimportant so let's > switch over to ptep_get_lockless_norecency(). > > The wrinkle is that gup needs to check that the pte hasn't changed once > it has pinned the folio following this model: > > pte = ptep_get_lockless_norecency(ptep) > ... > if (!pte_same(pte, ptep_get_lockless(ptep))) > // RACE! > ... > > And now that pte may not contain correct access and dirty information, > the pte_same() comparison could spuriously fail. So let's introduce a > new pte_same_norecency() helper which will ignore the access and dirty > bits when doing the comparison. > > Note that previously, ptep_get() was being used for the comparison; this > is technically incorrect because the PTL is not held. I've also > converted the comparison to use the preferred pmd_same() helper instead > of doing a raw value comparison. > > As a side-effect, this new approach removes the possibility of > concurrent read/write to the page causing a spurious fast gup failure, > because the access and dirty bits are no longer used in the comparison. > > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > --- [...] > #ifndef __HAVE_ARCH_PTE_UNUSED > /* > * Some architectures provide facilities to virtualization guests > diff --git a/mm/gup.c b/mm/gup.c > index df83182ec72d..0f96d0a5ec09 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2576,7 +2576,7 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr, > if (!ptep) > return 0; > do { > - pte_t pte = ptep_get_lockless(ptep); > + pte_t pte = ptep_get_lockless_norecency(ptep); > struct page *page; > struct folio *folio; > > @@ -2617,8 +2617,9 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr, > goto pte_unmap; > } > > - if (unlikely(pmd_val(pmd) != pmd_val(*pmdp)) || > - unlikely(pte_val(pte) != pte_val(ptep_get(ptep)))) { > + if (unlikely(!pmd_same(pmd, *pmdp)) || > + unlikely(!pte_same_norecency(pte, > + ptep_get_lockless_norecency(ptep)))) { > gup_put_folio(folio, 1, flags); > goto pte_unmap; We pass the pte into pte_access_permitted(). It would be good to mention that you checked all implementations. Acked-by: David Hildenbrand <david@redhat.com>
On 26/03/2024 16:30, David Hildenbrand wrote: > On 15.02.24 13:17, Ryan Roberts wrote: >> Gup needs to read ptes locklessly, so it uses ptep_get_lockless(). >> However, the returned access and dirty bits are unimportant so let's >> switch over to ptep_get_lockless_norecency(). >> >> The wrinkle is that gup needs to check that the pte hasn't changed once >> it has pinned the folio following this model: >> >> pte = ptep_get_lockless_norecency(ptep) >> ... >> if (!pte_same(pte, ptep_get_lockless(ptep))) >> // RACE! >> ... >> >> And now that pte may not contain correct access and dirty information, >> the pte_same() comparison could spuriously fail. So let's introduce a >> new pte_same_norecency() helper which will ignore the access and dirty >> bits when doing the comparison. >> >> Note that previously, ptep_get() was being used for the comparison; this >> is technically incorrect because the PTL is not held. I've also >> converted the comparison to use the preferred pmd_same() helper instead >> of doing a raw value comparison. >> >> As a side-effect, this new approach removes the possibility of >> concurrent read/write to the page causing a spurious fast gup failure, >> because the access and dirty bits are no longer used in the comparison. >> >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >> --- > > [...] > >> #ifndef __HAVE_ARCH_PTE_UNUSED >> /* >> * Some architectures provide facilities to virtualization guests >> diff --git a/mm/gup.c b/mm/gup.c >> index df83182ec72d..0f96d0a5ec09 100644 >> --- a/mm/gup.c >> +++ b/mm/gup.c >> @@ -2576,7 +2576,7 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, >> unsigned long addr, >> if (!ptep) >> return 0; >> do { >> - pte_t pte = ptep_get_lockless(ptep); >> + pte_t pte = ptep_get_lockless_norecency(ptep); >> struct page *page; >> struct folio *folio; >> >> @@ -2617,8 +2617,9 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, >> unsigned long addr, >> goto pte_unmap; >> } >> >> - if (unlikely(pmd_val(pmd) != pmd_val(*pmdp)) || >> - unlikely(pte_val(pte) != pte_val(ptep_get(ptep)))) { >> + if (unlikely(!pmd_same(pmd, *pmdp)) || >> + unlikely(!pte_same_norecency(pte, >> + ptep_get_lockless_norecency(ptep)))) { >> gup_put_folio(folio, 1, flags); >> goto pte_unmap; > > We pass the pte into pte_access_permitted(). It would be good to mention that > you checked all implementations. TBH, I hadn't; I decided that since the "inaccurate access/dirty bits" was only possible on arm64, then only arm64's implementation needed checking. But given your comment, I just had a quick look at all impls. I didn't spot any problems where any impl needs the access/dirty bits. I'll add this to the commit log. > > Acked-by: David Hildenbrand <david@redhat.com> >
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h index 9dd40fdbd825..8123affa8baf 100644 --- a/include/linux/pgtable.h +++ b/include/linux/pgtable.h @@ -936,6 +936,24 @@ static inline int pte_same(pte_t pte_a, pte_t pte_b) } #endif +/** + * pte_same_norecency - Compare pte_a and pte_b, ignoring young and dirty bits, + * if the ptes are present. + * + * @pte_a: First pte to compare. + * @pte_b: Second pte to compare. + * + * Returns 1 if the ptes match, else 0. + */ +static inline int pte_same_norecency(pte_t pte_a, pte_t pte_b) +{ + if (pte_present(pte_a)) + pte_a = pte_mkold(pte_mkclean(pte_a)); + if (pte_present(pte_b)) + pte_b = pte_mkold(pte_mkclean(pte_b)); + return pte_same(pte_a, pte_b); +} + #ifndef __HAVE_ARCH_PTE_UNUSED /* * Some architectures provide facilities to virtualization guests diff --git a/mm/gup.c b/mm/gup.c index df83182ec72d..0f96d0a5ec09 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -2576,7 +2576,7 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr, if (!ptep) return 0; do { - pte_t pte = ptep_get_lockless(ptep); + pte_t pte = ptep_get_lockless_norecency(ptep); struct page *page; struct folio *folio; @@ -2617,8 +2617,9 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr, goto pte_unmap; } - if (unlikely(pmd_val(pmd) != pmd_val(*pmdp)) || - unlikely(pte_val(pte) != pte_val(ptep_get(ptep)))) { + if (unlikely(!pmd_same(pmd, *pmdp)) || + unlikely(!pte_same_norecency(pte, + ptep_get_lockless_norecency(ptep)))) { gup_put_folio(folio, 1, flags); goto pte_unmap; }
Gup needs to read ptes locklessly, so it uses ptep_get_lockless(). However, the returned access and dirty bits are unimportant so let's switch over to ptep_get_lockless_norecency(). The wrinkle is that gup needs to check that the pte hasn't changed once it has pinned the folio following this model: pte = ptep_get_lockless_norecency(ptep) ... if (!pte_same(pte, ptep_get_lockless(ptep))) // RACE! ... And now that pte may not contain correct access and dirty information, the pte_same() comparison could spuriously fail. So let's introduce a new pte_same_norecency() helper which will ignore the access and dirty bits when doing the comparison. Note that previously, ptep_get() was being used for the comparison; this is technically incorrect because the PTL is not held. I've also converted the comparison to use the preferred pmd_same() helper instead of doing a raw value comparison. As a side-effect, this new approach removes the possibility of concurrent read/write to the page causing a spurious fast gup failure, because the access and dirty bits are no longer used in the comparison. Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> --- include/linux/pgtable.h | 18 ++++++++++++++++++ mm/gup.c | 7 ++++--- 2 files changed, 22 insertions(+), 3 deletions(-) -- 2.25.1