hugetlb: call mmu notifiers on hugepage cow

PATCH In review Doug Doan 3 participants 10 messages May 27, 2010 – Jun 3, 2010 Memory management

lore

Share & tools

lore

Doug Doan

author

2010-05-27 20:43 UTC #0 lore raw

PATCH 1/1 hugetlb: call mmu notifiers on hugepage cow

From: Doug Doan <[email protected]>

When a copy-on-write occurs, we take one of two paths in handle_mm_fault: through handle_pte_fault for normal pages, or through hugetlb_fault for huge pages.

In the normal page case, we eventually get to do_wp_page and call mmu notifiers via ptep_clear_flush_notify. There is no callout to the mmmu notifiers in the huge page case. This patch fixes that.

Signed-off-by Doug Doan <[email protected]>

---

hugetlb.c renamed from hugetlb.c.orig +6 -0

@@ -2345,11 +2345,17 @@ retry_avoidcopy:
 	ptep = huge_pte_offset(mm, address & huge_page_mask(h));
 	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
 		/* Break COW */
+		mmu_notifier_invalidate_range_start(mm,
+			address & huge_page_mask(h),
+			(address & huge_page_mask(h)) + huge_page_size(h));
 		huge_ptep_clear_flush(vma, address, ptep);
 		set_huge_pte_at(mm, address, ptep,
 				make_huge_pte(vma, new_page, 1));
 		/* Make the old page be freed below */
 		new_page = old_page;
+		mmu_notifier_invalidate_range_end(mm,
+			address & huge_page_mask(h),
+			(address & huge_page_mask(h)) + huge_page_size(h));
 	}
 	page_cache_release(new_page);
 	page_cache_release(old_page);

Mel Gorman

2010-05-28 10:00 UTC #1 lore raw

Quoted 16 lines · Doug Doan

From: Doug Doan <[email protected]>
When a copy-on-write occurs, we take one of two paths in handle_mm_fault:
through handle_pte_fault for normal pages, or through hugetlb_fault for huge pages.
In the normal page case, we eventually get to do_wp_page and call mmu notifiers via ptep_clear_flush_notify. There is no callout to the mmmu notifiers in the huge page case. This patch fixes that.
Signed-off-by Doug Doan <[email protected]>
---

hugetlb.c retry_avoidcopy: · line 2350

Quoted 6 lines

hugetlb.c renamed from hugetlb.c.orig +3 -0

@@ -2345,11 +2345,17 @@ retry_avoidcopy:
 	ptep = huge_pte_offset(mm, address & huge_page_mask(h));
 	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
 		/* Break COW */
+		mmu_notifier_invalidate_range_start(mm,
+			address & huge_page_mask(h),
+			(address & huge_page_mask(h)) + huge_page_size(h));

Should the address not already be aligned?

Otherwise, I don't see any problem.

Quoted 11 lines

hugetlb.c renamed from hugetlb.c.orig +3 -0

 		huge_ptep_clear_flush(vma, address, ptep);
 		set_huge_pte_at(mm, address, ptep,
 				make_huge_pte(vma, new_page, 1));
 		/* Make the old page be freed below */
 		new_page = old_page;
+		mmu_notifier_invalidate_range_end(mm,
+			address & huge_page_mask(h),
+			(address & huge_page_mask(h)) + huge_page_size(h));
 	}
 	page_cache_release(new_page);
 	page_cache_release(old_page);

Signature · 9 lines

Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Doug Doan

author

2010-05-28 16:40 UTC #2 lore raw

Quoted 14 lines · Mel Gorman

Quoted 12 lines · Doug Doan
From: Doug Doan<[email protected]>
When a copy-on-write occurs, we take one of two paths in handle_mm_fault:
through handle_pte_fault for normal pages, or through hugetlb_fault for huge pages.
In the normal page case, we eventually get to do_wp_page and call mmu notifiers via ptep_clear_flush_notify. There is no callout to the mmmu notifiers in the huge page case. This patch fixes that.
Signed-off-by Doug Doan<[email protected]>
---

Quoted 11 lines

hugetlb.c retry_avoidcopy: · line 2350

Quoted 9 lines

hugetlb.c renamed from hugetlb.c.orig +3 -0

@@ -2345,11 +2345,17 @@ retry_avoidcopy:
  	ptep = huge_pte_offset(mm, address&  huge_page_mask(h));
  	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
  		/* Break COW */
+		mmu_notifier_invalidate_range_start(mm,
+			address&  huge_page_mask(h),
+			(address&  huge_page_mask(h)) + huge_page_size(h));

Should the address not already be aligned?

I'm not seeing where the address was aligned before this point. The code just above aligns it:

   	ptep = huge_pte_offset(mm, address&  huge_page_mask(h));
   	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
   		/* Break COW */

Quoted 15 lines

Otherwise, I don't see any problem.

hugetlb.c retry_avoidcopy: · line 2361

Quoted 11 lines

hugetlb.c renamed from hugetlb.c.orig +3 -0

  		huge_ptep_clear_flush(vma, address, ptep);
  		set_huge_pte_at(mm, address, ptep,
  				make_huge_pte(vma, new_page, 1));
  		/* Make the old page be freed below */
  		new_page = old_page;
+		mmu_notifier_invalidate_range_end(mm,
+			address&  huge_page_mask(h),
+			(address&  huge_page_mask(h)) + huge_page_size(h));
  	}
  	page_cache_release(new_page);
  	page_cache_release(old_page);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mel Gorman

Acked-by

2010-05-28 17:34 UTC #3 lore raw

Quoted 27 lines · Doug Doan

Quoted 14 lines · Mel Gorman
Quoted 12 lines · Doug Doan
From: Doug Doan<[email protected]>
When a copy-on-write occurs, we take one of two paths in handle_mm_fault:
through handle_pte_fault for normal pages, or through hugetlb_fault for huge pages.
In the normal page case, we eventually get to do_wp_page and call mmu notifiers via ptep_clear_flush_notify. There is no callout to the mmmu notifiers in the huge page case. This patch fixes that.
Signed-off-by Doug Doan<[email protected]>
---
Quoted 11 lines
hugetlb.c retry_avoidcopy: · line 2350
Quoted 9 lines
hugetlb.c renamed from hugetlb.c.orig +3 -0
@@ -2345,11 +2345,17 @@ retry_avoidcopy:
  	ptep = huge_pte_offset(mm, address&  huge_page_mask(h));
  	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
  		/* Break COW */
+		mmu_notifier_invalidate_range_start(mm,
+			address&  huge_page_mask(h),
+			(address&  huge_page_mask(h)) + huge_page_size(h));
Should the address not already be aligned?

Quoted 7 lines

I'm not seeing where the address was aligned before this point. The code just above aligns it:
   	ptep = huge_pte_offset(mm, address&  huge_page_mask(h));
   	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
   		/* Break COW */

I'm sorry. You're right. I was looking at the copy_huge_page which was not aligning the address. It should be but it's ultimately harmless as the parameter is discarded.

Acked-by Mel Gorman <[email protected]>

Quoted 16 lines

Quoted 15 lines

Otherwise, I don't see any problem.

Quoted 11 lines

hugetlb.c renamed from hugetlb.c.orig +3 -0

  		huge_ptep_clear_flush(vma, address, ptep);
  		set_huge_pte_at(mm, address, ptep,
  				make_huge_pte(vma, new_page, 1));
  		/* Make the old page be freed below */
  		new_page = old_page;
+		mmu_notifier_invalidate_range_end(mm,
+			address&  huge_page_mask(h),
+			(address&  huge_page_mask(h)) + huge_page_size(h));
  	}
  	page_cache_release(new_page);
  	page_cache_release(old_page);

Signature · 3 lines

Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Andrew Morton

maintainer

2010-06-02 06:17 UTC #4 lore raw

In reply to Doug Doan

hugetlb.c

Quoted 14 lines · On Thu, 27 May 2010 13:43:00 -0700 Doug Doan <[email protected]> wrote:

When a copy-on-write occurs, we take one of two paths in handle_mm_fault:
through handle_pte_fault for normal pages, or through hugetlb_fault for huge pages.
In the normal page case, we eventually get to do_wp_page and call mmu notifiers via ptep_clear_flush_notify. There is no callout to the mmmu notifiers in the huge page case. This patch fixes that.
Signed-off-by Doug Doan <[email protected]>
---
[patch text/plain (802B)]
hugetlb.c renamed from hugetlb.c.orig +0 -0

(In patch -p1 form, please. So a/mm/hugetlb.c)

Quoted 8 lines

hugetlb.c renamed from hugetlb.c.orig +3 -0

@@ -2345,11 +2345,17 @@ retry_avoidcopy:
 	ptep = huge_pte_offset(mm, address & huge_page_mask(h));
 	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
 		/* Break COW */
+		mmu_notifier_invalidate_range_start(mm,
+			address & huge_page_mask(h),
+			(address & huge_page_mask(h)) + huge_page_size(h));
 		huge_ptep_clear_flush(vma, address, ptep);

hugetlb.c retry_avoidcopy: · line 2361

Quoted 10 lines

hugetlb.c renamed from hugetlb.c.orig +3 -0

 		set_huge_pte_at(mm, address, ptep,
 				make_huge_pte(vma, new_page, 1));
 		/* Make the old page be freed below */
 		new_page = old_page;
+		mmu_notifier_invalidate_range_end(mm,
+			address & huge_page_mask(h),
+			(address & huge_page_mask(h)) + huge_page_size(h));
 	}
 	page_cache_release(new_page);
 	page_cache_release(old_page);

This causes mmu_notifier_invalidate_range_start() to be called under page_table_lock. The immediately preceding code seems to take some care to avoid doing that. I took a quick look at other callsites and cannot immediately see other cases where mmu_notifier_invalidate_range_start/end() are called under that lock.

This may not introduce bugs with current notifier implementations (I didn't check), but it does lessen flexibility?

Doug Doan

author

2010-06-02 23:14 UTC #5 lore raw

Quoted 44 lines · Andrew Morton

hugetlb.c
Quoted 14 lines · On Thu, 27 May 2010 13:43:00 -0700 Doug Doan<[email protected]> wrote:
When a copy-on-write occurs, we take one of two paths in handle_mm_fault:
through handle_pte_fault for normal pages, or through hugetlb_fault for huge pages.
In the normal page case, we eventually get to do_wp_page and call mmu notifiers via ptep_clear_flush_notify. There is no callout to the mmmu notifiers in the huge page case. This patch fixes that.
Signed-off-by Doug Doan<[email protected]>
---
[patch text/plain (802B)]
hugetlb.c renamed from hugetlb.c.orig +0 -0
(In patch -p1 form, please. So a/mm/hugetlb.c)
Quoted 8 lines
hugetlb.c renamed from hugetlb.c.orig +3 -0
@@ -2345,11 +2345,17 @@ retry_avoidcopy:
  	ptep = huge_pte_offset(mm, address&  huge_page_mask(h));
  	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
  		/* Break COW */
+		mmu_notifier_invalidate_range_start(mm,
+			address&  huge_page_mask(h),
+			(address&  huge_page_mask(h)) + huge_page_size(h));
  		huge_ptep_clear_flush(vma, address, ptep);
hugetlb.c retry_avoidcopy: · line 2361
Quoted 10 lines
hugetlb.c renamed from hugetlb.c.orig +3 -0
  		set_huge_pte_at(mm, address, ptep,
  				make_huge_pte(vma, new_page, 1));
  		/* Make the old page be freed below */
  		new_page = old_page;
+		mmu_notifier_invalidate_range_end(mm,
+			address&  huge_page_mask(h),
+			(address&  huge_page_mask(h)) + huge_page_size(h));
  	}
  	page_cache_release(new_page);
  	page_cache_release(old_page);
This causes mmu_notifier_invalidate_range_start() to be called under page_table_lock. The immediately preceding code seems to take some care to avoid doing that. I took a quick look at other callsites and cannot immediately see other cases where mmu_notifier_invalidate_range_start/end() are called under that lock.

Quoted 2 lines

This may not introduce bugs with current notifier implementations (I didn't check), but it does lessen flexibility?

In the normal page case, handle_pte_fault calls do_wp_page inside a spinlock on ptl = pte_lockptr(mm, pmd), which uses mm->page_table_lock if USE_SPLIT_PTLOCKS is not defined.

I don't understand what you mean by lessen flexibilty.

Andrew Morton

maintainer

2010-06-02 23:34 UTC #6 lore raw

Quoted 45 lines · On Wed, 2 Jun 2010 16:13:42 -0700 Doug Doan <[email protected]> wrote:

Quoted 44 lines · Andrew Morton
hugetlb.c
Quoted 14 lines · On Thu, 27 May 2010 13:43:00 -0700 Doug Doan<[email protected]> wrote:
When a copy-on-write occurs, we take one of two paths in handle_mm_fault:
through handle_pte_fault for normal pages, or through hugetlb_fault for huge pages.
In the normal page case, we eventually get to do_wp_page and call mmu notifiers via ptep_clear_flush_notify. There is no callout to the mmmu notifiers in the huge page case. This patch fixes that.
Signed-off-by Doug Doan<[email protected]>
---
[patch text/plain (802B)]
hugetlb.c renamed from hugetlb.c.orig +0 -0
(In patch -p1 form, please. So a/mm/hugetlb.c)
Quoted 8 lines
hugetlb.c renamed from hugetlb.c.orig +3 -0
@@ -2345,11 +2345,17 @@ retry_avoidcopy:
  	ptep = huge_pte_offset(mm, address&  huge_page_mask(h));
  	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
  		/* Break COW */
+		mmu_notifier_invalidate_range_start(mm,
+			address&  huge_page_mask(h),
+			(address&  huge_page_mask(h)) + huge_page_size(h));
  		huge_ptep_clear_flush(vma, address, ptep);
hugetlb.c retry_avoidcopy: · line 2361
Quoted 10 lines
hugetlb.c renamed from hugetlb.c.orig +3 -0
  		set_huge_pte_at(mm, address, ptep,
  				make_huge_pte(vma, new_page, 1));
  		/* Make the old page be freed below */
  		new_page = old_page;
+		mmu_notifier_invalidate_range_end(mm,
+			address&  huge_page_mask(h),
+			(address&  huge_page_mask(h)) + huge_page_size(h));
  	}
  	page_cache_release(new_page);
  	page_cache_release(old_page);
This causes mmu_notifier_invalidate_range_start() to be called under page_table_lock. The immediately preceding code seems to take some care to avoid doing that. I took a quick look at other callsites and cannot immediately see other cases where mmu_notifier_invalidate_range_start/end() are called under that lock.

Quoted 8 lines

Quoted 2 lines
This may not introduce bugs with current notifier implementations (I didn't check), but it does lessen flexibility?
In the normal page case, handle_pte_fault calls do_wp_page inside a spinlock on ptl = pte_lockptr(mm, pmd), which uses mm->page_table_lock if USE_SPLIT_PTLOCKS is not defined.
I don't understand what you mean by lessen flexibilty.

Well, specifically it means that mmu_notifier_invalidate_range_start/end() implemetnations can no longer take page_table_lock or any lock which nests outside page_table_lock. That lessens flexibility.

As the other mmu_notifier_invalidate_range_start/end() callsite in this function carefully nested those calls outside page_table_lock, perhaps that was thought to be a significant thing.

Doug Doan

author

2010-06-03 17:36 UTC #7 lore raw

Quoted 57 lines · Andrew Morton

Quoted 45 lines · On Wed, 2 Jun 2010 16:13:42 -0700 Doug Doan<[email protected]> wrote:
Quoted 44 lines · Andrew Morton
hugetlb.c
Quoted 14 lines · On Thu, 27 May 2010 13:43:00 -0700 Doug Doan<[email protected]> wrote:
When a copy-on-write occurs, we take one of two paths in handle_mm_fault:
through handle_pte_fault for normal pages, or through hugetlb_fault for huge pages.
In the normal page case, we eventually get to do_wp_page and call mmu notifiers via ptep_clear_flush_notify. There is no callout to the mmmu notifiers in the huge page case. This patch fixes that.
Signed-off-by Doug Doan<[email protected]>
---
[patch text/plain (802B)]
hugetlb.c renamed from hugetlb.c.orig +0 -0
(In patch -p1 form, please. So a/mm/hugetlb.c)
Quoted 8 lines
hugetlb.c renamed from hugetlb.c.orig +3 -0
@@ -2345,11 +2345,17 @@ retry_avoidcopy:
   	ptep = huge_pte_offset(mm, address&   huge_page_mask(h));
   	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
   		/* Break COW */
+		mmu_notifier_invalidate_range_start(mm,
+			address&   huge_page_mask(h),
+			(address&   huge_page_mask(h)) + huge_page_size(h));
   		huge_ptep_clear_flush(vma, address, ptep);
hugetlb.c retry_avoidcopy: · line 2361
Quoted 10 lines
hugetlb.c renamed from hugetlb.c.orig +3 -0
   		set_huge_pte_at(mm, address, ptep,
   				make_huge_pte(vma, new_page, 1));
   		/* Make the old page be freed below */
   		new_page = old_page;
+		mmu_notifier_invalidate_range_end(mm,
+			address&   huge_page_mask(h),
+			(address&   huge_page_mask(h)) + huge_page_size(h));
   	}
   	page_cache_release(new_page);
   	page_cache_release(old_page);
This causes mmu_notifier_invalidate_range_start() to be called under page_table_lock. The immediately preceding code seems to take some care to avoid doing that. I took a quick look at other callsites and cannot immediately see other cases where mmu_notifier_invalidate_range_start/end() are called under that lock.
Quoted 8 lines
Quoted 2 lines
This may not introduce bugs with current notifier implementations (I didn't check), but it does lessen flexibility?
In the normal page case, handle_pte_fault calls do_wp_page inside a spinlock on ptl = pte_lockptr(mm, pmd), which uses mm->page_table_lock if USE_SPLIT_PTLOCKS is not defined.
I don't understand what you mean by lessen flexibilty.

Quoted 8 lines

Well, specifically it means that mmu_notifier_invalidate_range_start/end() implemetnations can no longer take page_table_lock or any lock which nests outside page_table_lock. That lessens flexibility.
As the other mmu_notifier_invalidate_range_start/end() callsite in this function carefully nested those calls outside page_table_lock, perhaps that was thought to be a significant thing.

Here's my rationale: for the normal page case, the invalidation call is done inside a page_table_lock, so the same should also be done in the huge page case. Does it really make sense to call invalidation on one hugepage and have another call invalidate the same hugepage while the first call is still not finished?

Andrew Morton

maintainer

2010-06-03 18:12 UTC #8 lore raw

Quoted 11 lines · On Thu, 3 Jun 2010 10:36:00 -0700 Doug Doan <[email protected]> wrote:

Quoted 8 lines
Well, specifically it means that mmu_notifier_invalidate_range_start/end() implemetnations can no longer take page_table_lock or any lock which nests outside page_table_lock. That lessens flexibility.
As the other mmu_notifier_invalidate_range_start/end() callsite in this function carefully nested those calls outside page_table_lock, perhaps that was thought to be a significant thing.
Here's my rationale: for the normal page case, the invalidation call is done inside a page_table_lock,

It is? Where does that happen?

Quoted 3 lines

so the same should also be done in the huge page case.
Does it really make sense to call invalidation on one hugepage and have another call invalidate the same hugepage while the first call is still not finished?

Doug Doan

author

2010-06-03 18:36 UTC #9 lore raw

Quoted 8 lines · Andrew Morton

Quoted 5 lines · On Thu, 3 Jun 2010 10:36:00 -0700 Doug Doan<[email protected]> wrote:
Quoted 5 lines
Well, specifically it means that mmu_notifier_invalidate_range_start/end() implemetnations can no longer take page_table_lock or any lock which nests outside page_table_lock. That lessens flexibility.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
As the other mmu_notifier_invalidate_range_start/end() callsite in this function carefully nested those calls outside page_table_lock, perhaps that was thought to be a significant thing.
Here's my rationale: for the normal page case, the invalidation call is done inside a page_table_lock,
It is? Where does that happen?

handle_pte_fault() acquires the lock before calling do_wp_page():

        ptl = pte_lockptr(mm, pmd);
        spin_lock(ptl);
        if (unlikely(!pte_same(*pte, entry)))
                goto unlock;
        if (flags & FAULT_FLAG_WRITE) {
                if (!pte_write(entry))
                        return do_wp_page(mm, vma, address,
                                        pte, pmd, ptl, entry);
                entry = pte_mkdirty(entry);
        }

do_wp_page() calls set_pte_at_notify(), which either calls mmu_notifier_change_pte() or mmu_notifier_invalidate_page().

Quoted 4 lines

Quoted 4 lines
so the same should also be done in the huge page case.
Does it really make sense to call invalidation on one hugepage and have another call invalidate the same hugepage while the first call is still not finished?