hugetlbfs: Take read_lock on i_mmap for PMD sharing

A customer with large SMP systems (up to 16 sockets) with application that uses large amount of static hugepages (~500-1500GB) are experiencing random multisecond delays. These delays was caused by the long time it took to scan the VMA interval tree with mmap_sem held.

The sharing of huge PMD does not require changes to the i_mmap at all. As a result, we can just take the read lock and let other threads searching for the right VMA to share in parallel. Once the right VMA is found, either the PMD lock (2M huge page for x86-64) or the mm->page_table_lock will be acquired to perform the actual PMD sharing.

Lock contention, if present, will happen in the spinlock. That is much better than contention in the rwsem where the time needed to scan the the interval tree is indeterminate.

With this patch applied, the customer is seeing significant improvements over the unpatched kernel.

Signed-off-by Waiman Long <[email protected]>

---
 mm/hugetlb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

mm/hugetlb.c +5 -1

index b45a95363a84..087e7ff00137 100644
@@ -4842,7 +4842,11 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
 	if (!vma_shareable(vma, addr))
 		return (pte_t *)pmd_alloc(mm, pud, addr);
 
-	i_mmap_lock_write(mapping);
+	/*
+	 * PMD sharing does not require changes to i_mmap. So a read lock
+	 * is enuogh.
+	 */
+	i_mmap_lock_read(mapping);
 	vma_interval_tree_foreach(svma, &mapping->i_mmap, idx, idx) {
 		if (svma == vma)
 			continue;

A customer with large SMP systems (up to 16 sockets) with application that uses large amount of static hugepages (~500-1500GB) are experiencing random multisecond delays. These delays was caused by the long time it took to scan the VMA interval tree with mmap_sem held.

The sharing of huge PMD does not require changes to the i_mmap at all. As a result, we can just take the read lock and let other threads searching for the right VMA to share in parallel. Once the right VMA is found, either the PMD lock (2M huge page for x86-64) or the mm->page_table_lock will be acquired to perform the actual PMD sharing.

Lock contention, if present, will happen in the spinlock. That is much better than contention in the rwsem where the time needed to scan the the interval tree is indeterminate.

With this patch applied, the customer is seeing significant improvements over the unpatched kernel.

Signed-off-by Waiman Long <[email protected]>

---
 mm/hugetlb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

mm/hugetlb.c +5 -1

index b45a95363a84..087e7ff00137 100644
@@ -4842,7 +4842,11 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
 	if (!vma_shareable(vma, addr))
 		return (pte_t *)pmd_alloc(mm, pud, addr);
 
-	i_mmap_lock_write(mapping);
+	/*
+	 * PMD sharing does not require changes to i_mmap. So a read lock
+	 * is enuogh.
+	 */
+	i_mmap_lock_read(mapping);
 	vma_interval_tree_foreach(svma, &mapping->i_mmap, idx, idx) {
 		if (svma == vma)
 			continue;

A customer with large SMP systems (up to 16 sockets) with application that uses large amount of static hugepages (~500-1500GB) are experiencing random multisecond delays. These delays was caused by the long time it took to scan the VMA interval tree with mmap_sem held.

The sharing of huge PMD does not require changes to the i_mmap at all. As a result, we can just take the read lock and let other threads searching for the right VMA to share in parallel. Once the right VMA is found, either the PMD lock (2M huge page for x86-64) or the mm->page_table_lock will be acquired to perform the actual PMD sharing.

Lock contention, if present, will happen in the spinlock. That is much better than contention in the rwsem where the time needed to scan the the interval tree is indeterminate.

With this patch applied, the customer is seeing significant improvements over the unpatched kernel.

Signed-off-by Waiman Long <[email protected]>

---
 mm/hugetlb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

mm/hugetlb.c +0 -0

index b45a95363a84..087e7ff00137 100644

mm/hugetlb.c +3 -1

+	 */
+	i_mmap_lock_read(mapping);
 	vma_interval_tree_foreach(svma, &mapping->i_mmap, idx, idx) {
 		if (svma == vma)
 			continue;

@@ -4872,7 +4876,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
 	spin_unlock(ptl);
 out:
 	pte = (pte_t *)pmd_alloc(mm, pud, addr);
-	i_mmap_unlock_write(mapping);
+	i_mmap_unlock_read(mapping);
 	return pte;
 }
 

mm/hugetlb.c pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud) · line 4849

Quoted 6 lines · Waiman Long

mm/hugetlb.c +5 -1

-	i_mmap_lock_write(mapping);
+	/*
+	 * PMD sharing does not require changes to i_mmap. So a read lock
+	 * is enuogh.
+	 */
+	i_mmap_lock_read(mapping);

We don't have comments before any of the other calls to i_mmap_lock_read() justifying why we don't need the write lock. I don't understand why this situation is different. Just delete the comment and make this a two-line patch.

A customer with large SMP systems (up to 16 sockets) with application that uses large amount of static hugepages (~500-1500GB) are experiencing random multisecond delays. These delays was caused by the long time it took to scan the VMA interval tree with mmap_sem held.

The sharing of huge PMD does not require changes to the i_mmap at all. As a result, we can just take the read lock and let other threads searching for the right VMA to share in parallel. Once the right VMA is found, either the PMD lock (2M huge page for x86-64) or the mm->page_table_lock will be acquired to perform the actual PMD sharing.

Lock contention, if present, will happen in the spinlock. That is much better than contention in the rwsem where the time needed to scan the the interval tree is indeterminate.

Quoted 11 lines · Waiman Long

A customer with large SMP systems (up to 16 sockets) with application that uses large amount of static hugepages (~500-1500GB) are experiencing random multisecond delays. These delays was caused by the long time it took to scan the VMA interval tree with mmap_sem held.

The sharing of huge PMD does not require changes to the i_mmap at all. As a result, we can just take the read lock and let other threads searching for the right VMA to share in parallel. Once the right VMA is found, either the PMD lock (2M huge page for x86-64) or the mm->page_table_lock will be acquired to perform the actual PMD sharing.

Quoted 3 lines

Lock contention, if present, will happen in the spinlock. That is much better than contention in the rwsem where the time needed to scan the the interval tree is indeterminate.

I don't think this description really explains the contention argument well. There are _more_ PMD locks than there are i_mmap_sem locks, so processes accessing different parts of the same file can work in parallel.

Are there other current users of the write lock that could use a read lock? At first blush, it would seem that unmap_ref_private() also only needs a read lock on the i_mmap tree. I don't think hugetlb_change_protection() needs the write lock either. Nor retract_page_tables().

Are there other current users of the write lock that could use a read lock? At first blush, it would seem that unmap_ref_private() also only needs a read lock on the i_mmap tree. I don't think hugetlb_change_protection() needs the write lock either. Nor retract_page_tables().

Quoted 4 lines · Matthew Wilcox

Are there other current users of the write lock that could use a read lock? At first blush, it would seem that unmap_ref_private() also only needs a read lock on the i_mmap tree. I don't think hugetlb_change_protection() needs the write lock either. Nor retract_page_tables().

I believe that the semaphore still needs to be held in write mode while calling huge_pmd_unshare (as is done in the call sites above). Why? There is this check for sharing in huge_pmd_unshare,

	if (page_count(virt_to_page(ptep)) == 1)
		return 0;	// implies no sharing

Note that huge_pmd_share now increments the page count with the semaphore held just in read mode. It is OK to do increments in parallel without synchronization. However, we don't want anyone else changing the count while that check in huge_pmd_unshare is happening. Hence, the need for taking the semaphore in write mode.

Note that huge_pmd_share now increments the page count with the semaphore held just in read mode. It is OK to do increments in parallel without synchronization. However, we don't want anyone else changing the count while that check in huge_pmd_unshare is happening. Hence, the need for taking the semaphore in write mode.

Quoted 5 lines · Mike Kravetz

Note that huge_pmd_share now increments the page count with the semaphore held just in read mode. It is OK to do increments in parallel without synchronization. However, we don't want anyone else changing the count while that check in huge_pmd_unshare is happening. Hence, the need for taking the semaphore in write mode.

This would be a nice addition to the changelog methinks.

Quoted 5 lines · Mike Kravetz

Note that huge_pmd_share now increments the page count with the semaphore held just in read mode. It is OK to do increments in parallel without synchronization. However, we don't want anyone else changing the count while that check in huge_pmd_unshare is happening. Hence, the need for taking the semaphore in write mode.

This would be a nice addition to the changelog methinks.

Quoted 8 lines · Davidlohr Bueso

Quoted 5 lines · Mike Kravetz

Note that huge_pmd_share now increments the page count with the semaphore held just in read mode. It is OK to do increments in parallel without synchronization. However, we don't want anyone else changing the count while that check in huge_pmd_unshare is happening. Hence, the need for taking the semaphore in write mode.

Quoted 1 line

This would be a nice addition to the changelog methinks.

Last night I remembered there is one place where we currently take i_mmap_rwsem in read mode and potentially call huge_pmd_unshare. That is in try_to_unmap_one. Yes, there is a potential race here today.

But that race is somewhat contained as you need two threads doing some combination of page migration and page poisoning to race. This change now allows migration or poisoning to race with page fault. I would really prefer if we do not open up the race window in this manner.

Quoted 8 lines · Mike Kravetz

Quoted 7 lines · Davidlohr Bueso

Quoted 5 lines · Mike Kravetz

Note that huge_pmd_share now increments the page count with the semaphore held just in read mode. It is OK to do increments in parallel without synchronization. However, we don't want anyone else changing the count while that check in huge_pmd_unshare is happening. Hence, the need for taking the semaphore in write mode.

Quoted 4 lines

Quoted 1 line

This would be a nice addition to the changelog methinks.

Last night I remembered there is one place where we currently take i_mmap_rwsem in read mode and potentially call huge_pmd_unshare. That is in try_to_unmap_one. Yes, there is a potential race here today.

Actually there is no race there today. Callers to huge_pmd_unshare hold the page table lock. So, this synchronizes those unshare calls from page migration and page poisoning.

Quoted 4 lines

But that race is somewhat contained as you need two threads doing some combination of page migration and page poisoning to race. This change now allows migration or poisoning to race with page fault. I would really prefer if we do not open up the race window in this manner.

But, we do open a race window by changing huge_pmd_share to take the i_mmap_rwsem in read mode as in the original patch.

Here is the additional code needed to take the semaphore in write mode for the huge_pmd_unshare calls via try_to_unmap_one. We would need to combine this with Longman's patch. Please take a look and provide feedback. Some of the changes are subtle, especially the exception for MAP_PRIVATE mappings, but I tried to add sufficient comments.

From 21735818a520705c8573b8d543b8f91aa187bd5d Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Fri, 8 Nov 2019 17:25:37 -0800
Subject: [PATCH] Changes needed for taking i_mmap_rwsem in write mode before
call to huge_pmd_unshare in try_to_unmap_one.

Signed-off-by Mike Kravetz <[email protected]>

---
 mm/hugetlb.c        |  9 ++++++++-
 mm/memory-failure.c | 28 +++++++++++++++++++++++++++-
 mm/migrate.c        | 27 +++++++++++++++++++++++++--
 3 files changed, 60 insertions(+), 4 deletions(-)

mm/hugetlb.c +0 -0

index f78891f92765..73d9136549a5 100644
@@ -4883,7 +4883,14 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
  * indicated by page_count > 1, unmap is achieved by clearing pud and
  * decrementing the ref count. If count == 1, the pte page is not shared.

mm/hugetlb.c +0 -0

  *
  * returns: 1 successfully unmapped a shared pte page
  *	    0 the underlying pte page is not shared, or it is the last user

mm/memory-failure.c +27 -1

index 3151c87dff73..8f52b22cf71b 100644
@@ -1030,7 +1030,33 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	if (kill)
 		collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
 
-	unmap_success = try_to_unmap(hpage, ttu);
+	if (!PageHuge(hpage)) {
+		unmap_success = try_to_unmap(hpage, ttu);
+	} else {
+		mapping = page_mapping(hpage);
+		if (mapping) {
+			/*
+			 * For hugetlb pages, try_to_unmap could potentially
+			 * call huge_pmd_unshare.  Because of this, take
+			 * semaphore in write mode here and set TTU_RMAP_LOCKED
+			 * to indicate we have taken the lock at this higher
+			 * level.
+			 */
+			i_mmap_lock_write(mapping);
+			unmap_success = try_to_unmap(hpage,
+							ttu|TTU_RMAP_LOCKED);
+			i_mmap_unlock_write(mapping);
+		} else {
+			/*
+			 * !mapping implies a MAP_PRIVATE huge page mapping.
+			 * Since PMDs will never be shared in a private
+			 * mapping, it is safe to let huge_pmd_unshare be
+			 * called with the semaphore in read mode.
+			 */
+			unmap_success = try_to_unmap(hpage, ttu);
+		}
+	}
+
 	if (!unmap_success)
 		pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
 		       pfn, page_mapcount(hpage));

mm/migrate.c +25 -2

index 4fe45d1428c8..9cae5a4f1e48 100644
@@ -1333,8 +1333,31 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 		goto put_anon;
 
 	if (page_mapped(hpage)) {
-		try_to_unmap(hpage,
-			TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
+		struct address_space *mapping = page_mapping(hpage);
+
+		if (mapping) {
+			/*
+			 * try_to_unmap could potentially call huge_pmd_unshare.
+			 * Because of this, take semaphore in write mode here
+			 * and set TTU_RMAP_LOCKED to indicate we have taken
+			 * the lock at this higher level.
+			 */
+			i_mmap_lock_write(mapping);
+			try_to_unmap(hpage,
+				TTU_MIGRATION|TTU_IGNORE_MLOCK|
+				TTU_IGNORE_ACCESS|TTU_RMAP_LOCKED);
+			i_mmap_unlock_write(mapping);
+		} else {
+			/*
+			 * !mapping implies a MAP_PRIVATE huge page mapping.
+			 * Since PMDs will never be shared in a private
+			 * mapping, it is safe to let huge_pmd_unshare be
+			 * called with the semaphore in read mode.
+			 */
+			try_to_unmap(hpage,
+				TTU_MIGRATION|TTU_IGNORE_MLOCK|
+				TTU_IGNORE_ACCESS);
+		}
 		page_was_mapped = 1;
 	}
 

Quoted 56 lines · Mike Kravetz

Quoted 8 lines · Mike Kravetz

Quoted 7 lines · Davidlohr Bueso

Quoted 5 lines · Mike Kravetz

Note that huge_pmd_share now increments the page count with the semaphore held just in read mode. It is OK to do increments in parallel without synchronization. However, we don't want anyone else changing the count while that check in huge_pmd_unshare is happening. Hence, the need for taking the semaphore in write mode.

Quoted 4 lines

Quoted 1 line

This would be a nice addition to the changelog methinks.

Last night I remembered there is one place where we currently take i_mmap_rwsem in read mode and potentially call huge_pmd_unshare. That is in try_to_unmap_one. Yes, there is a potential race here today.

Actually there is no race there today. Callers to huge_pmd_unshare hold the page table lock. So, this synchronizes those unshare calls from page migration and page poisoning.

Quoted 4 lines

But that race is somewhat contained as you need two threads doing some combination of page migration and page poisoning to race. This change now allows migration or poisoning to race with page fault. I would really prefer if we do not open up the race window in this manner.

But, we do open a race window by changing huge_pmd_share to take the i_mmap_rwsem in read mode as in the original patch.

Here is the additional code needed to take the semaphore in write mode for the huge_pmd_unshare calls via try_to_unmap_one. We would need to combine this with Longman's patch. Please take a look and provide feedback. Some of the changes are subtle, especially the exception for MAP_PRIVATE mappings, but I tried to add sufficient comments.

From 21735818a520705c8573b8d543b8f91aa187bd5d Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Fri, 8 Nov 2019 17:25:37 -0800
Subject: [PATCH] Changes needed for taking i_mmap_rwsem in write mode before
call to huge_pmd_unshare in try_to_unmap_one.

Signed-off-by Mike Kravetz <[email protected]>

---
 mm/hugetlb.c        |  9 ++++++++-
 mm/memory-failure.c | 28 +++++++++++++++++++++++++++-
 mm/migrate.c        | 27 +++++++++++++++++++++++++--
 3 files changed, 60 insertions(+), 4 deletions(-)

mm/hugetlb.c +4 -1

index f78891f92765..73d9136549a5 100644
@@ -4883,7 +4883,14 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
  * indicated by page_count > 1, unmap is achieved by clearing pud and
  * decrementing the ref count. If count == 1, the pte page is not shared.
  *
- * called with page table lock held.
+ * Must be called while holding page table lock.
+ * In general, the caller should also hold the i_mmap_rwsem in write mode.
+ * This is to prevent races with page faults calling huge_pmd_share which
+ * will not be holding the page table lock, but will be holding i_mmap_rwsem

mm/hugetlb.c pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud) · line 4893

Quoted 4 lines

mm/hugetlb.c +4 -0

+ * in read mode.  It is possible to call without holding i_mmap_rwsem in
+ * write mode if the caller KNOWS the page table is associated with a private
+ * mapping.  This is because private mappings can not share PMDs and can
+ * not race with huge_pmd_share calls during page faults.

So the page table lock here is the huge_pte_lock(). Right? In huge_pmd_share(), the pte lock has to be taken before one can share it. So would you mind explaining where exactly is the race?

Quoted 56 lines · Waiman Long

Quoted 55 lines · Mike Kravetz

Quoted 8 lines · Mike Kravetz

Quoted 7 lines · Davidlohr Bueso

Quoted 5 lines · Mike Kravetz

Note that huge_pmd_share now increments the page count with the semaphore held just in read mode. It is OK to do increments in parallel without synchronization. However, we don't want anyone else changing the count while that check in huge_pmd_unshare is happening. Hence, the need for taking the semaphore in write mode.

Quoted 4 lines

Quoted 1 line

This would be a nice addition to the changelog methinks.

Last night I remembered there is one place where we currently take i_mmap_rwsem in read mode and potentially call huge_pmd_unshare. That is in try_to_unmap_one. Yes, there is a potential race here today.

Actually there is no race there today. Callers to huge_pmd_unshare hold the page table lock. So, this synchronizes those unshare calls from page migration and page poisoning.

Quoted 4 lines

But that race is somewhat contained as you need two threads doing some combination of page migration and page poisoning to race. This change now allows migration or poisoning to race with page fault. I would really prefer if we do not open up the race window in this manner.

But, we do open a race window by changing huge_pmd_share to take the i_mmap_rwsem in read mode as in the original patch.

Here is the additional code needed to take the semaphore in write mode for the huge_pmd_unshare calls via try_to_unmap_one. We would need to combine this with Longman's patch. Please take a look and provide feedback. Some of the changes are subtle, especially the exception for MAP_PRIVATE mappings, but I tried to add sufficient comments.

From 21735818a520705c8573b8d543b8f91aa187bd5d Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Fri, 8 Nov 2019 17:25:37 -0800
Subject: [PATCH] Changes needed for taking i_mmap_rwsem in write mode before
call to huge_pmd_unshare in try_to_unmap_one.

Signed-off-by Mike Kravetz <[email protected]>

---
 mm/hugetlb.c        |  9 ++++++++-
 mm/memory-failure.c | 28 +++++++++++++++++++++++++++-
 mm/migrate.c        | 27 +++++++++++++++++++++++++--
 3 files changed, 60 insertions(+), 4 deletions(-)

mm/hugetlb.c +3 -1

index f78891f92765..73d9136549a5 100644
@@ -4883,7 +4883,14 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
  * indicated by page_count > 1, unmap is achieved by clearing pud and
  * decrementing the ref count. If count == 1, the pte page is not shared.
  *
- * called with page table lock held.
+ * Must be called while holding page table lock.
+ * In general, the caller should also hold the i_mmap_rwsem in write mode.
+ * This is to prevent races with page faults calling huge_pmd_share which

Quoted 8 lines

mm/hugetlb.c pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud) · line 4893

Quoted 5 lines

mm/hugetlb.c +5 -0

+ * will not be holding the page table lock, but will be holding i_mmap_rwsem
+ * in read mode.  It is possible to call without holding i_mmap_rwsem in
+ * write mode if the caller KNOWS the page table is associated with a private
+ * mapping.  This is because private mappings can not share PMDs and can
+ * not race with huge_pmd_share calls during page faults.

So the page table lock here is the huge_pte_lock(). Right? In huge_pmd_share(), the pte lock has to be taken before one can share it. So would you mind explaining where exactly is the race?

My concern was that this code in huge_pmd_share:

		saddr = page_table_shareable(svma, vma, addr, idx);
		if (saddr) {
			spte = huge_pte_offset(svma->vm_mm, saddr,
					       vma_mmu_pagesize(svma));
			if (spte) {
				get_page(virt_to_page(spte));
				break;
			}
		}

and this code in huge_pmd_unshare:

        BUG_ON(page_count(virt_to_page(ptep)) == 0);
        if (page_count(virt_to_page(ptep)) == 1)
                return 0;

could run concurrently. Note that return code for huge_pmd_unshare is specified as,

* returns: 1 successfully unmapped a shared pte page
* 0 the underlying pte page is not shared, or it is the last user

And, callers take different action depending on the return value.

Now, since the code blocks above can run concurrently it is possible that:
- huge_pmd_unshare sees page_count == 1
- huge_pmd_share increments page_count in anticipation of sharing
- huge_pmd_unshare returns 0 indicating there is no pmd sharing
- huge_pmd_share waits for page table lock to insert pmd page before it sharts sharing

My concern was with what might happen if a huge_pmd_unshare caller received a 0 return code and thought there were no other users of the pmd. In the specific case mentioned here (try_to_unmap_one) I now do not believe there are any issues. The caller is simply going to clear one entry on the pmd page. I can not think of a way this could impact the other thread waiting to share the pmd. It will simply start sharing the pmd after it gets the page table lock and the entry has been removed.

As the result of your question, I went back and took a really close look at the code in question. While that huge_pmd_share/huge_pmd_unshare code running concurrently does make me a bit nervous, I can not find any specific problem. In addition, I ran a test causes this race for several hours without issue.

Sorry for the churn/second thoughts. This code is subtle, and sometimes hard to understand. IMO, Longman's patch (V2) can go forward, but please delete the following blob in the commit message from me:

"Note that huge_pmd_share now increments the page count with the
semaphore held just in read mode. It is OK to do increments in parallel without synchronization. However, we don't want anyone else changing the count while that check in huge_pmd_unshare is happening. Hence, the need for taking the semaphore in write mode."