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

Groups > linux.kernel > #1665859 > unrolled thread

[HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages

Back to article view | Back to linux.kernel

Contents

  [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-06-14 16:00 +0200
    [PATCH 2/3] mm: Do not loose dirty and access bits in pmdp_invalidate() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-06-14 16:00 +0200
    Re: [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-06-14 16:10 +0200
    Re: [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-14 17:30 +0200
      Re: [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages Will Deacon <will.deacon@arm.com> - 2017-06-14 19:00 +0200
        Re: [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages Vlastimil Babka <vbabka@suse.cz> - 2017-06-14 19:10 +0200
          Re: [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-15 03:40 +0200
        Re: [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-15 03:10 +0200
          Re: [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-15 05:00 +0200
          Re: [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-15 10:50 +0200
            Re: [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-15 11:40 +0200

#1665859 — [HELP-NEEDED, PATCH 0/3] Do not loose dirty bit on THP pages

Hi,

Vlastimil noted that pmdp_invalidate() is not atomic and we can loose
dirty and access bits if CPU sets them after pmdp dereference, but
before set_pmd_at().

The bug doesn't lead to user-visible misbehaviour in current kernel, but
fixing this would be critical for future work on THP: both huge-ext4 and THP
swap out rely on proper dirty tracking.

Unfortunately, there's no way to address the issue in a generic way. We need to
fix all architectures that support THP one-by-one.

All architectures that have THP supported have to provide atomic
pmdp_invalidate(). If generic implementation of pmdp_invalidate() is used,
architecture needs to provide atomic pmdp_mknonpresent().

I've fixed the issue for x86, but I need help with the rest.

So far THP is supported on 8 architectures. Power and S390 already provides
atomic pmdp_invalidate(). x86 is fixed by this patches, so 5 architectures
left:

 - arc;
 - arm;
 - arm64;
 - mips;
 - sparc -- it has custom pmdp_invalidate(), but it's racy too;

Please, help me with them.

Kirill A. Shutemov (3):
  x86/mm: Provide pmdp_mknotpresent() helper
  mm: Do not loose dirty and access bits in pmdp_invalidate()
  mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()

 arch/x86/include/asm/pgtable-3level.h | 17 +++++++++++++++++
 arch/x86/include/asm/pgtable.h        | 13 +++++++++++++
 mm/huge_memory.c                      | 13 +++++++++----
 mm/pgtable-generic.c                  |  3 +--
 4 files changed, 40 insertions(+), 6 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1665860 — [PATCH 2/3] mm: Do not loose dirty and access bits in pmdp_invalidate()

Vlastimil noted that pmdp_invalidate() is not atomic and we can loose
dirty and access bits if CPU sets them after pmdp dereference, but
before set_pmd_at().

The bug doesn't lead to user-visible misbehaviour in current kernel.

Loosing access bit can lead to sub-optimal reclaim behaviour for THP,
but nothing destructive.

Loosing dirty bit is not a big deal too: we would make page dirty
unconditionally on splitting huge page.

The fix is critical for future work on THP: both huge-ext4 and THP swap
out rely on proper dirty tracking.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/pgtable-generic.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
index c99d9512a45b..68094fa190d1 100644
--- a/mm/pgtable-generic.c
+++ b/mm/pgtable-generic.c
@@ -182,8 +182,7 @@ pgtable_t pgtable_trans_huge_withdraw(struct mm_struct *mm, pmd_t *pmdp)
 void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
 		     pmd_t *pmdp)
 {
-	pmd_t entry = *pmdp;
-	set_pmd_at(vma->vm_mm, address, pmdp, pmd_mknotpresent(entry));
+	pmdp_mknotpresent(pmdp);
 	flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
 }
 #endif
-- 
2.11.0

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

#1665866

Hi Kirill,

On Wed, 14 Jun 2017 16:51:40 +0300
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> Vlastimil noted that pmdp_invalidate() is not atomic and we can loose
> dirty and access bits if CPU sets them after pmdp dereference, but
> before set_pmd_at().
> 
> The bug doesn't lead to user-visible misbehaviour in current kernel, but
> fixing this would be critical for future work on THP: both huge-ext4 and THP
> swap out rely on proper dirty tracking.
> 
> Unfortunately, there's no way to address the issue in a generic way. We need to
> fix all architectures that support THP one-by-one.
> 
> All architectures that have THP supported have to provide atomic
> pmdp_invalidate(). If generic implementation of pmdp_invalidate() is used,
> architecture needs to provide atomic pmdp_mknonpresent().
> 
> I've fixed the issue for x86, but I need help with the rest.
> 
> So far THP is supported on 8 architectures. Power and S390 already provides
> atomic pmdp_invalidate(). x86 is fixed by this patches, so 5 architectures
> left:

For s390 the pmdp_invalidate() is atomic only in regard to the dirty and
referenced bits because we use a fault driven approach for this, no?

More specifically the update via the pmdp_xchg_direct() function is protected
by the page table lock, the update on the pmd entry itself does *not* have
to be atomic (for s390).

-- 
blue skies,
   Martin.

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

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

#1665988

On Wednesday 14 June 2017 07:21 PM, Kirill A. Shutemov wrote:
> Hi,
> 
> Vlastimil noted that pmdp_invalidate() is not atomic and we can loose
> dirty and access bits if CPU sets them after pmdp dereference, but
> before set_pmd_at().
> 
> The bug doesn't lead to user-visible misbehaviour in current kernel, but
> fixing this would be critical for future work on THP: both huge-ext4 and THP
> swap out rely on proper dirty tracking.
> 
> Unfortunately, there's no way to address the issue in a generic way. We need to
> fix all architectures that support THP one-by-one.
> 
> All architectures that have THP supported have to provide atomic
> pmdp_invalidate(). If generic implementation of pmdp_invalidate() is used,
> architecture needs to provide atomic pmdp_mknonpresent().
> 
> I've fixed the issue for x86, but I need help with the rest.
> 
> So far THP is supported on 8 architectures. Power and S390 already provides
> atomic pmdp_invalidate(). x86 is fixed by this patches, so 5 architectures
> left:
> 
>   - arc;
>   - arm;
>   - arm64;
>   - mips;
>   - sparc -- it has custom pmdp_invalidate(), but it's racy too;
> 
> Please, help me with them.
> 
> Kirill A. Shutemov (3):
>    x86/mm: Provide pmdp_mknotpresent() helper
>    mm: Do not loose dirty and access bits in pmdp_invalidate()
>    mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()
> 


But in __split_huge_pmd_locked() we collected the dirty bit early. So 
even if we made pmdp_invalidate() atomic, if we had marked the pmd pte 
entry dirty after we collected the dirty bit, we still loose it right ?


May be we should relook at pmd PTE udpate interface. We really need an 
interface that can update pmd entries such that we don't clear it in 
between. IMHO, we can avoid the pmdp_invalidate() completely, if we can 
switch from a pmd PTE entry to a pointer to PTE page (pgtable_t). We 
also need this interface to avoid the madvise race fixed by

https://lkml.kernel.org/r/20170302151034.27829-1-kirill.shutemov@linux.intel.com

The usage of pmdp_invalidate while splitting the pmd also need updated 
documentation. In the earlier version of thp, we were required to keep 
the pmd present and marked splitting, so that code paths can wait till 
the splitting is done.

With the current design, we can ideally mark the pmdp not present early 
on right ? As long as we hold the pmd lock a parallel fault will try to 
mark the pmd accessed and wait on the pmd lock. On taking the lock it 
will find the pmd modified and we should retry access again ?

-aneesh

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

#1666073

Hi Aneesh,

On Wed, Jun 14, 2017 at 08:55:26PM +0530, Aneesh Kumar K.V wrote:
> On Wednesday 14 June 2017 07:21 PM, Kirill A. Shutemov wrote:
> >Vlastimil noted that pmdp_invalidate() is not atomic and we can loose
> >dirty and access bits if CPU sets them after pmdp dereference, but
> >before set_pmd_at().
> >
> >The bug doesn't lead to user-visible misbehaviour in current kernel, but
> >fixing this would be critical for future work on THP: both huge-ext4 and THP
> >swap out rely on proper dirty tracking.
> >
> >Unfortunately, there's no way to address the issue in a generic way. We need to
> >fix all architectures that support THP one-by-one.
> >
> >All architectures that have THP supported have to provide atomic
> >pmdp_invalidate(). If generic implementation of pmdp_invalidate() is used,
> >architecture needs to provide atomic pmdp_mknonpresent().
> >
> >I've fixed the issue for x86, but I need help with the rest.
> >
> >So far THP is supported on 8 architectures. Power and S390 already provides
> >atomic pmdp_invalidate(). x86 is fixed by this patches, so 5 architectures
> >left:
> >
> >  - arc;
> >  - arm;
> >  - arm64;
> >  - mips;
> >  - sparc -- it has custom pmdp_invalidate(), but it's racy too;
> >
> >Please, help me with them.
> >
> >Kirill A. Shutemov (3):
> >   x86/mm: Provide pmdp_mknotpresent() helper
> >   mm: Do not loose dirty and access bits in pmdp_invalidate()
> >   mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()
> >
> 
> 
> But in __split_huge_pmd_locked() we collected the dirty bit early. So even
> if we made pmdp_invalidate() atomic, if we had marked the pmd pte entry
> dirty after we collected the dirty bit, we still loose it right ?
> 
> 
> May be we should relook at pmd PTE udpate interface. We really need an
> interface that can update pmd entries such that we don't clear it in
> between. IMHO, we can avoid the pmdp_invalidate() completely, if we can
> switch from a pmd PTE entry to a pointer to PTE page (pgtable_t). We also
> need this interface to avoid the madvise race fixed by

There's a good chance I'm not following your suggestion here, but it's
probably worth me pointing out that swizzling a page table entry from a
block mapping (e.g. a huge page mapped at the PMD level) to a table entry
(e.g. a pointer to a page of PTEs) can lead to all sorts of horrible
problems on ARM, including amalgamation of TLB entries and fatal aborts.

So we really need to go via an invalid entry, with appropriate TLB
invalidation before installing the new entry.

Will

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

#1666077

On 06/14/2017 06:55 PM, Will Deacon wrote:
>>
>> May be we should relook at pmd PTE udpate interface. We really need an
>> interface that can update pmd entries such that we don't clear it in
>> between. IMHO, we can avoid the pmdp_invalidate() completely, if we can
>> switch from a pmd PTE entry to a pointer to PTE page (pgtable_t). We also
>> need this interface to avoid the madvise race fixed by
> 
> There's a good chance I'm not following your suggestion here, but it's
> probably worth me pointing out that swizzling a page table entry from a
> block mapping (e.g. a huge page mapped at the PMD level) to a table entry
> (e.g. a pointer to a page of PTEs) can lead to all sorts of horrible
> problems on ARM, including amalgamation of TLB entries and fatal aborts.

AFAIK some AMD x86_64 CPU's had the same problem and generated MCE's,
and on Intel there are some restrictions when you can do that. See the
large comment in __split_huge_pmd_locked().

> So we really need to go via an invalid entry, with appropriate TLB
> invalidation before installing the new entry.
> 
> Will
> 

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

#1666353

On Wednesday 14 June 2017 10:30 PM, Vlastimil Babka wrote:
> On 06/14/2017 06:55 PM, Will Deacon wrote:
>>>
>>> May be we should relook at pmd PTE udpate interface. We really need an
>>> interface that can update pmd entries such that we don't clear it in
>>> between. IMHO, we can avoid the pmdp_invalidate() completely, if we can
>>> switch from a pmd PTE entry to a pointer to PTE page (pgtable_t). We also
>>> need this interface to avoid the madvise race fixed by
>>
>> There's a good chance I'm not following your suggestion here, but it's
>> probably worth me pointing out that swizzling a page table entry from a
>> block mapping (e.g. a huge page mapped at the PMD level) to a table entry
>> (e.g. a pointer to a page of PTEs) can lead to all sorts of horrible
>> problems on ARM, including amalgamation of TLB entries and fatal aborts.
> 
> AFAIK some AMD x86_64 CPU's had the same problem and generated MCE's,
> and on Intel there are some restrictions when you can do that. See the
> large comment in __split_huge_pmd_locked().
> 

I was wondering whether we can do pmdp_establish(pgtable); and document 
all quirks needed for that in the per arch implementation of 
pmdp_establish(). We could also then switch all the 
pmdp_clear/set_pmd_at() usage to pmdp_establish().

-aneesh

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

#1666344

On Wednesday 14 June 2017 10:25 PM, Will Deacon wrote:
> Hi Aneesh,
> 
> On Wed, Jun 14, 2017 at 08:55:26PM +0530, Aneesh Kumar K.V wrote:
>> On Wednesday 14 June 2017 07:21 PM, Kirill A. Shutemov wrote:
>>> Vlastimil noted that pmdp_invalidate() is not atomic and we can loose
>>> dirty and access bits if CPU sets them after pmdp dereference, but
>>> before set_pmd_at().
>>>
>>> The bug doesn't lead to user-visible misbehaviour in current kernel, but
>>> fixing this would be critical for future work on THP: both huge-ext4 and THP
>>> swap out rely on proper dirty tracking.
>>>
>>> Unfortunately, there's no way to address the issue in a generic way. We need to
>>> fix all architectures that support THP one-by-one.
>>>
>>> All architectures that have THP supported have to provide atomic
>>> pmdp_invalidate(). If generic implementation of pmdp_invalidate() is used,
>>> architecture needs to provide atomic pmdp_mknonpresent().
>>>
>>> I've fixed the issue for x86, but I need help with the rest.
>>>
>>> So far THP is supported on 8 architectures. Power and S390 already provides
>>> atomic pmdp_invalidate(). x86 is fixed by this patches, so 5 architectures
>>> left:
>>>
>>>   - arc;
>>>   - arm;
>>>   - arm64;
>>>   - mips;
>>>   - sparc -- it has custom pmdp_invalidate(), but it's racy too;
>>>
>>> Please, help me with them.
>>>
>>> Kirill A. Shutemov (3):
>>>    x86/mm: Provide pmdp_mknotpresent() helper
>>>    mm: Do not loose dirty and access bits in pmdp_invalidate()
>>>    mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()
>>>
>>
>>
>> But in __split_huge_pmd_locked() we collected the dirty bit early. So even
>> if we made pmdp_invalidate() atomic, if we had marked the pmd pte entry
>> dirty after we collected the dirty bit, we still loose it right ?
>>
>>
>> May be we should relook at pmd PTE udpate interface. We really need an
>> interface that can update pmd entries such that we don't clear it in
>> between. IMHO, we can avoid the pmdp_invalidate() completely, if we can
>> switch from a pmd PTE entry to a pointer to PTE page (pgtable_t). We also
>> need this interface to avoid the madvise race fixed by
> 
> There's a good chance I'm not following your suggestion here, but it's
> probably worth me pointing out that swizzling a page table entry from a
> block mapping (e.g. a huge page mapped at the PMD level) to a table entry
> (e.g. a pointer to a page of PTEs) can lead to all sorts of horrible
> problems on ARM, including amalgamation of TLB entries and fatal aborts.
> 
> So we really need to go via an invalid entry, with appropriate TLB
> invalidation before installing the new entry.
> 

I am not suggesting we don't do the invalidate (the need for that is 
documented in __split_huge_pmd_locked(). I am suggesting we need a new 
interface, something like Andrea suggested.

old_pmd = pmdp_establish(pmd_mknotpresent());

instead of pmdp_invalidate(). We can then use this in scenarios where we 
want to update pmd PTE entries, where right now we go through a 
pmdp_clear and set_pmd path. We should really not do that for THP entries.


W.r.t pmdp_invalidate() usage, I was wondering whether we can do that 
early in __split_huge_pmd_locked().

-aneesh

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

#1666401

On Thursday 15 June 2017 06:35 AM, Aneesh Kumar K.V wrote:
> 

> W.r.t pmdp_invalidate() usage, I was wondering whether we can do that 
> early in __split_huge_pmd_locked().
> 


BTW by moving  pmdp_invalidate early, we can then get rid of

	pmdp_huge_split_prepare(vma, haddr, pmd);


-aneesh

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

#1666570

On Thu, Jun 15, 2017 at 06:35:21AM +0530, Aneesh Kumar K.V wrote:
> 
> 
> On Wednesday 14 June 2017 10:25 PM, Will Deacon wrote:
> > Hi Aneesh,
> > 
> > On Wed, Jun 14, 2017 at 08:55:26PM +0530, Aneesh Kumar K.V wrote:
> > > On Wednesday 14 June 2017 07:21 PM, Kirill A. Shutemov wrote:
> > > > Vlastimil noted that pmdp_invalidate() is not atomic and we can loose
> > > > dirty and access bits if CPU sets them after pmdp dereference, but
> > > > before set_pmd_at().
> > > > 
> > > > The bug doesn't lead to user-visible misbehaviour in current kernel, but
> > > > fixing this would be critical for future work on THP: both huge-ext4 and THP
> > > > swap out rely on proper dirty tracking.
> > > > 
> > > > Unfortunately, there's no way to address the issue in a generic way. We need to
> > > > fix all architectures that support THP one-by-one.
> > > > 
> > > > All architectures that have THP supported have to provide atomic
> > > > pmdp_invalidate(). If generic implementation of pmdp_invalidate() is used,
> > > > architecture needs to provide atomic pmdp_mknonpresent().
> > > > 
> > > > I've fixed the issue for x86, but I need help with the rest.
> > > > 
> > > > So far THP is supported on 8 architectures. Power and S390 already provides
> > > > atomic pmdp_invalidate(). x86 is fixed by this patches, so 5 architectures
> > > > left:
> > > > 
> > > >   - arc;
> > > >   - arm;
> > > >   - arm64;
> > > >   - mips;
> > > >   - sparc -- it has custom pmdp_invalidate(), but it's racy too;
> > > > 
> > > > Please, help me with them.
> > > > 
> > > > Kirill A. Shutemov (3):
> > > >    x86/mm: Provide pmdp_mknotpresent() helper
> > > >    mm: Do not loose dirty and access bits in pmdp_invalidate()
> > > >    mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()
> > > > 
> > > 
> > > 
> > > But in __split_huge_pmd_locked() we collected the dirty bit early. So even
> > > if we made pmdp_invalidate() atomic, if we had marked the pmd pte entry
> > > dirty after we collected the dirty bit, we still loose it right ?
> > > 
> > > 
> > > May be we should relook at pmd PTE udpate interface. We really need an
> > > interface that can update pmd entries such that we don't clear it in
> > > between. IMHO, we can avoid the pmdp_invalidate() completely, if we can
> > > switch from a pmd PTE entry to a pointer to PTE page (pgtable_t). We also
> > > need this interface to avoid the madvise race fixed by
> > 
> > There's a good chance I'm not following your suggestion here, but it's
> > probably worth me pointing out that swizzling a page table entry from a
> > block mapping (e.g. a huge page mapped at the PMD level) to a table entry
> > (e.g. a pointer to a page of PTEs) can lead to all sorts of horrible
> > problems on ARM, including amalgamation of TLB entries and fatal aborts.
> > 
> > So we really need to go via an invalid entry, with appropriate TLB
> > invalidation before installing the new entry.
> > 
> 
> I am not suggesting we don't do the invalidate (the need for that is
> documented in __split_huge_pmd_locked(). I am suggesting we need a new
> interface, something like Andrea suggested.
> 
> old_pmd = pmdp_establish(pmd_mknotpresent());
> 
> instead of pmdp_invalidate(). We can then use this in scenarios where we
> want to update pmd PTE entries, where right now we go through a pmdp_clear
> and set_pmd path. We should really not do that for THP entries.

Which cases are you talking about? When do we need to clear pmd and set
later?

-- 
 Kirill A. Shutemov

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

#1666600

On Thursday 15 June 2017 02:18 PM, Kirill A. Shutemov wrote:
> O
>> I am not suggesting we don't do the invalidate (the need for that is
>> documented in __split_huge_pmd_locked(). I am suggesting we need a new
>> interface, something like Andrea suggested.
>>
>> old_pmd = pmdp_establish(pmd_mknotpresent());
>>
>> instead of pmdp_invalidate(). We can then use this in scenarios where we
>> want to update pmd PTE entries, where right now we go through a pmdp_clear
>> and set_pmd path. We should really not do that for THP entries.
> 
> Which cases are you talking about? When do we need to clear pmd and set
> later?
> 

With the latest upstream I am finding the usage when we mark pte clean 
page_mkclean_one . Also there is a similar usage in 
migrate_misplaced_transhuge_page(). I haven't really verified whether 
they do cause any race. But my suggestion is, we should avoid the usage 
of set_pmd_at() unless we are creating a new pmd PTE entry. If we can 
provide pmdp_establish() we can achieve that easily.

-aneesh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web