[01/31] mm: use pmdp_get_lockless() without surplus barrier()

  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(-)
  

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