[PATCH 28/83] mm: Change timing of notification to IOMMUs about a page to be invalidated

[PATCH 28/83] mm: Change timing of notification to IOMMUs about a page to be invalidated

[Oded Gabbay]

From: Andrew Lewycky <Andrew.Lewycky@amd.com>

This patch changes the location of the mmu_notifier_invalidate_page function
call inside try_to_unmap_one. The mmu_notifier_invalidate_page function
call tells the IOMMU that a pgae should be invalidated.

The location is changed from after releasing the physical page to
before releasing the physical page.

This change should prevent the bug that would occur in the
(rare) case where the GPU attempts to access a page while the CPU
attempts to swap out that page (or discard it if it is not dirty).

Signed-off-by: Andrew Lewycky <Andrew.Lewycky@amd.com>
Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
---
 mm/rmap.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 196cd0c..73d4c3d 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1231,13 +1231,17 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 	} else
 		dec_mm_counter(mm, MM_FILEPAGES);
 
+	pte_unmap_unlock(pte, ptl);
+
+	mmu_notifier_invalidate_page(vma, address, event);
+
 	page_remove_rmap(page);
 	page_cache_release(page);
 
+	return ret;
+
 out_unmap:
 	pte_unmap_unlock(pte, ptl);
-	if (ret != SWAP_FAIL && !(flags & TTU_MUNLOCK))
-		mmu_notifier_invalidate_page(vma, address, event);
 out:
 	return ret;
 
-- 
1.9.1

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

Re: [PATCH 28/83] mm: Change timing of notification to IOMMUs about a page to be invalidated

[Andrew Morton]

On Fri, 11 Jul 2014 00:53:26 +0300 Oded Gabbay <oded.gabbay@gmail.com> wrote:

> From: Andrew Lewycky <Andrew.Lewycky@amd.com>
> 
> This patch changes the location of the mmu_notifier_invalidate_page function
> call inside try_to_unmap_one. The mmu_notifier_invalidate_page function
> call tells the IOMMU that a pgae should be invalidated.
> 
> The location is changed from after releasing the physical page to
> before releasing the physical page.
> 
> This change should prevent the bug that would occur in the
> (rare) case where the GPU attempts to access a page while the CPU
> attempts to swap out that page (or discard it if it is not dirty).

um OK, but what is the effect on all the other
mmu_notifier_ops.invalidate_page() implementations?

Please spell this out in full detail within the changelog and be sure
to cc the affected maintainers.


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

Re: [PATCH 28/83] mm: Change timing of notification to IOMMUs about a page to be invalidated

[Joerg Roedel]

On Fri, Jul 11, 2014 at 12:53:26AM +0300, Oded Gabbay wrote:
>  mm/rmap.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 196cd0c..73d4c3d 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1231,13 +1231,17 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>  	} else
>  		dec_mm_counter(mm, MM_FILEPAGES);
>  
> +	pte_unmap_unlock(pte, ptl);
> +
> +	mmu_notifier_invalidate_page(vma, address, event);
> +
>  	page_remove_rmap(page);
>  	page_cache_release(page);
>  
> +	return ret;
> +
>  out_unmap:
>  	pte_unmap_unlock(pte, ptl);
> -	if (ret != SWAP_FAIL && !(flags & TTU_MUNLOCK))
> -		mmu_notifier_invalidate_page(vma, address, event);
>  out:
>  	return ret;

I think there is no bug. In that function the page is just unmapped,
removed from the rmap (page_remove_rmap), and the LRU list
(page_cache_release). The page itself is not released in this function,
so the call mmu_notifier_invalidate_page() at the end is fine.


	Joerg


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

Re: [PATCH 28/83] mm: Change timing of notification to IOMMUs about a page to be invalidated

[Hugh Dickins]

On Fri, 11 Jul 2014, Joerg Roedel wrote:
> On Fri, Jul 11, 2014 at 12:53:26AM +0300, Oded Gabbay wrote:
> >  mm/rmap.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 196cd0c..73d4c3d 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1231,13 +1231,17 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> >  	} else
> >  		dec_mm_counter(mm, MM_FILEPAGES);
> >  
> > +	pte_unmap_unlock(pte, ptl);
> > +
> > +	mmu_notifier_invalidate_page(vma, address, event);
> > +
> >  	page_remove_rmap(page);
> >  	page_cache_release(page);
> >  
> > +	return ret;
> > +
> >  out_unmap:
> >  	pte_unmap_unlock(pte, ptl);
> > -	if (ret != SWAP_FAIL && !(flags & TTU_MUNLOCK))
> > -		mmu_notifier_invalidate_page(vma, address, event);
> >  out:
> >  	return ret;
> 
> I think there is no bug. In that function the page is just unmapped,
> removed from the rmap (page_remove_rmap), and the LRU list
> (page_cache_release). The page itself is not released in this function,
> so the call mmu_notifier_invalidate_page() at the end is fine.

Agreed, nothing to fix here: the try_to_unmap() callers must hold
their own reference to the page.  If they did not, how could they
be sure that this is a page which is appropriate to unmap?

(Nit: we don't actually take a separate reference for the LRU list:
the page_cache_release above corresponds to the reference in the
pte which has just been removed.)

Hugh

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