Skip to contentSkip to conversation

mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure

PATCH In review Wei Yang 3 participants 13 messages Nov 18, 2019 – Jan 9, 2020

Subsystems (2)
lore
Share & tools
lore

Wei Yang

author
#0 lore raw

PATCH 1/2 mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure

PageHuge is handled by memory_failure_hugetlb(), so this case could be removed.

---
 mm/memory-failure.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
mm/memory-failure.c +1 -4
index 3151c87dff73..392ac277b17d 100644
@@ -1359,10 +1359,7 @@ int memory_failure(unsigned long pfn, int flags)
 	 * page_remove_rmap() in try_to_unmap_one(). So to determine page status
 	 * correctly, we save a copy of the page flags at this time.
 	 */
-	if (PageHuge(p))
-		page_flags = hpage->flags;
-	else
-		page_flags = p->flags;
+	page_flags = p->flags;
 
 	/*
 	 * unpoison always clear PG_hwpoison inside page lock
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 2/2 mm/memory-failure.c: not necessary to recalculate hpage

hpage is not changed.

---
 mm/memory-failure.c | 1 -
 1 file changed, 1 deletion(-)
mm/memory-failure.c +0 -1
index 392ac277b17d..9784f4339ae7 100644
@@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
 		}
 		unlock_page(p);
 		VM_BUG_ON_PAGE(!page_count(p), p);
-		hpage = compound_head(p);
 	}
 
 	/*
Signature · 1 line
2.17.1
#2 lore raw

Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage

Quoted 13 lines · Wei Yang

hpage is not changed.

---
  mm/memory-failure.c | 1 -
  1 file changed, 1 deletion(-)
mm/memory-failure.c +0 -0
index 392ac277b17d..9784f4339ae7 100644

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1324

Quoted 7 lines
mm/memory-failure.c +0 -1
@@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
  		}
  		unlock_page(p);
  		VM_BUG_ON_PAGE(!page_count(p), p);
-		hpage = compound_head(p);
  	}
 
  	/*

I am *absolutely* no transparent huge page expert (sorry :) ), but won't the split_huge_page(p) eventually split the compound page, such that compound_head(p) will return something else after that call?

Signature · 3 lines
Thanks,

David / dhildenb
author
#3 lore raw

Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage

Quoted 8 lines · David Hildenbrand
Quoted 7 lines · Wei Yang

hpage is not changed.

---
  mm/memory-failure.c | 1 -
  1 file changed, 1 deletion(-)
Quoted 17 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1324

Quoted 12 lines
mm/memory-failure.c +0 -1
index 392ac277b17d..9784f4339ae7 100644
@@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
  		}
  		unlock_page(p);
  		VM_BUG_ON_PAGE(!page_count(p), p);
-		hpage = compound_head(p);
  	}
  	/*
 

I am *absolutely* no transparent huge page expert (sorry :) ), but won't the split_huge_page(p) eventually split the compound page, such that compound_head(p) will return something else after that call?

ok, you may get some point. I lost some concentration here.

Thanks

Quoted 5 lines
Signature · 3 lines
Thanks,

David / dhildenb
Signature · 2 lines
Wei Yang
Help you, Help me
#4 lore raw

Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage

In reply to David Hildenbrand
Quoted 8 lines · David Hildenbrand
Quoted 7 lines · Wei Yang

hpage is not changed.

---
  mm/memory-failure.c | 1 -
  1 file changed, 1 deletion(-)
Quoted 17 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1324

Quoted 12 lines
mm/memory-failure.c +0 -1
index 392ac277b17d..9784f4339ae7 100644
@@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
  		}
  		unlock_page(p);
  		VM_BUG_ON_PAGE(!page_count(p), p);
-		hpage = compound_head(p);
  	}
  	/*
 

I am *absolutely* no transparent huge page expert (sorry :) ), but won't the split_huge_page(p) eventually split the compound page, such that compound_head(p) will return something else after that call?

Hi, David

Took sometime to look into the code and re-think about it. Found maybe we can simplify this in another way.

First, code touches here means split_huge_page() succeeds and "p" is now a PTE page. So compound_head(p) == p.

Then let's look at who will use hpage in the following function. There are two uses in current upstream:

* page_flags calculation
* hwpoison_user_mappings()

The first one would be removed in next patch since PageHuge is handled at the beginning.

And in the second place, comment says if split succeeds, hpage points to page "p".

After all, we don't need to re-calculate hpage after split, and just replace hpage in hwpoison_user_mappings() with p is enough.

Quoted 5 lines
Signature · 3 lines
Thanks,

David / dhildenb
Signature · 2 lines
Wei Yang
Help you, Help me
#5 lore raw

Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage

Quoted 27 lines · Wei Yang
Quoted 8 lines · David Hildenbrand
Quoted 7 lines · Wei Yang

hpage is not changed.

---
  mm/memory-failure.c | 1 -
  1 file changed, 1 deletion(-)
Quoted 17 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1324

Quoted 12 lines
mm/memory-failure.c +0 -1
index 392ac277b17d..9784f4339ae7 100644
@@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
  		}
  		unlock_page(p);
  		VM_BUG_ON_PAGE(!page_count(p), p);
-		hpage = compound_head(p);
  	}
  	/*
 

I am *absolutely* no transparent huge page expert (sorry :) ), but won't the split_huge_page(p) eventually split the compound page, such that compound_head(p) will return something else after that call?

Quoted 7 lines

Hi, David

Took sometime to look into the code and re-think about it. Found maybe we can simplify this in another way.

First, code touches here means split_huge_page() succeeds and "p" is now a PTE page. So compound_head(p) == p.

While this would also be my intuition, I can't state that this is guaranteed to be the case (IOW, I did not check the code/documentation) :)

Quoted 7 lines

Then let's look at who will use hpage in the following function. There are two uses in current upstream:

* page_flags calculation
* hwpoison_user_mappings()

Quoted 8 lines

The first one would be removed in next patch since PageHuge is handled at the beginning.

And in the second place, comment says if split succeeds, hpage points to page "p".

After all, we don't need to re-calculate hpage after split, and just replace hpage in hwpoison_user_mappings() with p is enough.

That assumption would only be true in case all compound pages at this point are transparent huge pages, no? AFAIK that is not necessarily true. Or am I missing something?

Signature · 3 lines
Thanks,

David / dhildenb
author
#6 lore raw

Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage

Quoted 30 lines · David Hildenbrand
Quoted 29 lines · Wei Yang
Quoted 8 lines · David Hildenbrand
Quoted 7 lines · Wei Yang

hpage is not changed.

---
  mm/memory-failure.c | 1 -
  1 file changed, 1 deletion(-)
Quoted 17 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1324

Quoted 12 lines
mm/memory-failure.c +0 -1
index 392ac277b17d..9784f4339ae7 100644
@@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
  		}
  		unlock_page(p);
  		VM_BUG_ON_PAGE(!page_count(p), p);
-		hpage = compound_head(p);
  	}
  	/*
 

I am *absolutely* no transparent huge page expert (sorry :) ), but won't the split_huge_page(p) eventually split the compound page, such that compound_head(p) will return something else after that call?

Hi, David

Quoted 9 lines
Quoted 5 lines

Took sometime to look into the code and re-think about it. Found maybe we can simplify this in another way.

First, code touches here means split_huge_page() succeeds and "p" is now a PTE page. So compound_head(p) == p.

While this would also be my intuition, I can't state that this is guaranteed to be the case (IOW, I did not check the code/documentation) :)

If my understanding is correct, split_huge_page() succeeds the THP would be tear down to normal page.

Quoted 13 lines
Quoted 13 lines

Then let's look at who will use hpage in the following function. There are two uses in current upstream:

* page_flags calculation
* hwpoison_user_mappings()

The first one would be removed in next patch since PageHuge is handled at the beginning.

And in the second place, comment says if split succeeds, hpage points to page "p".

Quoted 7 lines
Quoted 2 lines

After all, we don't need to re-calculate hpage after split, and just replace hpage in hwpoison_user_mappings() with p is enough.

That assumption would only be true in case all compound pages at this point are transparent huge pages, no? AFAIK that is not necessarily true. Or am I missing something?

Function hwpoison_user_mappings() just handle user space mapping. If my understanding is correct, we just have three type of pages would be used in user space mapping:

* normal page
* THP
* hugetlb

Since THP would be split or already returned and hugetlb is handled in another branch, this means for other pages hwpoison_user_mappings() would just return true.

Quoted 5 lines
Signature · 4 lines
-- 
Thanks,

David / dhildenb
Signature · 2 lines
Wei Yang
Help you, Help me
#7 lore raw

Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage

Quoted 69 lines · Wei Yang
Quoted 30 lines · David Hildenbrand
Quoted 29 lines · Wei Yang
Quoted 8 lines · David Hildenbrand
Quoted 7 lines · Wei Yang

hpage is not changed.

---
  mm/memory-failure.c | 1 -
  1 file changed, 1 deletion(-)
Quoted 17 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1324

Quoted 12 lines
mm/memory-failure.c +0 -1
index 392ac277b17d..9784f4339ae7 100644
@@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
  		}
  		unlock_page(p);
  		VM_BUG_ON_PAGE(!page_count(p), p);
-		hpage = compound_head(p);
  	}
  	/*
 

I am *absolutely* no transparent huge page expert (sorry :) ), but won't the split_huge_page(p) eventually split the compound page, such that compound_head(p) will return something else after that call?

Hi, David

Quoted 9 lines
Quoted 5 lines

Took sometime to look into the code and re-think about it. Found maybe we can simplify this in another way.

First, code touches here means split_huge_page() succeeds and "p" is now a PTE page. So compound_head(p) == p.

While this would also be my intuition, I can't state that this is guaranteed to be the case (IOW, I did not check the code/documentation) :)

If my understanding is correct, split_huge_page() succeeds the THP would be tear down to normal page.

Quoted 13 lines
Quoted 13 lines

Then let's look at who will use hpage in the following function. There are two uses in current upstream:

* page_flags calculation
* hwpoison_user_mappings()

The first one would be removed in next patch since PageHuge is handled at the beginning.

And in the second place, comment says if split succeeds, hpage points to page "p".

Quoted 7 lines
Quoted 2 lines

After all, we don't need to re-calculate hpage after split, and just replace hpage in hwpoison_user_mappings() with p is enough.

That assumption would only be true in case all compound pages at this point are transparent huge pages, no? AFAIK that is not necessarily true. Or am I missing something?

Function hwpoison_user_mappings() just handle user space mapping. If my understanding is correct, we just have three type of pages would be used in user space mapping:

Quoted 8 lines

* normal page
* THP
* hugetlb

Since THP would be split or already returned and hugetlb is handled in another branch, this means for other pages hwpoison_user_mappings() would just return true.

Sorry for the late reply :)

While I think you are correct, I am not sure if the change you are suggesting is a) future proof and b) worth it. IOW, the recalculation after the split makes it clear that something changed and that the compound page does no longer exist. I might be wrong of course and this cleanup makes perfect sense :)

Signature · 3 lines
Thanks,

David / dhildenb
author
#8 lore raw

Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage

Quoted 79 lines · David Hildenbrand
Quoted 69 lines · Wei Yang
Quoted 30 lines · David Hildenbrand
Quoted 29 lines · Wei Yang
Quoted 8 lines · David Hildenbrand
Quoted 7 lines · Wei Yang

hpage is not changed.

---
  mm/memory-failure.c | 1 -
  1 file changed, 1 deletion(-)
Quoted 17 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1324

Quoted 12 lines
mm/memory-failure.c +0 -1
index 392ac277b17d..9784f4339ae7 100644
@@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
  		}
  		unlock_page(p);
  		VM_BUG_ON_PAGE(!page_count(p), p);
-		hpage = compound_head(p);
  	}
  	/*
 

I am *absolutely* no transparent huge page expert (sorry :) ), but won't the split_huge_page(p) eventually split the compound page, such that compound_head(p) will return something else after that call?

Hi, David

Quoted 9 lines
Quoted 5 lines

Took sometime to look into the code and re-think about it. Found maybe we can simplify this in another way.

First, code touches here means split_huge_page() succeeds and "p" is now a PTE page. So compound_head(p) == p.

While this would also be my intuition, I can't state that this is guaranteed to be the case (IOW, I did not check the code/documentation) :)

If my understanding is correct, split_huge_page() succeeds the THP would be tear down to normal page.

Quoted 13 lines
Quoted 13 lines

Then let's look at who will use hpage in the following function. There are two uses in current upstream:

* page_flags calculation
* hwpoison_user_mappings()

The first one would be removed in next patch since PageHuge is handled at the beginning.

And in the second place, comment says if split succeeds, hpage points to page "p".

Quoted 7 lines
Quoted 2 lines

After all, we don't need to re-calculate hpage after split, and just replace hpage in hwpoison_user_mappings() with p is enough.

That assumption would only be true in case all compound pages at this point are transparent huge pages, no? AFAIK that is not necessarily true. Or am I missing something?

Function hwpoison_user_mappings() just handle user space mapping. If my understanding is correct, we just have three type of pages would be used in user space mapping:

Quoted 8 lines

* normal page
* THP
* hugetlb

Since THP would be split or already returned and hugetlb is handled in another branch, this means for other pages hwpoison_user_mappings() would just return true.

Quoted 8 lines

Sorry for the late reply :)

While I think you are correct, I am not sure if the change you are suggesting is a) future proof and b) worth it. IOW, the recalculation after the split makes it clear that something changed and that the compound page does no longer exist. I might be wrong of course and this cleanup makes perfect sense :)

Yep, you are welcome.

I would think about the whole picture again.

Quoted 5 lines
Signature · 4 lines
-- 
Thanks,

David / dhildenb
Signature · 2 lines
Wei Yang
Help you, Help me
#9 lore raw
In reply to Wei Yang
Quoted 15 lines · Wei Yang

PageHuge is handled by memory_failure_hugetlb(), so this case could be removed.

---
  mm/memory-failure.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)
mm/memory-failure.c +0 -0
index 3151c87dff73..392ac277b17d 100644
@@ -1359,10 +1359,7 @@ int memory_failure(unsigned long pfn, int flags)
  	 * page_remove_rmap() in try_to_unmap_one(). So to determine page status

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1365

Quoted 10 lines
mm/memory-failure.c +1 -4
  	 * correctly, we save a copy of the page flags at this time.
  	 */
-	if (PageHuge(p))
-		page_flags = hpage->flags;
-	else
-		page_flags = p->flags;
+	page_flags = p->flags;
 
  	/*
  	 * unpoison always clear PG_hwpoison inside page lock

I somewhat miss a proper explanation why this is safe to do. We access page flags here, so why is it safe to refer to the ones of the sub-page?

Signature · 3 lines
Thanks,

David / dhildenb
author
#10 lore raw
Quoted 20 lines · David Hildenbrand
Quoted 19 lines · Wei Yang

PageHuge is handled by memory_failure_hugetlb(), so this case could be removed.

---
  mm/memory-failure.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)
mm/memory-failure.c +0 -3
index 3151c87dff73..392ac277b17d 100644
@@ -1359,10 +1359,7 @@ int memory_failure(unsigned long pfn, int flags)
  	 * page_remove_rmap() in try_to_unmap_one(). So to determine page status
  	 * correctly, we save a copy of the page flags at this time.
  	 */
-	if (PageHuge(p))
-		page_flags = hpage->flags;
-	else
Quoted 9 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags)

Quoted 5 lines
mm/memory-failure.c +1 -1
-		page_flags = p->flags;
+	page_flags = p->flags;
  	/*
  	 * unpoison always clear PG_hwpoison inside page lock
 

I somewhat miss a proper explanation why this is safe to do. We access page flags here, so why is it safe to refer to the ones of the sub-page?

Hi, David

I think your comment is on this line:

	page_flags = p->flags;

Maybe we need to use this:

	page_flags = hpage->flags;

And use hpage in the following or even the whole function?

While one thing interesting is not all "compound page" is PageCompound. For some sub-page, we can't get the correct head. This means we may just check on the sub-page.

Quoted 5 lines
Signature · 3 lines
Thanks,

David / dhildenb
Signature · 2 lines
Wei Yang
Help you, Help me
Reviewed-by
#11 lore raw
Quoted 39 lines · Wei Yang
Quoted 20 lines · David Hildenbrand
Quoted 19 lines · Wei Yang

PageHuge is handled by memory_failure_hugetlb(), so this case could be removed.

---
   mm/memory-failure.c | 5 +----
   1 file changed, 1 insertion(+), 4 deletions(-)
mm/memory-failure.c +0 -3
index 3151c87dff73..392ac277b17d 100644
@@ -1359,10 +1359,7 @@ int memory_failure(unsigned long pfn, int flags)
   	 * page_remove_rmap() in try_to_unmap_one(). So to determine page status
   	 * correctly, we save a copy of the page flags at this time.
   	 */
-	if (PageHuge(p))
-		page_flags = hpage->flags;
-	else
Quoted 9 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags)

Quoted 5 lines
mm/memory-failure.c +1 -1
-		page_flags = p->flags;
+	page_flags = p->flags;
   	/*
   	 * unpoison always clear PG_hwpoison inside page lock
 

I somewhat miss a proper explanation why this is safe to do. We access page flags here, so why is it safe to refer to the ones of the sub-page?

Hi, David

I think your comment is on this line:

	page_flags = p->flags;

Maybe we need to use this:

Quoted 7 lines
	page_flags = hpage->flags;

And use hpage in the following or even the whole function?

While one thing interesting is not all "compound page" is PageCompound. For some sub-page, we can't get the correct head. This means we may just check on the sub-page.

Oh wait, I think I missed the check right at the beginning of this function, sorry :/

Sooo, memory_failure_hugetlb() was introduced by

commit 761ad8d7c7b5485bb66fd5bccb58a891fe784544

Author: Naoya Horiguchi <[email protected]>
Date:   Mon Jul 10 15:47:47 2017 -0700

mm: hwpoison: introduce memory_failure_hugetlb()

and essentially ripped out all PageHuge() checks *except* this check. This check was introduced in

commit 7258ae5c5a2ce2f5969e8b18b881be40ab55433d

Author: James Morse <[email protected]>
Date:   Fri Jun 16 14:02:29 2017 -0700

mm/memory-failure.c: use compound_head() flags for huge pages

Maybe that was just a merge oddity as both commits are only ~1month apart. IOW, I think Naoya's patch forgot to rip it out.

Can we make this clear in the patch description like "This is dead code that cannot be reached after commit 761ad8d7c7b5 ("mm: hwpoison: introduce memory_failure_hugetlb()")"

I assume Andrew can fix up when applying

Signature · 3 lines
Thanks,

David / dhildenb
author
#12 lore raw
Quoted 73 lines · David Hildenbrand
Quoted 39 lines · Wei Yang
Quoted 20 lines · David Hildenbrand
Quoted 19 lines · Wei Yang

PageHuge is handled by memory_failure_hugetlb(), so this case could be removed.

---
   mm/memory-failure.c | 5 +----
   1 file changed, 1 insertion(+), 4 deletions(-)
mm/memory-failure.c +0 -3
index 3151c87dff73..392ac277b17d 100644
@@ -1359,10 +1359,7 @@ int memory_failure(unsigned long pfn, int flags)
   	 * page_remove_rmap() in try_to_unmap_one(). So to determine page status
   	 * correctly, we save a copy of the page flags at this time.
   	 */
-	if (PageHuge(p))
-		page_flags = hpage->flags;
-	else
Quoted 9 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags)

Quoted 5 lines
mm/memory-failure.c +1 -1
-		page_flags = p->flags;
+	page_flags = p->flags;
   	/*
   	 * unpoison always clear PG_hwpoison inside page lock
 

I somewhat miss a proper explanation why this is safe to do. We access page flags here, so why is it safe to refer to the ones of the sub-page?

Hi, David

I think your comment is on this line:

	page_flags = p->flags;

Maybe we need to use this:

Quoted 7 lines
	page_flags = hpage->flags;

And use hpage in the following or even the whole function?

While one thing interesting is not all "compound page" is PageCompound. For some sub-page, we can't get the correct head. This means we may just check on the sub-page.

Oh wait, I think I missed the check right at the beginning of this function, sorry :/

Sooo, memory_failure_hugetlb() was introduced by

commit 761ad8d7c7b5485bb66fd5bccb58a891fe784544

Author: Naoya Horiguchi <[email protected]>
Date:   Mon Jul 10 15:47:47 2017 -0700

mm: hwpoison: introduce memory_failure_hugetlb()

and essentially ripped out all PageHuge() checks *except* this check. This check was introduced in

commit 7258ae5c5a2ce2f5969e8b18b881be40ab55433d

Author: James Morse <[email protected]>
Date:   Fri Jun 16 14:02:29 2017 -0700

mm/memory-failure.c: use compound_head() flags for huge pages

Maybe that was just a merge oddity as both commits are only ~1month apart. IOW, I think Naoya's patch forgot to rip it out.

Quoted 6 lines

Can we make this clear in the patch description like "This is dead code that cannot be reached after commit 761ad8d7c7b5 ("mm: hwpoison: introduce memory_failure_hugetlb()")"

I assume Andrew can fix up when applying

That's very helpful. Thanks for your time sincerely.

Quoted 7 lines
Signature · 3 lines
Thanks,

David / dhildenb
Signature · 2 lines
Wei Yang
Help you, Help me