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

Groups > linux.kernel > #1665853 > unrolled thread

[PATCH 3/3] mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()

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

  [PATCH 3/3] mm, thp: Do not loose dirty bit in __split_huge_pmd_locked() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-06-14 16:00 +0200
    Re: [PATCH 3/3] mm, thp: Do not loose dirty bit in  __split_huge_pmd_locked() Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-06-14 16:20 +0200
      Re: [PATCH 3/3] mm, thp: Do not loose dirty bit in  __split_huge_pmd_locked() Andrea Arcangeli <aarcange@redhat.com> - 2017-06-14 17:40 +0200
        Re: [PATCH 3/3] mm, thp: Do not loose dirty bit in  __split_huge_pmd_locked() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-15 10:50 +0200
    Re: [PATCH 3/3] mm, thp: Do not loose dirty bit in  __split_huge_pmd_locked() "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-14 17:30 +0200

#1665853 — [PATCH 3/3] mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()

Until pmdp_invalidate() pmd entry is present and CPU can update it,
setting dirty. Currently, we tranfer dirty bit to page too early and
there is window when we can miss dirty bit.

Let's call SetPageDirty() after pmdp_invalidate().

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/huge_memory.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a84909cf20d3..c4ee5c890910 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1928,7 +1928,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 	struct page *page;
 	pgtable_t pgtable;
 	pmd_t _pmd;
-	bool young, write, dirty, soft_dirty;
+	bool young, write, soft_dirty;
 	unsigned long addr;
 	int i;
 
@@ -1965,7 +1965,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 	page_ref_add(page, HPAGE_PMD_NR - 1);
 	write = pmd_write(*pmd);
 	young = pmd_young(*pmd);
-	dirty = pmd_dirty(*pmd);
 	soft_dirty = pmd_soft_dirty(*pmd);
 
 	pmdp_huge_split_prepare(vma, haddr, pmd);
@@ -1995,8 +1994,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 			if (soft_dirty)
 				entry = pte_mksoft_dirty(entry);
 		}
-		if (dirty)
-			SetPageDirty(page + i);
 		pte = pte_offset_map(&_pmd, addr);
 		BUG_ON(!pte_none(*pte));
 		set_pte_at(mm, addr, pte, entry);
@@ -2046,6 +2043,14 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 	 * pmd_populate.
 	 */
 	pmdp_invalidate(vma, haddr, pmd);
+
+	/*
+	 * Transfer dirty bit to page after pmd invalidated, so CPU would not
+	 * be able to set it under us.
+	 */
+	if (pmd_dirty(*pmd))
+		SetPageDirty(page);
+
 	pmd_populate(mm, pmd, pgtable);
 
 	if (freeze) {
-- 
2.11.0

[toc] | [next] | [standalone]

#1665871 — Re: [PATCH 3/3] mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()

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

> Until pmdp_invalidate() pmd entry is present and CPU can update it,
> setting dirty. Currently, we tranfer dirty bit to page too early and
> there is window when we can miss dirty bit.
> 
> Let's call SetPageDirty() after pmdp_invalidate().
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ...
> @@ -2046,6 +2043,14 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  	 * pmd_populate.
>  	 */
>  	pmdp_invalidate(vma, haddr, pmd);
> +
> +	/*
> +	 * Transfer dirty bit to page after pmd invalidated, so CPU would not
> +	 * be able to set it under us.
> +	 */
> +	if (pmd_dirty(*pmd))
> +		SetPageDirty(page);
> +
>  	pmd_populate(mm, pmd, pgtable);
> 
>  	if (freeze) {

That won't work on s390. After pmdp_invalidate the pmd entry is gone,
it has been replaced with _SEGMENT_ENTRY_EMPTY. This includes the
dirty and referenced bits. The old scheme is

        entry = *pmd;
        pmdp_invalidate(vma, addr, pmd);
	if (pmd_dirty(entry))
		...

Could we change pmdp_invalidate to make it return the old pmd entry?
The pmdp_xchg_direct function already returns it, for s390 that would
be an easy change. The above code snippet would change like this:

	entry = pmdp_invalidate(vma, addr, pmd);
	if (pmd_dirty(entry))
		...

-- 
blue skies,
   Martin.

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

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

#1666004 — Re: [PATCH 3/3] mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()

Hello,

On Wed, Jun 14, 2017 at 04:18:57PM +0200, Martin Schwidefsky wrote:
> Could we change pmdp_invalidate to make it return the old pmd entry?

That to me seems the simplest fix to avoid losing the dirty bit.

I earlier suggested to replace pmdp_invalidate with something like
old_pmd = pmdp_establish(pmd_mknotpresent(pmd)) (then tlb flush could
then be conditional to the old pmd being present). Making
pmdp_invalidate return the old pmd entry would be mostly equivalent to
that.

The advantage of not changing pmdp_invalidate is that we could skip a
xchg which is more costly in __split_huge_pmd_locked and
madvise_free_huge_pmd so perhaps there's a point to keep a variant of
pmdp_invalidate that doesn't use xchg internally (and in turn can't
return the old pmd value atomically).

If we don't want new messy names like pmdp_establish we could have a
__pmdp_invalidate that returns void, and pmdp_invalidate that returns
the old pmd and uses xchg (and it'd also be backwards compatible as
far as the callers are concerned). So those places that don't need the
old value returned and can skip the xchg, could simply
s/pmdp_invalidate/__pmdp_invalidate/ to optimize.

One way or another for change_huge_pmd I think we need a xchg like in
native_pmdp_get_and_clear but that sets the pmd to
pmd_mknotpresent(pmd) instead of zero. And this whole issues
originates because both change_huge_pmd(prot_numa = 1) and
madvise_free_huge_pmd both run concurrently with the mmap_sem for
reading.

In the earlier email on this topic, I also mentioned the concern of
the _notify mmu notifier invalidate that got dropped silently with the
s/pmdp_huge_get_and_clear_notify/pmdp_invalidate/ conversion but I
later noticed the mmu notifier invalidate is already covered by the
caller. So change_huge_pmd should have called pmdp_huge_get_and_clear
in the first place and the _notify prefix in the old code was a
mistake as far as I can tell. So we can focus only on the dirty bit
retention issue.

Thanks,
Andrea

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

#1666562 — Re: [PATCH 3/3] mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()

On Wed, Jun 14, 2017 at 05:31:31PM +0200, Andrea Arcangeli wrote:
> Hello,
> 
> On Wed, Jun 14, 2017 at 04:18:57PM +0200, Martin Schwidefsky wrote:
> > Could we change pmdp_invalidate to make it return the old pmd entry?
> 
> That to me seems the simplest fix to avoid losing the dirty bit.
> 
> I earlier suggested to replace pmdp_invalidate with something like
> old_pmd = pmdp_establish(pmd_mknotpresent(pmd)) (then tlb flush could
> then be conditional to the old pmd being present). Making
> pmdp_invalidate return the old pmd entry would be mostly equivalent to
> that.
> 
> The advantage of not changing pmdp_invalidate is that we could skip a
> xchg which is more costly in __split_huge_pmd_locked and
> madvise_free_huge_pmd so perhaps there's a point to keep a variant of
> pmdp_invalidate that doesn't use xchg internally (and in turn can't
> return the old pmd value atomically).
> 
> If we don't want new messy names like pmdp_establish we could have a
> __pmdp_invalidate that returns void, and pmdp_invalidate that returns
> the old pmd and uses xchg (and it'd also be backwards compatible as
> far as the callers are concerned). So those places that don't need the
> old value returned and can skip the xchg, could simply
> s/pmdp_invalidate/__pmdp_invalidate/ to optimize.

We have few pmdp_invalidate() callers:

 - clear_soft_dirty_pmd();
 - madvise_free_huge_pmd();
 - change_huge_pmd();
 - __split_huge_pmd_locked();

Only madvise_free_huge_pmd() doesn't care about old pmd.

__split_huge_pmd_locked() actually needs to check dirty after
pmdp_invalidate(), see patch 3/3 of the patchset.

I don't think it worth introduce one more primitive only for
madvise_free_huge_pmd().

I'll stick with single pmdp_invalidate() that returns old value.

-- 
 Kirill A. Shutemov

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

#1665996 — Re: [PATCH 3/3] mm, thp: Do not loose dirty bit in __split_huge_pmd_locked()

On Wednesday 14 June 2017 07:21 PM, Kirill A. Shutemov wrote:
> Until pmdp_invalidate() pmd entry is present and CPU can update it,
> setting dirty. Currently, we tranfer dirty bit to page too early and
> there is window when we can miss dirty bit.
> 
> Let's call SetPageDirty() after pmdp_invalidate().
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>   mm/huge_memory.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index a84909cf20d3..c4ee5c890910 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1928,7 +1928,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>   	struct page *page;
>   	pgtable_t pgtable;
>   	pmd_t _pmd;
> -	bool young, write, dirty, soft_dirty;
> +	bool young, write, soft_dirty;
>   	unsigned long addr;
>   	int i;
> 
> @@ -1965,7 +1965,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>   	page_ref_add(page, HPAGE_PMD_NR - 1);
>   	write = pmd_write(*pmd);
>   	young = pmd_young(*pmd);
> -	dirty = pmd_dirty(*pmd);
>   	soft_dirty = pmd_soft_dirty(*pmd);
> 
>   	pmdp_huge_split_prepare(vma, haddr, pmd);
> @@ -1995,8 +1994,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>   			if (soft_dirty)
>   				entry = pte_mksoft_dirty(entry);
>   		}
> -		if (dirty)
> -			SetPageDirty(page + i);
>   		pte = pte_offset_map(&_pmd, addr);
>   		BUG_ON(!pte_none(*pte));
>   		set_pte_at(mm, addr, pte, entry);
> @@ -2046,6 +2043,14 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>   	 * pmd_populate.
>   	 */
>   	pmdp_invalidate(vma, haddr, pmd);
> +
> +	/*
> +	 * Transfer dirty bit to page after pmd invalidated, so CPU would not
> +	 * be able to set it under us.
> +	 */
> +	if (pmd_dirty(*pmd))
> +		SetPageDirty(page);
> +
>   	pmd_populate(mm, pmd, pgtable);
> 

you fixed dirty bit loosing i discussed in my previous mail here.

thanks
-aneesh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web