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

Groups > linux.kernel > #1670901 > unrolled thread

Re: [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

  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

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

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] | [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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web