Skip to contentSkip to conversation

mm/vmscan: drop checking if _deferred_list is empty before using TTU_SYNC

PATCH Applied Barry Song 4 participants 5 messages Jun 29, 2024 – Jul 2, 2024

Subsystems (3)
Memory management MGLRU MM reclaim
lore
Share & tools
lore

Barry Song

author
#0 lore raw

PATCH 1/1 mm/vmscan: drop checking if _deferred_list is empty before using TTU_SYNC

From: Barry Song <[email protected]>

The optimization of list_empty(&folio->_deferred_list) aimed to prevent increasing the PTL duration when a large folio is partially unmapped, for example, from subpage 0 to subpage (nr - 2).

But Ryan's commit 5ed890ce5147 ("mm: vmscan: avoid split during shrink_folio_list()") actually splits this kind of large folios. This makes the "optimization" useless.

Signed-off-by Barry Song <[email protected]>
Cc David Hildenbrand <[email protected]>
Cc Ryan Roberts <[email protected]>

---
-v1
* I remember David and Ryan once suggested that this check could be dropped while the patch was being pulled into mm-unstable. However, for some reason, I forgot to request squashing this change.

 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/vmscan.c +1 -1
index 3d4c681c6d40..0761f91b407f 100644
@@ -1291,7 +1291,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 			 * try_to_unmap acquire PTL from the first PTE,
 			 * eliminating the influence of temporary PTE values.
 			 */
-			if (folio_test_large(folio) && list_empty(&folio->_deferred_list))
+			if (folio_test_large(folio))
 				flags |= TTU_SYNC;
 
 			try_to_unmap(folio, flags);
Signature · 1 line
2.34.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 6 lines · Barry Song

From: Barry Song <[email protected]>

The optimization of list_empty(&folio->_deferred_list) aimed to prevent increasing the PTL duration when a large folio is partially unmapped, for example, from subpage 0 to subpage (nr - 2).

Quoted 7 lines

But Ryan's commit 5ed890ce5147 ("mm: vmscan: avoid split during shrink_folio_list()") actually splits this kind of large folios. This makes the "optimization" useless.

Signed-off-by Barry Song <[email protected]>
Cc David Hildenbrand <[email protected]>
Cc Ryan Roberts <[email protected]>
Reviewed-by Ryan Roberts <[email protected]>
Quoted 14 lines

---
-v1
* I remember David and Ryan once suggested that this check could be dropped while the patch was being pulled into mm-unstable. However, for some reason, I forgot to request squashing this change.

 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/vmscan.c +0 -0
index 3d4c681c6d40..0761f91b407f 100644

mm/vmscan.c static unsigned int shrink_folio_list(struct list_head *folio_list, · line 1294

Quoted 5 lines
mm/vmscan.c +1 -1
@@ -1291,7 +1291,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 			 * try_to_unmap acquire PTL from the first PTE,
 			 * eliminating the influence of temporary PTE values.
 			 */
-			if (folio_test_large(folio) && list_empty(&folio->_deferred_list))
+			if (folio_test_large(folio))

As it stands, the list_empty() technically needs a data_race() annotation. I think your original patch went into v6.10-rc1? If so, perhaps it makes sense to try to get this into the next rc?

Quoted 3 lines
mm/vmscan.c +0 -0
 				flags |= TTU_SYNC;
 
 			try_to_unmap(folio, flags);
author
#2 lore raw
Quoted 27 lines · Ryan Roberts
Quoted 6 lines · Barry Song

From: Barry Song <[email protected]>

The optimization of list_empty(&folio->_deferred_list) aimed to prevent increasing the PTL duration when a large folio is partially unmapped, for example, from subpage 0 to subpage (nr - 2).

Quoted 7 lines

But Ryan's commit 5ed890ce5147 ("mm: vmscan: avoid split during shrink_folio_list()") actually splits this kind of large folios. This makes the "optimization" useless.

Signed-off-by Barry Song <[email protected]>
Cc David Hildenbrand <[email protected]>
Cc Ryan Roberts <[email protected]>
Reviewed-by Ryan Roberts <[email protected]>
Quoted 9 lines

---
-v1
* I remember David and Ryan once suggested that this check could be dropped while the patch was being pulled into mm-unstable. However, for some reason, I forgot to request squashing this change.

 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 14 lines

mm/vmscan.c static unsigned int shrink_folio_list(struct list_head *folio_list, · line 1294

Quoted 10 lines
mm/vmscan.c +1 -1
index 3d4c681c6d40..0761f91b407f 100644
@@ -1291,7 +1291,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
                       * try_to_unmap acquire PTL from the first PTE,
                       * eliminating the influence of temporary PTE values.
                       */
-                     if (folio_test_large(folio) && list_empty(&folio->_deferred_list))
+                     if (folio_test_large(folio))

As it stands, the list_empty() technically needs a data_race() annotation. I think your original patch went into v6.10-rc1? If so, perhaps it makes sense to try to get this into the next rc?

Either way is fine. Hi Andrew, if you include it for the next RC, could you please add this tag?

Fixes 73bc32875ee9 ("mm: hold PTL from the first PTE while reclaiming

a large folio")

And an additional changelog:

Additionally, the list_empty() technically requires a data_race() annotation.

Quoted 5 lines

mm/vmscan.c static unsigned int shrink_folio_list(struct list_head *folio_list, · line 1297

Quoted 4 lines
mm/vmscan.c +0 -0
                              flags |= TTU_SYNC;
 
                      try_to_unmap(folio, flags);

Thanks
Barry

maintainer
#3 lore raw
Quoted 12 lines · On Mon, 1 Jul 2024 21:35:41 +1200 Barry Song <[email protected]> wrote:
Quoted 11 lines

mm/vmscan.c static unsigned int shrink_folio_list(struct list_head *folio_list, · line 1294

Quoted 7 lines

+++ b/mm/vmscan.c

mm/vmscan.c +1 -1
@@ -1291,7 +1291,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
                       * try_to_unmap acquire PTL from the first PTE,
                       * eliminating the influence of temporary PTE values.
                       */
-                     if (folio_test_large(folio) && list_empty(&folio->_deferred_list))
+                     if (folio_test_large(folio))

As it stands, the list_empty() technically needs a data_race() annotation. I think your original patch went into v6.10-rc1? If so, perhaps it makes sense to try to get this into the next rc?

Quoted 5 lines

Either way is fine. Hi Andrew, if you include it for the next RC, could you please add this tag?

Fixes 73bc32875ee9 ("mm: hold PTL from the first PTE while reclaiming

a large folio")

Nope! That invites people to backport this change into kernels which don't contain Ryan's 5ed890ce5147 ("mm: vmscan: avoid split during shrink_folio_list()").

I'll simply queue this in the normal fashion.

Quoted 3 lines

And an additional changelog:

Additionally, the list_empty() technically requires a data_race() annotation.

Done, thanks.

Reviewed-by on v1 1/1
#4 lore raw
In reply to Barry Song
Quoted 27 lines · Barry Song

From: Barry Song <[email protected]>

The optimization of list_empty(&folio->_deferred_list) aimed to prevent increasing the PTL duration when a large folio is partially unmapped, for example, from subpage 0 to subpage (nr - 2).

But Ryan's commit 5ed890ce5147 ("mm: vmscan: avoid split during shrink_folio_list()") actually splits this kind of large folios. This makes the "optimization" useless.

Signed-off-by Barry Song <[email protected]>
Cc David Hildenbrand <[email protected]>
Cc Ryan Roberts <[email protected]>

---
-v1
* I remember David and Ryan once suggested that this check could be dropped while the patch was being pulled into mm-unstable. However, for some reason, I forgot to request squashing this change.

  mm/vmscan.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
mm/vmscan.c +0 -0
index 3d4c681c6d40..0761f91b407f 100644

mm/vmscan.c static unsigned int shrink_folio_list(struct list_head *folio_list, · line 1297

Quoted 8 lines
mm/vmscan.c +1 -1
@@ -1291,7 +1291,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
  			 * try_to_unmap acquire PTL from the first PTE,
  			 * eliminating the influence of temporary PTE values.
  			 */
-			if (folio_test_large(folio) && list_empty(&folio->_deferred_list))
+			if (folio_test_large(folio))
  				flags |= TTU_SYNC;
 
  			try_to_unmap(folio, flags);
Reviewed-by David Hildenbrand <[email protected]>
Signature · 3 lines
Cheers,

David / dhildenb