Series comparison

-[RFC PATCH] mm: hold PTL from the first PTE while reclaiming a large folio
+[PATCH v2] mm: hold PTL from the first PTE while reclaiming a large folio
 From: Barry Song <v-songbaohua@oppo.com>
-page_vma_mapped_walk() within try_to_unmap_one() races with other
+Within try_to_unmap_one(), page_vma_mapped_walk() races with other
-PTEs modification such as break-before-make, while iterating PTEs
+PTE modifications preceded by pte clear. While iterating over PTEs
-of a large folio, it will only begin to acquire PTL after it gets
+of a large folio, it only starts acquiring PTL from the first valid
-a valid(present) PTE. break-before-make intermediately sets PTEs
+(present) PTE. PTE modifications can temporarily set PTEs to
-to pte_none. Thus, a large folio's PTEs might be partially skipped
+pte_none. Consequently, the initial PTEs of a large folio might
-in try_to_unmap_one().
+be skipped in try_to_unmap_one().
-For example, for an anon folio, after try_to_unmap_one(), we may
+For example, for an anon folio, if we skip PTE0, we may have PTE0
-have PTE0 present, while PTE1 ~ PTE(nr_pages - 1) are swap entries.
+which is still present, while PTE1 ~ PTE(nr_pages - 1) are swap
-So folio will be still mapped, the folio fails to be reclaimed.
+entries after try_to_unmap_one().
-What’s even more worrying is, its PTEs are no longer in a unified
+So folio will be still mapped, the folio fails to be reclaimed
-state. This might lead to accident folio_split() afterwards. And
+and is put back to LRU in this round.
-since a part of PTEs are now swap entries, accessing them will
+This also breaks up PTEs optimization such as CONT-PTE on this
-incur page fault - do_swap_page.
+large folio and may lead to accident folio_split() afterwards.
-It creates both anxiety and more expense. While we can't avoid
+And since a part of PTEs are now swap entries, accessing those
-userspace's unmap to break up unified PTEs such as CONT-PTE for
+parts will introduce overhead - do_swap_page.
-a large folio, we can indeed keep away from kernel's breaking up
+Although the kernel can withstand all of the above issues, the
-them due to its code design.
+situation still seems quite awkward and warrants making it more
-This patch is holding PTL from PTE0, thus, the folio will either
+ideal.
-be entirely reclaimed or entirely kept. On the other hand, this
+The same race also occurs with small folios, but they have only
-approach doesn't increase PTL contention. Even w/o the patch,
+one PTE, thus, it won't be possible for them to be partially
-page_vma_mapped_walk() will always get PTL after it sometimes
+unmapped.
-skips one or two PTEs because intermediate break-before-makes
+This patch holds PTL from PTE0, allowing us to avoid reading PTE
-are short, according to test. Of course, even w/o this patch,
+values that are in the process of being transformed. With stable
-the vast majority of try_to_unmap_one still can get PTL from
+PTE values, we can ensure that this large folio is either
-PTE0. This patch makes the number 100%.
+completely reclaimed or that all PTEs remain untouched in this
-The other option is that we can give up in try_to_unmap_one
+round.
-once we find PTE0 is not the first entry we get PTL, we call
+A corner case is that if we hold PTL from PTE0 and most initial
-page_vma_mapped_walk_done() to end the iteration at this case.
+PTEs have been really unmapped before that, we may increase the
-This will keep the unified PTEs while the folio isn't reclaimed.
+duration of holding PTL. Thus we only apply this optimization to
-The result is quite similar with small folios with one PTE -
+folios which are still entirely mapped (not in deferred_split
-either entirely reclaimed or entirely kept.
+list).
 Reclaiming large folios by holding PTL from PTE0 seems a better
 option comparing to giving up after detecting PTL begins from
 non-PTE0.
 Cc: Hugh Dickins <hughd@google.com>
 Signed-off-by: Barry Song <v-songbaohua@oppo.com>
 ---
- mm/vmscan.c | 11 +++++++++++
+ v2:
-file changed, 11 insertions(+)
+  * Refine commit message and code comment after reading all comments
     from Ryan and David, thanks!
   * Avoid increasing the duration of PTL by applying optimization
     on folios not in deferred_split_list with respect to Ying's
     comment, thanks!
  mm/vmscan.c | 12 ++++++++++++
 file changed, 12 insertions(+)
 diff --git a/mm/vmscan.c b/mm/vmscan.c
 index XXXXXXX..XXXXXXX 100644
 --- a/mm/vmscan.c
 +++ b/mm/vmscan.c
 @@ -XXX,XX +XXX,XX @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
              if (folio_test_pmd_mappable(folio))
                  flags |= TTU_SPLIT_HUGE_PMD;
 +            /*
-+             * if page table lock is not held from the first PTE of
++             * Without TTU_SYNC, try_to_unmap will only begin to hold PTL
-+             * a large folio, some PTEs might be skipped because of
++             * from the first present PTE within a large folio. Some initial
-+             * races with break-before-make, for example, PTEs can
++             * PTEs might be skipped due to races with parallel PTE writes
-+             * be pte_none intermediately, thus one or more PTEs
++             * in which PTEs can be cleared temporarily before being written
-+             * might be skipped in try_to_unmap_one, we might result
++             * new present values. This will lead to a large folio is still
-+             * in a large folio is partially mapped and partially
++             * mapped while some subpages have been partially unmapped after
-+             * unmapped after try_to_unmap
++             * try_to_unmap; TTU_SYNC helps try_to_unmap acquire PTL from the
 +             * first PTE, eliminating the influence of temporary PTE values.
 +             */
-+            if (folio_test_large(folio))
++            if (folio_test_large(folio) && list_empty(&folio->_deferred_list))
 +                flags |= TTU_SYNC;
              try_to_unmap(folio, flags);
              if (folio_mapped(folio)) {
 --
 .34.1

From: Barry Song <v-songbaohua@oppo.com>

page_vma_mapped_walk() within try_to_unmap_one() races with other
PTEs modification such as break-before-make, while iterating PTEs
of a large folio, it will only begin to acquire PTL after it gets
a valid(present) PTE. break-before-make intermediately sets PTEs
to pte_none. Thus, a large folio's PTEs might be partially skipped
in try_to_unmap_one().
For example, for an anon folio, after try_to_unmap_one(), we may
have PTE0 present, while PTE1 ~ PTE(nr_pages - 1) are swap entries.
So folio will be still mapped, the folio fails to be reclaimed.
What’s even more worrying is, its PTEs are no longer in a unified
state. This might lead to accident folio_split() afterwards. And
since a part of PTEs are now swap entries, accessing them will
incur page fault - do_swap_page.
It creates both anxiety and more expense. While we can't avoid
userspace's unmap to break up unified PTEs such as CONT-PTE for
a large folio, we can indeed keep away from kernel's breaking up
them due to its code design.
This patch is holding PTL from PTE0, thus, the folio will either
be entirely reclaimed or entirely kept. On the other hand, this
approach doesn't increase PTL contention. Even w/o the patch,
page_vma_mapped_walk() will always get PTL after it sometimes
skips one or two PTEs because intermediate break-before-makes
are short, according to test. Of course, even w/o this patch,
the vast majority of try_to_unmap_one still can get PTL from
PTE0. This patch makes the number 100%.
The other option is that we can give up in try_to_unmap_one
once we find PTE0 is not the first entry we get PTL, we call
page_vma_mapped_walk_done() to end the iteration at this case.
This will keep the unified PTEs while the folio isn't reclaimed.
The result is quite similar with small folios with one PTE -
either entirely reclaimed or entirely kept.
Reclaiming large folios by holding PTL from PTE0 seems a better
option comparing to giving up after detecting PTL begins from
non-PTE0.

Cc: Hugh Dickins <hughd@google.com>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
 mm/vmscan.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index XXXXXXX..XXXXXXX 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -XXX,XX +XXX,XX @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 
 			if (folio_test_pmd_mappable(folio))
 				flags |= TTU_SPLIT_HUGE_PMD;
+			/*
+			 * if page table lock is not held from the first PTE of
+			 * a large folio, some PTEs might be skipped because of
+			 * races with break-before-make, for example, PTEs can
+			 * be pte_none intermediately, thus one or more PTEs
+			 * might be skipped in try_to_unmap_one, we might result
+			 * in a large folio is partially mapped and partially
+			 * unmapped after try_to_unmap
+			 */
+			if (folio_test_large(folio))
+				flags |= TTU_SYNC;
 
 			try_to_unmap(folio, flags);
 			if (folio_mapped(folio)) {
-- 
2.34.1

From: Barry Song <v-songbaohua@oppo.com>

Within try_to_unmap_one(), page_vma_mapped_walk() races with other
PTE modifications preceded by pte clear. While iterating over PTEs
of a large folio, it only starts acquiring PTL from the first valid
(present) PTE. PTE modifications can temporarily set PTEs to
pte_none. Consequently, the initial PTEs of a large folio might
be skipped in try_to_unmap_one().
For example, for an anon folio, if we skip PTE0, we may have PTE0
which is still present, while PTE1 ~ PTE(nr_pages - 1) are swap
entries after try_to_unmap_one().
So folio will be still mapped, the folio fails to be reclaimed
and is put back to LRU in this round.
This also breaks up PTEs optimization such as CONT-PTE on this
large folio and may lead to accident folio_split() afterwards.
And since a part of PTEs are now swap entries, accessing those
parts will introduce overhead - do_swap_page.
Although the kernel can withstand all of the above issues, the
situation still seems quite awkward and warrants making it more
ideal.
The same race also occurs with small folios, but they have only
one PTE, thus, it won't be possible for them to be partially
unmapped.
This patch holds PTL from PTE0, allowing us to avoid reading PTE
values that are in the process of being transformed. With stable
PTE values, we can ensure that this large folio is either
completely reclaimed or that all PTEs remain untouched in this
round.
A corner case is that if we hold PTL from PTE0 and most initial
PTEs have been really unmapped before that, we may increase the
duration of holding PTL. Thus we only apply this optimization to
folios which are still entirely mapped (not in deferred_split
list).

Cc: Hugh Dickins <hughd@google.com>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
 v2:
  * Refine commit message and code comment after reading all comments
    from Ryan and David, thanks!
  * Avoid increasing the duration of PTL by applying optimization
    on folios not in deferred_split_list with respect to Ying's
    comment, thanks!

mm/vmscan.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index XXXXXXX..XXXXXXX 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -XXX,XX +XXX,XX @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 
 			if (folio_test_pmd_mappable(folio))
 				flags |= TTU_SPLIT_HUGE_PMD;
+			/*
+			 * Without TTU_SYNC, try_to_unmap will only begin to hold PTL
+			 * from the first present PTE within a large folio. Some initial
+			 * PTEs might be skipped due to races with parallel PTE writes
+			 * in which PTEs can be cleared temporarily before being written
+			 * new present values. This will lead to a large folio is still
+			 * mapped while some subpages have been partially unmapped after
+			 * try_to_unmap; TTU_SYNC helps 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))
+				flags |= TTU_SYNC;
 
 			try_to_unmap(folio, flags);
 			if (folio_mapped(folio)) {
-- 
2.34.1