[01/31] mm: use pmdp_get_lockless() without surplus barrier()
Commit Message
Use pmdp_get_lockless() in preference to READ_ONCE(*pmdp), to get a more
reliable result with PAE (or READ_ONCE as before without PAE); and remove
the unnecessary extra barrier()s which got left behind in its callers.
HOWEVER: Note the small print in linux/pgtable.h, where it was designed
specifically for fast GUP, and depends on interrupts being disabled for
its full guarantee: most callers which have been added (here and before)
do NOT have interrupts disabled, so there is still some need for caution.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
fs/userfaultfd.c | 10 +---------
include/linux/pgtable.h | 17 -----------------
mm/gup.c | 6 +-----
mm/hmm.c | 2 +-
mm/khugepaged.c | 5 -----
mm/ksm.c | 3 +--
mm/memory.c | 14 ++------------
mm/mprotect.c | 5 -----
mm/page_vma_mapped.c | 2 +-
9 files changed, 7 insertions(+), 57 deletions(-)
Comments
On Sun, May 21, 2023 at 09:49:45PM -0700, Hugh Dickins wrote:
> Use pmdp_get_lockless() in preference to READ_ONCE(*pmdp), to get a more
> reliable result with PAE (or READ_ONCE as before without PAE); and remove
> the unnecessary extra barrier()s which got left behind in its callers.
Pure question: does it mean that some of below path (missing barrier()
ones) could have problem when CONFIG_PAE, hence this can be seen as a
(potential) bug fix?
Thanks,
>
> HOWEVER: Note the small print in linux/pgtable.h, where it was designed
> specifically for fast GUP, and depends on interrupts being disabled for
> its full guarantee: most callers which have been added (here and before)
> do NOT have interrupts disabled, so there is still some need for caution.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> fs/userfaultfd.c | 10 +---------
> include/linux/pgtable.h | 17 -----------------
> mm/gup.c | 6 +-----
> mm/hmm.c | 2 +-
> mm/khugepaged.c | 5 -----
> mm/ksm.c | 3 +--
> mm/memory.c | 14 ++------------
> mm/mprotect.c | 5 -----
> mm/page_vma_mapped.c | 2 +-
> 9 files changed, 7 insertions(+), 57 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 0fd96d6e39ce..f7a0817b1ec0 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -349,15 +349,7 @@ static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx,
> if (!pud_present(*pud))
> goto out;
> pmd = pmd_offset(pud, address);
> - /*
> - * READ_ONCE must function as a barrier with narrower scope
> - * and it must be equivalent to:
> - * _pmd = *pmd; barrier();
> - *
> - * This is to deal with the instability (as in
> - * pmd_trans_unstable) of the pmd.
> - */
> - _pmd = READ_ONCE(*pmd);
> + _pmd = pmdp_get_lockless(pmd);
> if (pmd_none(_pmd))
> goto out;
>
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index c5a51481bbb9..8ec27fe69dc8 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1344,23 +1344,6 @@ static inline int pud_trans_unstable(pud_t *pud)
> static inline int pmd_none_or_trans_huge_or_clear_bad(pmd_t *pmd)
> {
> pmd_t pmdval = pmdp_get_lockless(pmd);
> - /*
> - * The barrier will stabilize the pmdval in a register or on
> - * the stack so that it will stop changing under the code.
> - *
> - * When CONFIG_TRANSPARENT_HUGEPAGE=y on x86 32bit PAE,
> - * pmdp_get_lockless is allowed to return a not atomic pmdval
> - * (for example pointing to an hugepage that has never been
> - * mapped in the pmd). The below checks will only care about
> - * the low part of the pmd with 32bit PAE x86 anyway, with the
> - * exception of pmd_none(). So the important thing is that if
> - * the low part of the pmd is found null, the high part will
> - * be also null or the pmd_none() check below would be
> - * confused.
> - */
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - barrier();
> -#endif
> /*
> * !pmd_present() checks for pmd migration entries
> *
> diff --git a/mm/gup.c b/mm/gup.c
> index bbe416236593..3bd5d3854c51 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -653,11 +653,7 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
> struct mm_struct *mm = vma->vm_mm;
>
> pmd = pmd_offset(pudp, address);
> - /*
> - * The READ_ONCE() will stabilize the pmdval in a register or
> - * on the stack so that it will stop changing under the code.
> - */
> - pmdval = READ_ONCE(*pmd);
> + pmdval = pmdp_get_lockless(pmd);
> if (pmd_none(pmdval))
> return no_page_table(vma, flags);
> if (!pmd_present(pmdval))
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 6a151c09de5e..e23043345615 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -332,7 +332,7 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
> pmd_t pmd;
>
> again:
> - pmd = READ_ONCE(*pmdp);
> + pmd = pmdp_get_lockless(pmdp);
> if (pmd_none(pmd))
> return hmm_vma_walk_hole(start, end, -1, walk);
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 6b9d39d65b73..732f9ac393fc 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -961,11 +961,6 @@ static int find_pmd_or_thp_or_none(struct mm_struct *mm,
> return SCAN_PMD_NULL;
>
> pmde = pmdp_get_lockless(*pmd);
> -
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - /* See comments in pmd_none_or_trans_huge_or_clear_bad() */
> - barrier();
> -#endif
> if (pmd_none(pmde))
> return SCAN_PMD_NONE;
> if (!pmd_present(pmde))
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 0156bded3a66..df2aa281d49d 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1194,8 +1194,7 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
> * without holding anon_vma lock for write. So when looking for a
> * genuine pmde (in which to find pte), test present and !THP together.
> */
> - pmde = *pmd;
> - barrier();
> + pmde = pmdp_get_lockless(pmd);
> if (!pmd_present(pmde) || pmd_trans_huge(pmde))
> goto out;
>
> diff --git a/mm/memory.c b/mm/memory.c
> index f69fbc251198..2eb54c0d5d3c 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4925,18 +4925,9 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
> * So now it's safe to run pte_offset_map().
> */
> vmf->pte = pte_offset_map(vmf->pmd, vmf->address);
> - vmf->orig_pte = *vmf->pte;
> + vmf->orig_pte = ptep_get_lockless(vmf->pte);
> vmf->flags |= FAULT_FLAG_ORIG_PTE_VALID;
>
> - /*
> - * some architectures can have larger ptes than wordsize,
> - * e.g.ppc44x-defconfig has CONFIG_PTE_64BIT=y and
> - * CONFIG_32BIT=y, so READ_ONCE cannot guarantee atomic
> - * accesses. The code below just needs a consistent view
> - * for the ifs and we later double check anyway with the
> - * ptl lock held. So here a barrier will do.
> - */
> - barrier();
> if (pte_none(vmf->orig_pte)) {
> pte_unmap(vmf->pte);
> vmf->pte = NULL;
> @@ -5060,9 +5051,8 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
> if (!(ret & VM_FAULT_FALLBACK))
> return ret;
> } else {
> - vmf.orig_pmd = *vmf.pmd;
> + vmf.orig_pmd = pmdp_get_lockless(vmf.pmd);
>
> - barrier();
> if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
> VM_BUG_ON(thp_migration_supported() &&
> !is_pmd_migration_entry(vmf.orig_pmd));
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 92d3d3ca390a..c5a13c0f1017 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -309,11 +309,6 @@ static inline int pmd_none_or_clear_bad_unless_trans_huge(pmd_t *pmd)
> {
> pmd_t pmdval = pmdp_get_lockless(pmd);
>
> - /* See pmd_none_or_trans_huge_or_clear_bad for info on barrier */
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - barrier();
> -#endif
> -
> if (pmd_none(pmdval))
> return 1;
> if (pmd_trans_huge(pmdval))
> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> index 4e448cfbc6ef..64aff6718bdb 100644
> --- a/mm/page_vma_mapped.c
> +++ b/mm/page_vma_mapped.c
> @@ -210,7 +210,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
> * compiler and used as a stale value after we've observed a
> * subsequent update.
> */
> - pmde = READ_ONCE(*pvmw->pmd);
> + pmde = pmdp_get_lockless(pvmw->pmd);
>
> if (pmd_trans_huge(pmde) || is_pmd_migration_entry(pmde) ||
> (pmd_present(pmde) && pmd_devmap(pmde))) {
> --
> 2.35.3
>
On Sun, May 21, 2023 at 10:49 PM Hugh Dickins <hughd@google.com> wrote:
>
> Use pmdp_get_lockless() in preference to READ_ONCE(*pmdp), to get a more
> reliable result with PAE (or READ_ONCE as before without PAE); and remove
> the unnecessary extra barrier()s which got left behind in its callers.
>
> HOWEVER: Note the small print in linux/pgtable.h, where it was designed
> specifically for fast GUP, and depends on interrupts being disabled for
> its full guarantee: most callers which have been added (here and before)
> do NOT have interrupts disabled, so there is still some need for caution.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
Acked-by: Yu Zhao <yuzhao@google.com>
The previous ask here:
https://lore.kernel.org/r/CAOUHufZo=fB2HcaCrj2aidLJ2zEhOpi7ou5M_7qOQiuQq8+wTQ@mail.gmail.com/
On Wed, 24 May 2023, Peter Xu wrote:
> On Sun, May 21, 2023 at 09:49:45PM -0700, Hugh Dickins wrote:
> > Use pmdp_get_lockless() in preference to READ_ONCE(*pmdp), to get a more
> > reliable result with PAE (or READ_ONCE as before without PAE); and remove
> > the unnecessary extra barrier()s which got left behind in its callers.
>
> Pure question: does it mean that some of below path (missing barrier()
> ones) could have problem when CONFIG_PAE, hence this can be seen as a
> (potential) bug fix?
I don't think so; or at least, I am not claiming that this fixes any.
It really depends on what use is made of the pmdval afterwards, and
I've not checked through them. The READ_ONCE()s which were there,
were good enough to make sure that the compiler did not reevaluate
the pmdval later on, with perhaps a confusingly different result.
But, at least in the x86 PAE case, they were not good enough to ensure
that the two halves of the entry match up; and, sad to say, nor is that
absolutely guaranteed by these conversions to pmdp_get_lockless() -
because of the "HOWEVER" below. PeterZ's comments in linux/pgtable.h
are well worth reading through.
You might question why I made these changes at all: some days
I question them too. Better though imperfect? Or deceptive?
Hugh
> >
> > HOWEVER: Note the small print in linux/pgtable.h, where it was designed
> > specifically for fast GUP, and depends on interrupts being disabled for
> > its full guarantee: most callers which have been added (here and before)
> > do NOT have interrupts disabled, so there is still some need for caution.
On Thu, May 25, 2023 at 03:35:01PM -0700, Hugh Dickins wrote:
> On Wed, 24 May 2023, Peter Xu wrote:
> > On Sun, May 21, 2023 at 09:49:45PM -0700, Hugh Dickins wrote:
> > > Use pmdp_get_lockless() in preference to READ_ONCE(*pmdp), to get a more
> > > reliable result with PAE (or READ_ONCE as before without PAE); and remove
> > > the unnecessary extra barrier()s which got left behind in its callers.
> >
> > Pure question: does it mean that some of below path (missing barrier()
> > ones) could have problem when CONFIG_PAE, hence this can be seen as a
> > (potential) bug fix?
>
> I don't think so; or at least, I am not claiming that this fixes any.
>
> It really depends on what use is made of the pmdval afterwards, and
> I've not checked through them. The READ_ONCE()s which were there,
> were good enough to make sure that the compiler did not reevaluate
> the pmdval later on, with perhaps a confusingly different result.
>
> But, at least in the x86 PAE case, they were not good enough to ensure
> that the two halves of the entry match up; and, sad to say, nor is that
> absolutely guaranteed by these conversions to pmdp_get_lockless() -
> because of the "HOWEVER" below. PeterZ's comments in linux/pgtable.h
> are well worth reading through.
Yes exactly - that's one major thing of my confusion on using
{ptep|pmdp}_get_lockless().
In irqoff ctx, AFAICT we can see a totally messed up pte/pmd with present
bit set if extremely unlucky. E.g. it can race with something like
"DONTNEED (contains tlbflush) then a POPULATE_WRITE" so we can have
"present -> present" conversion of pte when reading, so we can read half
pfn1 and then the other half pfn2.
The other confusing thing on this _lockless trick on PAE is, I think it
_might_ go wrong with devmap..
The problem is here we assumed even if high & low may not match, we still
can rely on most pte/pmd checks are done only on low bits (except _none()
check) to guarantee at least the checks are still atomic on low bits.
But it seems to me it's not true anymore if with pmd_trans_huge() after
devmap introduced, e.g.:
static inline int pmd_trans_huge(pmd_t pmd)
{
return (pmd_val(pmd) & (_PAGE_PSE|_PAGE_DEVMAP)) == _PAGE_PSE;
}
#define _PAGE_PSE (_AT(pteval_t, 1) << _PAGE_BIT_PSE)
#define _PAGE_BIT_PSE 7 /* 4 MB (or 2MB) page */
#define _PAGE_DEVMAP (_AT(u64, 1) << _PAGE_BIT_DEVMAP)
#define _PAGE_BIT_DEVMAP _PAGE_BIT_SOFTW4
#define _PAGE_BIT_SOFTW4 58 /* available for programmer */
So after devmap with CONFIG_PAE, pmd_trans_huge() checks more than low bits
but also high bits. I didn't go further to check whether there can be any
real issue but IIUC that's not expected when the low/high trick introduced
(originally introduced in commit e585513b76f7b05d sololy for x86 PAE
fast-gup only).
>
> You might question why I made these changes at all: some days
> I question them too. Better though imperfect? Or deceptive?
I think it's probably a separate topic to address in all cases, so I think
this patch still make it slightly better on barrier() which I agree:
Acked-by: Peter Xu <peterx@redhat.com>
Thanks,
On Fri, 26 May 2023, Peter Xu wrote:
>
> The other confusing thing on this _lockless trick on PAE is, I think it
> _might_ go wrong with devmap..
>
> The problem is here we assumed even if high & low may not match, we still
> can rely on most pte/pmd checks are done only on low bits (except _none()
> check) to guarantee at least the checks are still atomic on low bits.
>
> But it seems to me it's not true anymore if with pmd_trans_huge() after
> devmap introduced, e.g.:
I agree that there would likely be a problem for p??_devmap() on 32-bit
PAE: but (I hope I followed the chain correctly!) I had earlier found
that pmd_devmap() can only return true when CONFIG_ZONE_DEVICE=y, and
config ZONE_DEVICE depends on ARCH_HAS_PTE_DEVMAP, and ARCH_HAS_PTE_DEVMAP
is only selected (in some cases) by arm64, powerpc if PPC_BOOK3S_64, and
x86 if X86_64.
So I stopped worrying about devmap.
Hugh
@@ -349,15 +349,7 @@ static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx,
if (!pud_present(*pud))
goto out;
pmd = pmd_offset(pud, address);
- /*
- * READ_ONCE must function as a barrier with narrower scope
- * and it must be equivalent to:
- * _pmd = *pmd; barrier();
- *
- * This is to deal with the instability (as in
- * pmd_trans_unstable) of the pmd.
- */
- _pmd = READ_ONCE(*pmd);
+ _pmd = pmdp_get_lockless(pmd);
if (pmd_none(_pmd))
goto out;
@@ -1344,23 +1344,6 @@ static inline int pud_trans_unstable(pud_t *pud)
static inline int pmd_none_or_trans_huge_or_clear_bad(pmd_t *pmd)
{
pmd_t pmdval = pmdp_get_lockless(pmd);
- /*
- * The barrier will stabilize the pmdval in a register or on
- * the stack so that it will stop changing under the code.
- *
- * When CONFIG_TRANSPARENT_HUGEPAGE=y on x86 32bit PAE,
- * pmdp_get_lockless is allowed to return a not atomic pmdval
- * (for example pointing to an hugepage that has never been
- * mapped in the pmd). The below checks will only care about
- * the low part of the pmd with 32bit PAE x86 anyway, with the
- * exception of pmd_none(). So the important thing is that if
- * the low part of the pmd is found null, the high part will
- * be also null or the pmd_none() check below would be
- * confused.
- */
-#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- barrier();
-#endif
/*
* !pmd_present() checks for pmd migration entries
*
@@ -653,11 +653,7 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
struct mm_struct *mm = vma->vm_mm;
pmd = pmd_offset(pudp, address);
- /*
- * The READ_ONCE() will stabilize the pmdval in a register or
- * on the stack so that it will stop changing under the code.
- */
- pmdval = READ_ONCE(*pmd);
+ pmdval = pmdp_get_lockless(pmd);
if (pmd_none(pmdval))
return no_page_table(vma, flags);
if (!pmd_present(pmdval))
@@ -332,7 +332,7 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
pmd_t pmd;
again:
- pmd = READ_ONCE(*pmdp);
+ pmd = pmdp_get_lockless(pmdp);
if (pmd_none(pmd))
return hmm_vma_walk_hole(start, end, -1, walk);
@@ -961,11 +961,6 @@ static int find_pmd_or_thp_or_none(struct mm_struct *mm,
return SCAN_PMD_NULL;
pmde = pmdp_get_lockless(*pmd);
-
-#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- /* See comments in pmd_none_or_trans_huge_or_clear_bad() */
- barrier();
-#endif
if (pmd_none(pmde))
return SCAN_PMD_NONE;
if (!pmd_present(pmde))
@@ -1194,8 +1194,7 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
* without holding anon_vma lock for write. So when looking for a
* genuine pmde (in which to find pte), test present and !THP together.
*/
- pmde = *pmd;
- barrier();
+ pmde = pmdp_get_lockless(pmd);
if (!pmd_present(pmde) || pmd_trans_huge(pmde))
goto out;
@@ -4925,18 +4925,9 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
* So now it's safe to run pte_offset_map().
*/
vmf->pte = pte_offset_map(vmf->pmd, vmf->address);
- vmf->orig_pte = *vmf->pte;
+ vmf->orig_pte = ptep_get_lockless(vmf->pte);
vmf->flags |= FAULT_FLAG_ORIG_PTE_VALID;
- /*
- * some architectures can have larger ptes than wordsize,
- * e.g.ppc44x-defconfig has CONFIG_PTE_64BIT=y and
- * CONFIG_32BIT=y, so READ_ONCE cannot guarantee atomic
- * accesses. The code below just needs a consistent view
- * for the ifs and we later double check anyway with the
- * ptl lock held. So here a barrier will do.
- */
- barrier();
if (pte_none(vmf->orig_pte)) {
pte_unmap(vmf->pte);
vmf->pte = NULL;
@@ -5060,9 +5051,8 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
if (!(ret & VM_FAULT_FALLBACK))
return ret;
} else {
- vmf.orig_pmd = *vmf.pmd;
+ vmf.orig_pmd = pmdp_get_lockless(vmf.pmd);
- barrier();
if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
VM_BUG_ON(thp_migration_supported() &&
!is_pmd_migration_entry(vmf.orig_pmd));
@@ -309,11 +309,6 @@ static inline int pmd_none_or_clear_bad_unless_trans_huge(pmd_t *pmd)
{
pmd_t pmdval = pmdp_get_lockless(pmd);
- /* See pmd_none_or_trans_huge_or_clear_bad for info on barrier */
-#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- barrier();
-#endif
-
if (pmd_none(pmdval))
return 1;
if (pmd_trans_huge(pmdval))
@@ -210,7 +210,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
* compiler and used as a stale value after we've observed a
* subsequent update.
*/
- pmde = READ_ONCE(*pvmw->pmd);
+ pmde = pmdp_get_lockless(pvmw->pmd);
if (pmd_trans_huge(pmde) || is_pmd_migration_entry(pmde) ||
(pmd_present(pmde) && pmd_devmap(pmde))) {