[PATCH 28/83] mm: Change timing of notification to IOMMUs about a page to be invalidated
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;
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.
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
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
dri-devel@lists.freedesktop.org
-
Andrew Morton -
Hugh Dickins -
Joerg Roedel -
Oded Gabbay