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

Commit Message

Oded Gabbay July 10, 2014, 9:53 p.m. UTC

From: Andrew Lewycky <[e-mail]>

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 <[e-mail]>
Signed-off-by: Oded Gabbay <[e-mail]>
---
 mm/rmap.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Hugh Dickins July 10, 2014, 10:34 p.m. UTC | #1

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

Patch

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;