THP: broken page count after commit aa88b68c

BUG/REGRESSION Gerald Schaefer 5 participants 10 messages Jun 2, 2016 – Jun 3, 2016 1 message not in the archive

Subsystems (2)

Memory management MM swap

lore

Share & tools

lore

Gerald Schaefer

author

2016-06-02 15:21 UTC #0 lore raw

Christian Borntraeger reported a kernel panic after corrupt page counts, and it turned out to be a regression introduced with commit aa88b68c "thp: keep huge zero page pinned until tlb flush", at least on s390.

put_huge_zero_page() was moved over from zap_huge_pmd() to release_pages(), and it was replaced by tlb_remove_page(). However, release_pages() might not always be triggered by (the arch-specific) tlb_remove_page().

On s390 we call free_page_and_swap_cache() from tlb_remove_page(), and not tlb_flush_mmu() -> free_pages_and_swap_cache() like the generic version, because we don't use the MMU-gather logic. Although both functions have very similar names, they are doing very unsimilar things, in particular free_page_xxx is just doing a put_page(), while free_pages_xxx calls release_pages().

This of course results in very harmful put_page()s on the huge zero page, on architectures where tlb_remove_page() is implemented in this way. It seems to affect only s390 and sh, but sh doesn't have THP support, so the problem (currently) probably only exists on s390.

The following quick hack fixed the issue:

mm/swap_state.c +4 -1

index 0d457e7..c99463a 100644
@@ -252,7 +252,10 @@ static inline void free_swap_cache(struct page *page)
 void free_page_and_swap_cache(struct page *page)
 {
 	free_swap_cache(page);
-	put_page(page);
+	if (is_huge_zero_page(page))
+		put_huge_zero_page();
+	else
+		put_page(page);
 }
 
 /*

But of course there might be a better solution, and there still are some questions left:
- Why does free_page_xxx() behave so differently from free_pages_xxx()?
- Would it be OK to implement free_page_xxx() by calling free_pages_xxx() with nr = 1, similar to free_page() vs. free_pages()?
- Would it be OK to replace the put_page() in free_page_xxx() with a call to release_pages() with nr = 1?
- Would it be better to fix this in the arch-specific tlb_remove_page(), by calling free_pages_xxx() with nr = 1 instead of free_page_xxx()?

Regards,
Gerald

--
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>

Kirill A. Shutemov

2016-06-02 15:51 UTC #1 lore raw

Quoted 28 lines · Gerald Schaefer

Christian Borntraeger reported a kernel panic after corrupt page counts, and it turned out to be a regression introduced with commit aa88b68c "thp: keep huge zero page pinned until tlb flush", at least on s390.
put_huge_zero_page() was moved over from zap_huge_pmd() to release_pages(), and it was replaced by tlb_remove_page(). However, release_pages() might not always be triggered by (the arch-specific) tlb_remove_page().
On s390 we call free_page_and_swap_cache() from tlb_remove_page(), and not tlb_flush_mmu() -> free_pages_and_swap_cache() like the generic version, because we don't use the MMU-gather logic. Although both functions have very similar names, they are doing very unsimilar things, in particular free_page_xxx is just doing a put_page(), while free_pages_xxx calls release_pages().
This of course results in very harmful put_page()s on the huge zero page, on architectures where tlb_remove_page() is implemented in this way. It seems to affect only s390 and sh, but sh doesn't have THP support, so the problem (currently) probably only exists on s390.
The following quick hack fixed the issue:
mm/swap_state.c +0 -0
index 0d457e7..c99463a 100644
@@ -252,7 +252,10 @@ static inline void free_swap_cache(struct page *page)
 void free_page_and_swap_cache(struct page *page)

mm/swap_state.c static inline void free_swap_cache(struct page *page) · line 261

Quoted 10 lines

mm/swap_state.c +4 -1

 {
 	free_swap_cache(page);
-	put_page(page);
+	if (is_huge_zero_page(page))
+		put_huge_zero_page();
+	else
+		put_page(page);
 }
 
 /*

The fix looks good to me.

Quoted 3 lines

But of course there might be a better solution, and there still are some questions left:
- Why does free_page_xxx() behave so differently from free_pages_xxx()?

I don't see it behave too deiferently. It just try to batch freeing to lower locking overhead.

Quoted 4 lines

- Would it be OK to implement free_page_xxx() by calling free_pages_xxx() with nr = 1, similar to free_page() vs. free_pages()?
- Would it be OK to replace the put_page() in free_page_xxx() with a call to release_pages() with nr = 1?

release_pages() somewhat suboptimal for nr=1. I guess we can fix this with shortcut to put_page() at start of release_page() if nr == 1.

Quoted 11 lines

- Would it be better to fix this in the arch-specific tlb_remove_page(), by calling free_pages_xxx() with nr = 1 instead of free_page_xxx()?
Regards,
Gerald
--
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>

Signature · 7 lines

 Kirill A. Shutemov

--
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>

Andrew Morton

maintainer

2016-06-02 18:40 UTC #2 lore raw

Quoted 38 lines · On Thu, 2 Jun 2016 18:51:50 +0300 "Kirill A. Shutemov" <[email protected]>…

Quoted 37 lines · Gerald Schaefer
Christian Borntraeger reported a kernel panic after corrupt page counts, and it turned out to be a regression introduced with commit aa88b68c "thp: keep huge zero page pinned until tlb flush", at least on s390.
put_huge_zero_page() was moved over from zap_huge_pmd() to release_pages(), and it was replaced by tlb_remove_page(). However, release_pages() might not always be triggered by (the arch-specific) tlb_remove_page().
On s390 we call free_page_and_swap_cache() from tlb_remove_page(), and not tlb_flush_mmu() -> free_pages_and_swap_cache() like the generic version, because we don't use the MMU-gather logic. Although both functions have very similar names, they are doing very unsimilar things, in particular free_page_xxx is just doing a put_page(), while free_pages_xxx calls release_pages().
This of course results in very harmful put_page()s on the huge zero page, on architectures where tlb_remove_page() is implemented in this way. It seems to affect only s390 and sh, but sh doesn't have THP support, so the problem (currently) probably only exists on s390.
The following quick hack fixed the issue:
mm/swap_state.c +4 -1
index 0d457e7..c99463a 100644
@@ -252,7 +252,10 @@ static inline void free_swap_cache(struct page *page)
 void free_page_and_swap_cache(struct page *page)
 {
 	free_swap_cache(page);
-	put_page(page);
+	if (is_huge_zero_page(page))
+		put_huge_zero_page();
+	else
+		put_page(page);
 }
 

Quoted 3 lines

mm/swap_state.c static inline void free_swap_cache(struct page *page) · line 261
Quoted 1 line
mm/swap_state.c +0 -0
 /*
The fix looks good to me.

Yes. A bit regrettable, but that's what release_pages() does.

Can we have a signed-off-by please?

Christian Borntraeger

2016-06-02 18:56 UTC #3 lore raw

Quoted 40 lines · Andrew Morton

Quoted 38 lines · On Thu, 2 Jun 2016 18:51:50 +0300 "Kirill A. Shutemov" <[email protected]>…
Quoted 37 lines · Gerald Schaefer
Christian Borntraeger reported a kernel panic after corrupt page counts, and it turned out to be a regression introduced with commit aa88b68c "thp: keep huge zero page pinned until tlb flush", at least on s390.
put_huge_zero_page() was moved over from zap_huge_pmd() to release_pages(), and it was replaced by tlb_remove_page(). However, release_pages() might not always be triggered by (the arch-specific) tlb_remove_page().
On s390 we call free_page_and_swap_cache() from tlb_remove_page(), and not tlb_flush_mmu() -> free_pages_and_swap_cache() like the generic version, because we don't use the MMU-gather logic. Although both functions have very similar names, they are doing very unsimilar things, in particular free_page_xxx is just doing a put_page(), while free_pages_xxx calls release_pages().
This of course results in very harmful put_page()s on the huge zero page, on architectures where tlb_remove_page() is implemented in this way. It seems to affect only s390 and sh, but sh doesn't have THP support, so the problem (currently) probably only exists on s390.
The following quick hack fixed the issue:
mm/swap_state.c +4 -1
index 0d457e7..c99463a 100644
@@ -252,7 +252,10 @@ static inline void free_swap_cache(struct page *page)
 void free_page_and_swap_cache(struct page *page)
 {
 	free_swap_cache(page);
-	put_page(page);
+	if (is_huge_zero_page(page))
+		put_huge_zero_page();
+	else
+		put_page(page);
 }
 

Quoted 7 lines

Quoted 3 lines
mm/swap_state.c static inline void free_swap_cache(struct page *page) · line 261
Quoted 1 line
mm/swap_state.c +0 -0
 /*
The fix looks good to me.
Yes. A bit regrettable, but that's what release_pages() does.
Can we have a signed-off-by please?

Please also add CC: stable for 4.6

Christian Borntraeger

2016-06-02 18:56 UTC #4 lore raw

In reply to Andrew Morton

Quoted 40 lines · Andrew Morton

Quoted 38 lines · On Thu, 2 Jun 2016 18:51:50 +0300 "Kirill A. Shutemov" <[email protected]>…
Quoted 37 lines · Gerald Schaefer
Christian Borntraeger reported a kernel panic after corrupt page counts, and it turned out to be a regression introduced with commit aa88b68c "thp: keep huge zero page pinned until tlb flush", at least on s390.
put_huge_zero_page() was moved over from zap_huge_pmd() to release_pages(), and it was replaced by tlb_remove_page(). However, release_pages() might not always be triggered by (the arch-specific) tlb_remove_page().
On s390 we call free_page_and_swap_cache() from tlb_remove_page(), and not tlb_flush_mmu() -> free_pages_and_swap_cache() like the generic version, because we don't use the MMU-gather logic. Although both functions have very similar names, they are doing very unsimilar things, in particular free_page_xxx is just doing a put_page(), while free_pages_xxx calls release_pages().
This of course results in very harmful put_page()s on the huge zero page, on architectures where tlb_remove_page() is implemented in this way. It seems to affect only s390 and sh, but sh doesn't have THP support, so the problem (currently) probably only exists on s390.
The following quick hack fixed the issue:
mm/swap_state.c +4 -1
index 0d457e7..c99463a 100644
@@ -252,7 +252,10 @@ static inline void free_swap_cache(struct page *page)
 void free_page_and_swap_cache(struct page *page)
 {
 	free_swap_cache(page);
-	put_page(page);
+	if (is_huge_zero_page(page))
+		put_huge_zero_page();
+	else
+		put_page(page);
 }
 

Quoted 7 lines

Quoted 3 lines
mm/swap_state.c static inline void free_swap_cache(struct page *page) · line 261
Quoted 1 line
mm/swap_state.c +0 -0
 /*
The fix looks good to me.
Yes. A bit regrettable, but that's what release_pages() does.
Can we have a signed-off-by please?

Please also add CC: stable for 4.6

A message that is not in the archive (sent off-list or not yet received): [email protected]

Andrew Morton

maintainer

2016-06-02 19:03 UTC #6 lore raw

Quoted 7 lines · On Thu, 2 Jun 2016 20:56:27 +0200 Christian Borntraeger…

Quoted 5 lines
Quoted 1 line
The fix looks good to me.
Yes. A bit regrettable, but that's what release_pages() does.
Can we have a signed-off-by please?
Please also add CC: stable for 4.6

I shall take that as a "yes" and I'll add

Signed-off-by Christian Borntraeger <[email protected]>

to the changelog.

Christian Borntraeger

Tested-by

2016-06-02 19:10 UTC #7 lore raw

Quoted 8 lines · Andrew Morton

Quoted 6 lines · On Thu, 2 Jun 2016 20:56:27 +0200 Christian Borntraeger…
Quoted 5 lines
Quoted 1 line
The fix looks good to me.
Yes. A bit regrettable, but that's what release_pages() does.
Can we have a signed-off-by please?

Quoted 7 lines

Quoted 1 line
Please also add CC: stable for 4.6
I shall take that as a "yes" and I'll add
Signed-off-by Christian Borntraeger <[email protected]>
to the changelog.

Gerald has created the patch,
but you could add

Reported-by Christian Borntraeger <[email protected]>

Tested-by Christian Borntraeger <[email protected]>

Christian Borntraeger

Tested-by

2016-06-02 19:11 UTC #8 lore raw

In reply to Andrew Morton

Quoted 8 lines · Andrew Morton

Quoted 6 lines · On Thu, 2 Jun 2016 20:56:27 +0200 Christian Borntraeger…
Quoted 5 lines
Quoted 1 line
The fix looks good to me.
Yes. A bit regrettable, but that's what release_pages() does.
Can we have a signed-off-by please?

Quoted 7 lines

Quoted 1 line
Please also add CC: stable for 4.6
I shall take that as a "yes" and I'll add
Signed-off-by Christian Borntraeger <[email protected]>
to the changelog.

Gerald has created the patch,
but you could add

Reported-by Christian Borntraeger <[email protected]>

Tested-by Christian Borntraeger <[email protected]>

Hugh Dickins

2016-06-02 19:48 UTC #9 lore raw

In reply to Kirill A. Shutemov

Quoted 19 lines · Kirill A. Shutemov

Quoted 18 lines · Gerald Schaefer

The following quick hack fixed the issue:

mm/swap_state.c +4 -1

index 0d457e7..c99463a 100644
@@ -252,7 +252,10 @@ static inline void free_swap_cache(struct page *page)
 void free_page_and_swap_cache(struct page *page)
 {
 	free_swap_cache(page);
-	put_page(page);
+	if (is_huge_zero_page(page))
+		put_huge_zero_page();
+	else
+		put_page(page);
 }

Quoted 3 lines

mm/swap_state.c static inline void free_swap_cache(struct page *page) · line 261
Quoted 1 line
mm/swap_state.c +0 -0
 /*
The fix looks good to me.

Is there a good reason why the refcount of the huge_zero_page is huge_zero_refcount, instead of the refcount of the huge_zero_page? Wouldn't the latter avoid such is_huge_zero_page() special-casing?

Hugh

Kirill A. Shutemov

2016-06-03 10:37 UTC #10 lore raw

Quoted 20 lines · Hugh Dickins

Quoted 19 lines · Kirill A. Shutemov

Quoted 18 lines · Gerald Schaefer

The following quick hack fixed the issue:

mm/swap_state.c +4 -1

index 0d457e7..c99463a 100644
@@ -252,7 +252,10 @@ static inline void free_swap_cache(struct page *page)
 void free_page_and_swap_cache(struct page *page)
 {
 	free_swap_cache(page);
-	put_page(page);
+	if (is_huge_zero_page(page))
+		put_huge_zero_page();
+	else
+		put_page(page);
 }

Quoted 7 lines

Quoted 3 lines
mm/swap_state.c static inline void free_swap_cache(struct page *page) · line 261
Quoted 1 line
mm/swap_state.c +0 -0
 /*
The fix looks good to me.
Is there a good reason why the refcount of the huge_zero_page is huge_zero_refcount, instead of the refcount of the huge_zero_page? Wouldn't the latter avoid such is_huge_zero_page() special-casing?

Hm. I thought I had a reason for not using page's refcount, but I can't find any now. We would loose sanity check in put_huge_zero_page(), but I guess it's fine since we never triggered it.

I'll put it to my todo list.

Signature · 7 lines

 Kirill A. Shutemov

--
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>