Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1666778 > unrolled thread

[PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-06-15 17:00 +0200
    Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Andrea Arcangeli <aarcange@redhat.com> - 2017-06-16 15:40 +0200
      Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-19 14:50 +0200
    Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-06-19 07:50 +0200
      Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-19 14:50 +0200
        Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-06-19 15:10 +0200
    Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Catalin Marinas <catalin.marinas@arm.com> - 2017-06-19 17:30 +0200
      Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-19 18:10 +0200
        Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Catalin Marinas <catalin.marinas@arm.com> - 2017-06-19 19:10 +0200
          Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-20 00:00 +0200
            Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Catalin Marinas <catalin.marinas@arm.com> - 2017-06-20 18:00 +0200
              Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-21 12:00 +0200
                Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Catalin Marinas <catalin.marinas@arm.com> - 2017-06-21 12:50 +0200
                Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Catalin Marinas <catalin.marinas@arm.com> - 2017-06-21 13:30 +0200
                  Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-21 14:10 +0200
                  Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2017-06-21 18:30 +0200
                    Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-21 19:20 +0200
                      Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2017-06-21 19:30 +0200
                        Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-21 20:00 +0200
    Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper Nadav Amit <nadav.amit@gmail.com> - 2017-06-19 19:20 +0200
      Re: [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-20 00:00 +0200

Page 1 of 2  [1] 2  Next page →

#1666778 — [PATCHv2 1/3] x86/mm: Provide pmdp_establish() helper

We need an atomic way to setup pmd page table entry, avoiding races with
CPU setting dirty/accessed bits. This is required to implement
pmdp_invalidate() that doesn't loose these bits.

On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
setting it up half-by-half can expose broken corrupted entry to CPU.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/pgtable-3level.h | 18 ++++++++++++++++++
 arch/x86/include/asm/pgtable.h        | 14 ++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/arch/x86/include/asm/pgtable-3level.h b/arch/x86/include/asm/pgtable-3level.h
index 50d35e3185f5..471c8a851363 100644
--- a/arch/x86/include/asm/pgtable-3level.h
+++ b/arch/x86/include/asm/pgtable-3level.h
@@ -180,6 +180,24 @@ static inline pmd_t native_pmdp_get_and_clear(pmd_t *pmdp)
 #define native_pmdp_get_and_clear(xp) native_local_pmdp_get_and_clear(xp)
 #endif
 
+#ifndef pmdp_establish
+#define pmdp_establish pmdp_establish
+static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
+{
+	pmd_t old;
+
+	/*
+	 * We cannot assume what is value of pmd here, so there's no easy way
+	 * to set if half by half. We have to fall back to cmpxchg64.
+	 */
+	{
+		old = *pmdp;
+	} while (cmpxchg64(&pmdp->pmd, old.pmd, pmd.pmd) != old.pmd);
+
+	return old;
+}
+#endif
+
 #ifdef CONFIG_SMP
 union split_pud {
 	struct {
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index f5af95a0c6b8..a924fc6a96b9 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -1092,6 +1092,20 @@ static inline void pmdp_set_wrprotect(struct mm_struct *mm,
 	clear_bit(_PAGE_BIT_RW, (unsigned long *)pmdp);
 }
 
+#ifndef pmdp_establish
+#define pmdp_establish pmdp_establish
+static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
+{
+	if (IS_ENABLED(CONFIG_SMP)) {
+		return xchg(pmdp, pmd);
+	} else {
+		pmd_t old = *pmdp;
+		*pmdp = pmd;
+		return old;
+	}
+}
+#endif
+
 /*
  * clone_pgd_range(pgd_t *dst, pgd_t *src, int count);
  *
-- 
2.11.0

[toc] | [next] | [standalone]

#1667753

Hello Krill,

On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> +static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
> +{
> +	pmd_t old;
> +
> +	/*
> +	 * We cannot assume what is value of pmd here, so there's no easy way
> +	 * to set if half by half. We have to fall back to cmpxchg64.
> +	 */
> +	{
> +		old = *pmdp;
> +	} while (cmpxchg64(&pmdp->pmd, old.pmd, pmd.pmd) != old.pmd);
> +
> +	return old;
> +}

I see further margin for optimization here (although it's only for PAE
x32..).

pmd is stable so we could do:

if (!(pmd & _PAGE_PRESENT)) {
   cast to split_pmd and use xchg on pmd_low like
   native_pmdp_get_and_clear and copy pmd_high non atomically
} else {
  the above cmpxchg64 loop
}

Now thinking about the above I had a second thought if pmdp_establish
is the right interface and if we shouldn't replace pmdp_establish with
pmdp_mknotpresent instead to skip the pmd & _PAGE_PRESENT check that
will always be true in practice, so pmdp_mknotpresent will call
internally pmd_mknotpresent and it won't have to check for pmd &
_PAGE_PRESENT and it would have no cons on x86-64.

[toc] | [prev] | [next] | [standalone]

#1669041

On Fri, Jun 16, 2017 at 03:36:00PM +0200, Andrea Arcangeli wrote:
> Hello Krill,
> 
> On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> > +static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
> > +{
> > +	pmd_t old;
> > +
> > +	/*
> > +	 * We cannot assume what is value of pmd here, so there's no easy way
> > +	 * to set if half by half. We have to fall back to cmpxchg64.
> > +	 */
> > +	{
> > +		old = *pmdp;
> > +	} while (cmpxchg64(&pmdp->pmd, old.pmd, pmd.pmd) != old.pmd);
> > +
> > +	return old;
> > +}
> 
> I see further margin for optimization here (although it's only for PAE
> x32..).
> 
> pmd is stable so we could do:
> 
> if (!(pmd & _PAGE_PRESENT)) {
>    cast to split_pmd and use xchg on pmd_low like
>    native_pmdp_get_and_clear and copy pmd_high non atomically
> } else {
>   the above cmpxchg64 loop
> }
> 
> Now thinking about the above I had a second thought if pmdp_establish
> is the right interface and if we shouldn't replace pmdp_establish with
> pmdp_mknotpresent instead to skip the pmd & _PAGE_PRESENT check that
> will always be true in practice, so pmdp_mknotpresent will call
> internally pmd_mknotpresent and it won't have to check for pmd &
> _PAGE_PRESENT and it would have no cons on x86-64.

With your proposed optimization, compiler is in good position to eliminate
cmpxchg loop for trivial cases as we have in pmdp_invalidate() case.
It can see that pmd is always has the present bit cleared.

I'll keep more flexible interface for now. Will see if anybody would see
more problems with it.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1668797

On Thu, 15 Jun 2017 17:52:22 +0300
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> We need an atomic way to setup pmd page table entry, avoiding races with
> CPU setting dirty/accessed bits. This is required to implement
> pmdp_invalidate() that doesn't loose these bits.
> 
> On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
> setting it up half-by-half can expose broken corrupted entry to CPU.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/pgtable-3level.h | 18 ++++++++++++++++++
>  arch/x86/include/asm/pgtable.h        | 14 ++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index f5af95a0c6b8..a924fc6a96b9 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -1092,6 +1092,20 @@ static inline void pmdp_set_wrprotect(struct mm_struct *mm,
>  	clear_bit(_PAGE_BIT_RW, (unsigned long *)pmdp);
>  }
> 
> +#ifndef pmdp_establish
> +#define pmdp_establish pmdp_establish
> +static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
> +{
> +	if (IS_ENABLED(CONFIG_SMP)) {
> +		return xchg(pmdp, pmd);
> +	} else {
> +		pmd_t old = *pmdp;
> +		*pmdp = pmd;
> +		return old;
> +	}
> +}
> +#endif
> +
>  /*
>   * clone_pgd_range(pgd_t *dst, pgd_t *src, int count);
>   *

For the s390 version of the pmdp_establish function we need the mm to be able
to do the TLB flush correctly. Can we please add a "struct vm_area_struct *vma"
argument to pmdp_establish analog to pmdp_invalidate?

The s390 patch would then look like this:
--
From 4d4641249d5e826c21c522d149553e89d73fcd4f Mon Sep 17 00:00:00 2001
From: Martin Schwidefsky <schwidefsky@de.ibm.com>
Date: Mon, 19 Jun 2017 07:40:11 +0200
Subject: [PATCH] s390/mm: add pmdp_establish

Define the pmdp_establish function to replace a pmd entry with a new
one and return the old value.

Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/include/asm/pgtable.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index bb59a0aa3249..dedeecd5455c 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1511,6 +1511,13 @@ static inline void pmdp_invalidate(struct vm_area_struct *vma,
 	pmdp_xchg_direct(vma->vm_mm, addr, pmdp, __pmd(_SEGMENT_ENTRY_EMPTY));
 }
 
+static inline pmd_t pmdp_establish(struct vm_area_struct *vma,
+				   pmd_t *pmdp, pmd_t pmd)
+{
+	return pmdp_xchg_direct(vma->vm_mm, addr, pmdp, pmd);
+}
+#define pmdp_establish pmdp_establish
+
 #define __HAVE_ARCH_PMDP_SET_WRPROTECT
 static inline void pmdp_set_wrprotect(struct mm_struct *mm,
 				      unsigned long addr, pmd_t *pmdp)
-- 
2.11.2


-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[toc] | [prev] | [next] | [standalone]

#1669039

On Mon, Jun 19, 2017 at 07:48:01AM +0200, Martin Schwidefsky wrote:
> On Thu, 15 Jun 2017 17:52:22 +0300
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > We need an atomic way to setup pmd page table entry, avoiding races with
> > CPU setting dirty/accessed bits. This is required to implement
> > pmdp_invalidate() that doesn't loose these bits.
> > 
> > On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
> > setting it up half-by-half can expose broken corrupted entry to CPU.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  arch/x86/include/asm/pgtable-3level.h | 18 ++++++++++++++++++
> >  arch/x86/include/asm/pgtable.h        | 14 ++++++++++++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> > index f5af95a0c6b8..a924fc6a96b9 100644
> > --- a/arch/x86/include/asm/pgtable.h
> > +++ b/arch/x86/include/asm/pgtable.h
> > @@ -1092,6 +1092,20 @@ static inline void pmdp_set_wrprotect(struct mm_struct *mm,
> >  	clear_bit(_PAGE_BIT_RW, (unsigned long *)pmdp);
> >  }
> > 
> > +#ifndef pmdp_establish
> > +#define pmdp_establish pmdp_establish
> > +static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
> > +{
> > +	if (IS_ENABLED(CONFIG_SMP)) {
> > +		return xchg(pmdp, pmd);
> > +	} else {
> > +		pmd_t old = *pmdp;
> > +		*pmdp = pmd;
> > +		return old;
> > +	}
> > +}
> > +#endif
> > +
> >  /*
> >   * clone_pgd_range(pgd_t *dst, pgd_t *src, int count);
> >   *
> 
> For the s390 version of the pmdp_establish function we need the mm to be able
> to do the TLB flush correctly. Can we please add a "struct vm_area_struct *vma"
> argument to pmdp_establish analog to pmdp_invalidate?
> 
> The s390 patch would then look like this:
> --
> From 4d4641249d5e826c21c522d149553e89d73fcd4f Mon Sep 17 00:00:00 2001
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Date: Mon, 19 Jun 2017 07:40:11 +0200
> Subject: [PATCH] s390/mm: add pmdp_establish
> 
> Define the pmdp_establish function to replace a pmd entry with a new
> one and return the old value.
> 
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
>  arch/s390/include/asm/pgtable.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> index bb59a0aa3249..dedeecd5455c 100644
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
> @@ -1511,6 +1511,13 @@ static inline void pmdp_invalidate(struct vm_area_struct *vma,
>  	pmdp_xchg_direct(vma->vm_mm, addr, pmdp, __pmd(_SEGMENT_ENTRY_EMPTY));
>  }
>  
> +static inline pmd_t pmdp_establish(struct vm_area_struct *vma,
> +				   pmd_t *pmdp, pmd_t pmd)
> +{
> +	return pmdp_xchg_direct(vma->vm_mm, addr, pmdp, pmd);

I guess, you need address too :-P.

I'll change prototype of pmdp_establish() and apply your patch.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1669050

On Mon, 19 Jun 2017 15:48:19 +0300
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Mon, Jun 19, 2017 at 07:48:01AM +0200, Martin Schwidefsky wrote:
> > On Thu, 15 Jun 2017 17:52:22 +0300
> > "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> >   
> > > We need an atomic way to setup pmd page table entry, avoiding races with
> > > CPU setting dirty/accessed bits. This is required to implement
> > > pmdp_invalidate() that doesn't loose these bits.
> > > 
> > > On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
> > > setting it up half-by-half can expose broken corrupted entry to CPU.
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > Cc: Ingo Molnar <mingo@kernel.org>
> > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > ---
> > >  arch/x86/include/asm/pgtable-3level.h | 18 ++++++++++++++++++
> > >  arch/x86/include/asm/pgtable.h        | 14 ++++++++++++++
> > >  2 files changed, 32 insertions(+)
> > > 
> > > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> > > index f5af95a0c6b8..a924fc6a96b9 100644
> > > --- a/arch/x86/include/asm/pgtable.h
> > > +++ b/arch/x86/include/asm/pgtable.h
> > > @@ -1092,6 +1092,20 @@ static inline void pmdp_set_wrprotect(struct mm_struct *mm,
> > >  	clear_bit(_PAGE_BIT_RW, (unsigned long *)pmdp);
> > >  }
> > > 
> > > +#ifndef pmdp_establish
> > > +#define pmdp_establish pmdp_establish
> > > +static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
> > > +{
> > > +	if (IS_ENABLED(CONFIG_SMP)) {
> > > +		return xchg(pmdp, pmd);
> > > +	} else {
> > > +		pmd_t old = *pmdp;
> > > +		*pmdp = pmd;
> > > +		return old;
> > > +	}
> > > +}
> > > +#endif
> > > +
> > >  /*
> > >   * clone_pgd_range(pgd_t *dst, pgd_t *src, int count);
> > >   *  
> > 
> > For the s390 version of the pmdp_establish function we need the mm to be able
> > to do the TLB flush correctly. Can we please add a "struct vm_area_struct *vma"
> > argument to pmdp_establish analog to pmdp_invalidate?
> > 
> > The s390 patch would then look like this:
> > --
> > From 4d4641249d5e826c21c522d149553e89d73fcd4f Mon Sep 17 00:00:00 2001
> > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > Date: Mon, 19 Jun 2017 07:40:11 +0200
> > Subject: [PATCH] s390/mm: add pmdp_establish
> > 
> > Define the pmdp_establish function to replace a pmd entry with a new
> > one and return the old value.
> > 
> > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > ---
> >  arch/s390/include/asm/pgtable.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> > index bb59a0aa3249..dedeecd5455c 100644
> > --- a/arch/s390/include/asm/pgtable.h
> > +++ b/arch/s390/include/asm/pgtable.h
> > @@ -1511,6 +1511,13 @@ static inline void pmdp_invalidate(struct vm_area_struct *vma,
> >  	pmdp_xchg_direct(vma->vm_mm, addr, pmdp, __pmd(_SEGMENT_ENTRY_EMPTY));
> >  }
> >  
> > +static inline pmd_t pmdp_establish(struct vm_area_struct *vma,
> > +				   pmd_t *pmdp, pmd_t pmd)
> > +{
> > +	return pmdp_xchg_direct(vma->vm_mm, addr, pmdp, pmd);  
> 
> I guess, you need address too :-P.
> 
> I'll change prototype of pmdp_establish() and apply your patch.
 
Ahh, yes. vma + addr please ;-)

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[toc] | [prev] | [next] | [standalone]

#1669241

Hi Kirill,

On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> We need an atomic way to setup pmd page table entry, avoiding races with
> CPU setting dirty/accessed bits. This is required to implement
> pmdp_invalidate() that doesn't loose these bits.
> 
> On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
> setting it up half-by-half can expose broken corrupted entry to CPU.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>

I'll look at this from the arm64 perspective. It would be good if we can
have a generic atomic implementation based on cmpxchg64 but I need to
look at the details first.

> +static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
> +{
> +	pmd_t old;
> +
> +	/*
> +	 * We cannot assume what is value of pmd here, so there's no easy way
> +	 * to set if half by half. We have to fall back to cmpxchg64.
> +	 */
> +	{

BTW, you are missing a "do" here (and it probably compiles just fine
without it, though different behaviour).

> +		old = *pmdp;
> +	} while (cmpxchg64(&pmdp->pmd, old.pmd, pmd.pmd) != old.pmd);
> +
> +	return old;
> +}

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1669395

On Mon, Jun 19, 2017 at 04:22:29PM +0100, Catalin Marinas wrote:
> Hi Kirill,
> 
> On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> > We need an atomic way to setup pmd page table entry, avoiding races with
> > CPU setting dirty/accessed bits. This is required to implement
> > pmdp_invalidate() that doesn't loose these bits.
> > 
> > On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
> > setting it up half-by-half can expose broken corrupted entry to CPU.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> 
> I'll look at this from the arm64 perspective. It would be good if we can
> have a generic atomic implementation based on cmpxchg64 but I need to
> look at the details first.

Unfortunately, I'm not sure it's possbile.

The format of a page table is defined per-arch. We cannot assume much about
it in generic code.

I guess we could make it compile by casting to 'unsigned long', but is it
useful?
Every architecture manintainer still has to validate that this assumption
is valid for the architecture.

> > +static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
> > +{
> > +	pmd_t old;
> > +
> > +	/*
> > +	 * We cannot assume what is value of pmd here, so there's no easy way
> > +	 * to set if half by half. We have to fall back to cmpxchg64.
> > +	 */
> > +	{
> 
> BTW, you are missing a "do" here (and it probably compiles just fine
> without it, though different behaviour).

Ouch. Thanks.

Hm, what is semantics of the construct without a "do"?

> 
> > +		old = *pmdp;
> > +	} while (cmpxchg64(&pmdp->pmd, old.pmd, pmd.pmd) != old.pmd);
> > +
> > +	return old;
> > +}
> 
> -- 
> Catalin
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1669498

On Mon, Jun 19, 2017 at 07:00:05PM +0300, Kirill A. Shutemov wrote:
> On Mon, Jun 19, 2017 at 04:22:29PM +0100, Catalin Marinas wrote:
> > On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> > > We need an atomic way to setup pmd page table entry, avoiding races with
> > > CPU setting dirty/accessed bits. This is required to implement
> > > pmdp_invalidate() that doesn't loose these bits.
> > > 
> > > On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
> > > setting it up half-by-half can expose broken corrupted entry to CPU.
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > Cc: Ingo Molnar <mingo@kernel.org>
> > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > 
> > I'll look at this from the arm64 perspective. It would be good if we can
> > have a generic atomic implementation based on cmpxchg64 but I need to
> > look at the details first.
> 
> Unfortunately, I'm not sure it's possbile.
> 
> The format of a page table is defined per-arch. We cannot assume much about
> it in generic code.
> 
> I guess we could make it compile by casting to 'unsigned long', but is it
> useful?
> Every architecture manintainer still has to validate that this assumption
> is valid for the architecture.

You are right, not much gained in doing this.

Maybe a stupid question but can we not implement pmdp_invalidate() with
something like pmdp_get_and_clear() (usually reusing the ptep_*
equivalent). Or pmdp_clear_flush() (again, reusing ptep_clear_flush())?

In my quick grep on pmdp_invalidate, it seems to be followed by
set_pmd_at() or pmd_populate() already and the *pmd value after
mknotpresent isn't any different from 0 to the hardware (at least on
ARM). That's unless Linux expects to see some non-zero value here if
walking the page tables on another CPU.

> > > +static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
> > > +{
> > > +	pmd_t old;
> > > +
> > > +	/*
> > > +	 * We cannot assume what is value of pmd here, so there's no easy way
> > > +	 * to set if half by half. We have to fall back to cmpxchg64.
> > > +	 */
> > > +	{
> > 
> > BTW, you are missing a "do" here (and it probably compiles just fine
> > without it, though different behaviour).
> 
> Ouch. Thanks.
> 
> Hm, what is semantics of the construct without a "do"?

You can just ignore the brackets:

	old = *pmdp;
	while (cmpxchg64(&pmdp->pmd, old.pmd, pmd.pmd) != old.pmd)
		;

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1669986

On Mon, Jun 19, 2017 at 06:09:12PM +0100, Catalin Marinas wrote:
> On Mon, Jun 19, 2017 at 07:00:05PM +0300, Kirill A. Shutemov wrote:
> > On Mon, Jun 19, 2017 at 04:22:29PM +0100, Catalin Marinas wrote:
> > > On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> > > > We need an atomic way to setup pmd page table entry, avoiding races with
> > > > CPU setting dirty/accessed bits. This is required to implement
> > > > pmdp_invalidate() that doesn't loose these bits.
> > > > 
> > > > On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
> > > > setting it up half-by-half can expose broken corrupted entry to CPU.
> > > > 
> > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > Cc: Ingo Molnar <mingo@kernel.org>
> > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > 
> > > I'll look at this from the arm64 perspective. It would be good if we can
> > > have a generic atomic implementation based on cmpxchg64 but I need to
> > > look at the details first.
> > 
> > Unfortunately, I'm not sure it's possbile.
> > 
> > The format of a page table is defined per-arch. We cannot assume much about
> > it in generic code.
> > 
> > I guess we could make it compile by casting to 'unsigned long', but is it
> > useful?
> > Every architecture manintainer still has to validate that this assumption
> > is valid for the architecture.
> 
> You are right, not much gained in doing this.
> 
> Maybe a stupid question but can we not implement pmdp_invalidate() with
> something like pmdp_get_and_clear() (usually reusing the ptep_*
> equivalent). Or pmdp_clear_flush() (again, reusing ptep_clear_flush())?
> 
> In my quick grep on pmdp_invalidate, it seems to be followed by
> set_pmd_at() or pmd_populate() already and the *pmd value after
> mknotpresent isn't any different from 0 to the hardware (at least on
> ARM). That's unless Linux expects to see some non-zero value here if
> walking the page tables on another CPU.

The whole reason to have pmdp_invalidate() in first place is to never make
pmd clear in the middle. Otherwise we will get race with MADV_DONTNEED.
See ced108037c2a for an example of such race.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1670901

On Tue, Jun 20, 2017 at 12:52:10AM +0300, Kirill A. Shutemov wrote:
> On Mon, Jun 19, 2017 at 06:09:12PM +0100, Catalin Marinas wrote:
> > On Mon, Jun 19, 2017 at 07:00:05PM +0300, Kirill A. Shutemov wrote:
> > > On Mon, Jun 19, 2017 at 04:22:29PM +0100, Catalin Marinas wrote:
> > > > On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> > > > > We need an atomic way to setup pmd page table entry, avoiding races with
> > > > > CPU setting dirty/accessed bits. This is required to implement
> > > > > pmdp_invalidate() that doesn't loose these bits.
> > > > > 
> > > > > On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
> > > > > setting it up half-by-half can expose broken corrupted entry to CPU.
> > > > > 
> > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > > Cc: Ingo Molnar <mingo@kernel.org>
> > > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > 
> > > > I'll look at this from the arm64 perspective. It would be good if we can
> > > > have a generic atomic implementation based on cmpxchg64 but I need to
> > > > look at the details first.
> > > 
> > > Unfortunately, I'm not sure it's possbile.
> > > 
> > > The format of a page table is defined per-arch. We cannot assume much about
> > > it in generic code.
> > > 
> > > I guess we could make it compile by casting to 'unsigned long', but is it
> > > useful?
> > > Every architecture manintainer still has to validate that this assumption
> > > is valid for the architecture.
> > 
> > You are right, not much gained in doing this.
> > 
> > Maybe a stupid question but can we not implement pmdp_invalidate() with
> > something like pmdp_get_and_clear() (usually reusing the ptep_*
> > equivalent). Or pmdp_clear_flush() (again, reusing ptep_clear_flush())?
> > 
> > In my quick grep on pmdp_invalidate, it seems to be followed by
> > set_pmd_at() or pmd_populate() already and the *pmd value after
> > mknotpresent isn't any different from 0 to the hardware (at least on
> > ARM). That's unless Linux expects to see some non-zero value here if
> > walking the page tables on another CPU.
> 
> The whole reason to have pmdp_invalidate() in first place is to never make
> pmd clear in the middle. Otherwise we will get race with MADV_DONTNEED.
> See ced108037c2a for an example of such race.

Thanks for the explanation. So you basically just want to set a !present
and !none pmd. I noticed that with your proposed pmdp_invalidate(),
pmdp_establish(pmd_mknotpresent(*pmdp)) could set a stale *pmdp (with
the present bit cleared) temporarily until updated with what
pmdp_establish() returned. Is there a risk of racing with other parts of
the kernel? I guess not since the pmd is !present.

For arm64, I don't see the point of a cmpxchg, so something like below
would do (it needs proper testing though):

-------------8<---------------------------
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index c213fdbd056c..8fe1dad9100a 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -39,6 +39,7 @@
 
 #ifndef __ASSEMBLY__
 
+#include <asm/cmpxchg.h>
 #include <asm/fixmap.h>
 #include <linux/mmdebug.h>
 
@@ -683,6 +684,11 @@ static inline void pmdp_set_wrprotect(struct mm_struct *mm,
 {
 	ptep_set_wrprotect(mm, address, (pte_t *)pmdp);
 }
+
+static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
+{
+	return __pmd(xchg_relaxed(&pmd_val(*pmdp), pmd_val(pmd)));
+}
 #endif
 #endif	/* CONFIG_ARM64_HW_AFDBM */
 
-------------8<---------------------------

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1671577

On Tue, Jun 20, 2017 at 04:54:38PM +0100, Catalin Marinas wrote:
> On Tue, Jun 20, 2017 at 12:52:10AM +0300, Kirill A. Shutemov wrote:
> > On Mon, Jun 19, 2017 at 06:09:12PM +0100, Catalin Marinas wrote:
> > > On Mon, Jun 19, 2017 at 07:00:05PM +0300, Kirill A. Shutemov wrote:
> > > > On Mon, Jun 19, 2017 at 04:22:29PM +0100, Catalin Marinas wrote:
> > > > > On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> > > > > > We need an atomic way to setup pmd page table entry, avoiding races with
> > > > > > CPU setting dirty/accessed bits. This is required to implement
> > > > > > pmdp_invalidate() that doesn't loose these bits.
> > > > > > 
> > > > > > On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
> > > > > > setting it up half-by-half can expose broken corrupted entry to CPU.
> > > > > > 
> > > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > > > Cc: Ingo Molnar <mingo@kernel.org>
> > > > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > 
> > > > > I'll look at this from the arm64 perspective. It would be good if we can
> > > > > have a generic atomic implementation based on cmpxchg64 but I need to
> > > > > look at the details first.
> > > > 
> > > > Unfortunately, I'm not sure it's possbile.
> > > > 
> > > > The format of a page table is defined per-arch. We cannot assume much about
> > > > it in generic code.
> > > > 
> > > > I guess we could make it compile by casting to 'unsigned long', but is it
> > > > useful?
> > > > Every architecture manintainer still has to validate that this assumption
> > > > is valid for the architecture.
> > > 
> > > You are right, not much gained in doing this.
> > > 
> > > Maybe a stupid question but can we not implement pmdp_invalidate() with
> > > something like pmdp_get_and_clear() (usually reusing the ptep_*
> > > equivalent). Or pmdp_clear_flush() (again, reusing ptep_clear_flush())?
> > > 
> > > In my quick grep on pmdp_invalidate, it seems to be followed by
> > > set_pmd_at() or pmd_populate() already and the *pmd value after
> > > mknotpresent isn't any different from 0 to the hardware (at least on
> > > ARM). That's unless Linux expects to see some non-zero value here if
> > > walking the page tables on another CPU.
> > 
> > The whole reason to have pmdp_invalidate() in first place is to never make
> > pmd clear in the middle. Otherwise we will get race with MADV_DONTNEED.
> > See ced108037c2a for an example of such race.
> 
> Thanks for the explanation. So you basically just want to set a !present
> and !none pmd. I noticed that with your proposed pmdp_invalidate(),
> pmdp_establish(pmd_mknotpresent(*pmdp)) could set a stale *pmdp (with
> the present bit cleared) temporarily until updated with what
> pmdp_establish() returned. Is there a risk of racing with other parts of
> the kernel? I guess not since the pmd is !present.

I don't see such risk. Other parts of the kernel would see non-present pmd
and will have to take ptl to do anything meaningful with it.
pmdp_invalidate() caller has to hold ptl too, so the race is excluded.

> For arm64, I don't see the point of a cmpxchg, so something like below
> would do (it needs proper testing though):

Right. cmpxchg is required for x86 PAE, as it has sizeof(pmd_t) >
sizeof(long). We don't have 8-byte xchg() there.

Thanks, for the patch. I assume, I can use your signed-off-by, right?

Any chance you could help me with arm too?

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1671602

On Wed, Jun 21, 2017 at 12:53:03PM +0300, Kirill A. Shutemov wrote:
> On Tue, Jun 20, 2017 at 04:54:38PM +0100, Catalin Marinas wrote:
> > For arm64, I don't see the point of a cmpxchg, so something like below
> > would do (it needs proper testing though):
> 
> Right. cmpxchg is required for x86 PAE, as it has sizeof(pmd_t) >
> sizeof(long). We don't have 8-byte xchg() there.
> 
> Thanks, for the patch. I assume, I can use your signed-off-by, right?

Yes. And maybe some text (well, I just copied yours):

---------------8<--------------
arm64: Provide pmdp_establish() helper

We need an atomic way to setup pmd page table entry, avoiding races with
CPU setting dirty/accessed bits. This is required to implement
pmdp_invalidate() that doesn't lose these bits.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---------------8<--------------

> Any chance you could help me with arm too?

I'll have a look.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1671622

On Wed, Jun 21, 2017 at 12:53:03PM +0300, Kirill A. Shutemov wrote:
> > > > > > On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> > > > > > > We need an atomic way to setup pmd page table entry, avoiding races with
> > > > > > > CPU setting dirty/accessed bits. This is required to implement
> > > > > > > pmdp_invalidate() that doesn't loose these bits.
[...]
> Any chance you could help me with arm too?

On arm (ARMv7 with LPAE) we don't have hardware updates of the
access/dirty bits, so a generic implementation would suffice. I didn't
find one in your patches, so here's an untested version:

static inline pmd_t pmdp_establish(struct mm_struct *mm, unsigned long address,
				   pmd_t *pmdp, pmd_t pmd)
{
	pmd_t old_pmd = *pmdp;
	set_pmd_at(mm, address, pmdp, pmd);
	return old_pmd;
}

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1671639

On Wed, Jun 21, 2017 at 12:27:02PM +0100, Catalin Marinas wrote:
> On Wed, Jun 21, 2017 at 12:53:03PM +0300, Kirill A. Shutemov wrote:
> > > > > > > On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> > > > > > > > We need an atomic way to setup pmd page table entry, avoiding races with
> > > > > > > > CPU setting dirty/accessed bits. This is required to implement
> > > > > > > > pmdp_invalidate() that doesn't loose these bits.
> [...]
> > Any chance you could help me with arm too?
> 
> On arm (ARMv7 with LPAE) we don't have hardware updates of the
> access/dirty bits, so a generic implementation would suffice. I didn't
> find one in your patches, so here's an untested version:
> 
> static inline pmd_t pmdp_establish(struct mm_struct *mm, unsigned long address,
> 				   pmd_t *pmdp, pmd_t pmd)
> {
> 	pmd_t old_pmd = *pmdp;
> 	set_pmd_at(mm, address, pmdp, pmd);
> 	return old_pmd;
> }

Thanks, I'll integrate this into the patchset.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1671774

On 06/21/2017 04:27 AM, Catalin Marinas wrote:
> On Wed, Jun 21, 2017 at 12:53:03PM +0300, Kirill A. Shutemov wrote:
>>>>>>> On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
>>>>>>>> We need an atomic way to setup pmd page table entry, avoiding races with
>>>>>>>> CPU setting dirty/accessed bits. This is required to implement
>>>>>>>> pmdp_invalidate() that doesn't loose these bits.
> [...]
>> Any chance you could help me with arm too?
> On arm (ARMv7 with LPAE) we don't have hardware updates of the
> access/dirty bits, so a generic implementation would suffice. I didn't
> find one in your patches, so here's an untested version:
>
> static inline pmd_t pmdp_establish(struct mm_struct *mm, unsigned long address,
> 				   pmd_t *pmdp, pmd_t pmd)
> {
> 	pmd_t old_pmd = *pmdp;
> 	set_pmd_at(mm, address, pmdp, pmd);
> 	return old_pmd;
> }

So it seems the discussions have settled down and pmdp_establish() can be 
implemented in generic way as above and it will suffice if arch doesn't have a 
special need. It would be nice to add the comment above generic version that it 
only needs to be implemented if hardware sets the accessed/dirty bits !

Then nothing special is needed for ARC - right ?

-Vineet

[toc] | [prev] | [next] | [standalone]

#1671829

On Wed, Jun 21, 2017 at 08:49:03AM -0700, Vineet Gupta wrote:
> On 06/21/2017 04:27 AM, Catalin Marinas wrote:
> > On Wed, Jun 21, 2017 at 12:53:03PM +0300, Kirill A. Shutemov wrote:
> > > > > > > > On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> > > > > > > > > We need an atomic way to setup pmd page table entry, avoiding races with
> > > > > > > > > CPU setting dirty/accessed bits. This is required to implement
> > > > > > > > > pmdp_invalidate() that doesn't loose these bits.
> > [...]
> > > Any chance you could help me with arm too?
> > On arm (ARMv7 with LPAE) we don't have hardware updates of the
> > access/dirty bits, so a generic implementation would suffice. I didn't
> > find one in your patches, so here's an untested version:
> > 
> > static inline pmd_t pmdp_establish(struct mm_struct *mm, unsigned long address,
> > 				   pmd_t *pmdp, pmd_t pmd)
> > {
> > 	pmd_t old_pmd = *pmdp;
> > 	set_pmd_at(mm, address, pmdp, pmd);
> > 	return old_pmd;
> > }
> 
> So it seems the discussions have settled down and pmdp_establish() can be
> implemented in generic way as above and it will suffice if arch doesn't have
> a special need. It would be nice to add the comment above generic version
> that it only needs to be implemented if hardware sets the accessed/dirty
> bits !
> 
> Then nothing special is needed for ARC - right ?

I will define generic version as Catalin proposed with a comment, but
under the name generic_pmdp_establish. An arch can make use of it by

#define pmdp_establish generic_pmdp_establish

I don't want it to be used by default without attention from architecture
maintainer. It can lead unnoticied breakage if THP got enabled on new
arch.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1671836

On 06/21/2017 10:16 AM, Kirill A. Shutemov wrote:
> On Wed, Jun 21, 2017 at 08:49:03AM -0700, Vineet Gupta wrote:
>> On 06/21/2017 04:27 AM, Catalin Marinas wrote:
>>> On Wed, Jun 21, 2017 at 12:53:03PM +0300, Kirill A. Shutemov wrote:
>>>>>>>>> On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
>>>>>>>>>> We need an atomic way to setup pmd page table entry, avoiding races with
>>>>>>>>>> CPU setting dirty/accessed bits. This is required to implement
>>>>>>>>>> pmdp_invalidate() that doesn't loose these bits.
>>> [...]
>>>> Any chance you could help me with arm too?
>>> On arm (ARMv7 with LPAE) we don't have hardware updates of the
>>> access/dirty bits, so a generic implementation would suffice. I didn't
>>> find one in your patches, so here's an untested version:
>>>
>>> static inline pmd_t pmdp_establish(struct mm_struct *mm, unsigned long address,
>>> 				   pmd_t *pmdp, pmd_t pmd)
>>> {
>>> 	pmd_t old_pmd = *pmdp;
>>> 	set_pmd_at(mm, address, pmdp, pmd);
>>> 	return old_pmd;
>>> }
>> So it seems the discussions have settled down and pmdp_establish() can be
>> implemented in generic way as above and it will suffice if arch doesn't have
>> a special need. It would be nice to add the comment above generic version
>> that it only needs to be implemented if hardware sets the accessed/dirty
>> bits !
>>
>> Then nothing special is needed for ARC - right ?
> I will define generic version as Catalin proposed with a comment, but
> under the name generic_pmdp_establish. An arch can make use of it by
>
> #define pmdp_establish generic_pmdp_establish

Can you do that for ARC in your next posting - or want me to once you have posted 
that ?


> I don't want it to be used by default without attention from architecture
> maintainer. It can lead unnoticied breakage if THP got enabled on new
> arch.

Makes sense !

-Vineet

[toc] | [prev] | [next] | [standalone]

#1671863

On Wed, Jun 21, 2017 at 10:20:47AM -0700, Vineet Gupta wrote:
> On 06/21/2017 10:16 AM, Kirill A. Shutemov wrote:
> > On Wed, Jun 21, 2017 at 08:49:03AM -0700, Vineet Gupta wrote:
> > > On 06/21/2017 04:27 AM, Catalin Marinas wrote:
> > > > On Wed, Jun 21, 2017 at 12:53:03PM +0300, Kirill A. Shutemov wrote:
> > > > > > > > > > On Thu, Jun 15, 2017 at 05:52:22PM +0300, Kirill A. Shutemov wrote:
> > > > > > > > > > > We need an atomic way to setup pmd page table entry, avoiding races with
> > > > > > > > > > > CPU setting dirty/accessed bits. This is required to implement
> > > > > > > > > > > pmdp_invalidate() that doesn't loose these bits.
> > > > [...]
> > > > > Any chance you could help me with arm too?
> > > > On arm (ARMv7 with LPAE) we don't have hardware updates of the
> > > > access/dirty bits, so a generic implementation would suffice. I didn't
> > > > find one in your patches, so here's an untested version:
> > > > 
> > > > static inline pmd_t pmdp_establish(struct mm_struct *mm, unsigned long address,
> > > > 				   pmd_t *pmdp, pmd_t pmd)
> > > > {
> > > > 	pmd_t old_pmd = *pmdp;
> > > > 	set_pmd_at(mm, address, pmdp, pmd);
> > > > 	return old_pmd;
> > > > }
> > > So it seems the discussions have settled down and pmdp_establish() can be
> > > implemented in generic way as above and it will suffice if arch doesn't have
> > > a special need. It would be nice to add the comment above generic version
> > > that it only needs to be implemented if hardware sets the accessed/dirty
> > > bits !
> > > 
> > > Then nothing special is needed for ARC - right ?
> > I will define generic version as Catalin proposed with a comment, but
> > under the name generic_pmdp_establish. An arch can make use of it by
> > 
> > #define pmdp_establish generic_pmdp_establish
> 
> Can you do that for ARC in your next posting - or want me to once you have
> posted that ?

I'll do this.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1669524

Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote:

> We need an atomic way to setup pmd page table entry, avoiding races with
> CPU setting dirty/accessed bits. This is required to implement
> pmdp_invalidate() that doesn't loose these bits.
> 
> On PAE we have to use cmpxchg8b as we cannot assume what is value of new pmd and
> setting it up half-by-half can expose broken corrupted entry to CPU.

...

> 
> +#ifndef pmdp_establish
> +#define pmdp_establish pmdp_establish
> +static inline pmd_t pmdp_establish(pmd_t *pmdp, pmd_t pmd)
> +{
> +	if (IS_ENABLED(CONFIG_SMP)) {
> +		return xchg(pmdp, pmd);
> +	} else {
> +		pmd_t old = *pmdp;
> +		*pmdp = pmd;

I think you may want to use WRITE_ONCE() here - otherwise nobody guarantees
that the compiler will not split writes to *pmdp. Although the kernel uses
similar code to setting PTEs and PMDs, I think that it is best to start
fixing it. Obviously, you might need a different code path for 32-bit
kernels.

Regards,
Nadav

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web