[PATCH] mm: vmscan: the dirty folio unmap redundantly

[PATCH] mm: vmscan: the dirty folio unmap redundantly

[Zhiguo Jiang]

If the dirty folio is not reclaimed in the shrink process, it do
not need to unmap, which can save shrinking time during traversaling
the dirty folio.

Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>
---
 mm/vmscan.c | 72 +++++++++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 35 deletions(-)
 mode change 100644 => 100755 mm/vmscan.c

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 2cc0cb41fb32..cf555cdfcefc
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1261,6 +1261,43 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 			enum ttu_flags flags = TTU_BATCH_FLUSH;
 			bool was_swapbacked = folio_test_swapbacked(folio);
 
+			if (folio_test_dirty(folio)) {
+				/*
+				 * Only kswapd can writeback filesystem folios
+				 * to avoid risk of stack overflow. But avoid
+				 * injecting inefficient single-folio I/O into
+				 * flusher writeback as much as possible: only
+				 * write folios when we've encountered many
+				 * dirty folios, and when we've already scanned
+				 * the rest of the LRU for clean folios and see
+				 * the same dirty folios again (with the reclaim
+				 * flag set).
+				 */
+				if (folio_is_file_lru(folio) &&
+					(!current_is_kswapd() ||
+					 !folio_test_reclaim(folio) ||
+					 !test_bit(PGDAT_DIRTY, &pgdat->flags))) {
+					/*
+					 * Immediately reclaim when written back.
+					 * Similar in principle to folio_deactivate()
+					 * except we already have the folio isolated
+					 * and know it's dirty
+					 */
+					node_stat_mod_folio(folio, NR_VMSCAN_IMMEDIATE,
+							nr_pages);
+					folio_set_reclaim(folio);
+
+					goto activate_locked;
+				}
+
+				if (references == FOLIOREF_RECLAIM_CLEAN)
+					goto keep_locked;
+				if (!may_enter_fs(folio, sc->gfp_mask))
+					goto keep_locked;
+				if (!sc->may_writepage)
+					goto keep_locked;
+			}
+
 			if (folio_test_pmd_mappable(folio))
 				flags |= TTU_SPLIT_HUGE_PMD;
 
@@ -1286,41 +1323,6 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 
 		mapping = folio_mapping(folio);
 		if (folio_test_dirty(folio)) {
-			/*
-			 * Only kswapd can writeback filesystem folios
-			 * to avoid risk of stack overflow. But avoid
-			 * injecting inefficient single-folio I/O into
-			 * flusher writeback as much as possible: only
-			 * write folios when we've encountered many
-			 * dirty folios, and when we've already scanned
-			 * the rest of the LRU for clean folios and see
-			 * the same dirty folios again (with the reclaim
-			 * flag set).
-			 */
-			if (folio_is_file_lru(folio) &&
-			    (!current_is_kswapd() ||
-			     !folio_test_reclaim(folio) ||
-			     !test_bit(PGDAT_DIRTY, &pgdat->flags))) {
-				/*
-				 * Immediately reclaim when written back.
-				 * Similar in principle to folio_deactivate()
-				 * except we already have the folio isolated
-				 * and know it's dirty
-				 */
-				node_stat_mod_folio(folio, NR_VMSCAN_IMMEDIATE,
-						nr_pages);
-				folio_set_reclaim(folio);
-
-				goto activate_locked;
-			}
-
-			if (references == FOLIOREF_RECLAIM_CLEAN)
-				goto keep_locked;
-			if (!may_enter_fs(folio, sc->gfp_mask))
-				goto keep_locked;
-			if (!sc->may_writepage)
-				goto keep_locked;
-
 			/*
 			 * Folio is dirty. Flush the TLB if a writable entry
 			 * potentially exists to avoid CPU writes after I/O
-- 
2.39.0

Re: [PATCH] mm: vmscan: the dirty folio unmap redundantly

[Matthew Wilcox]

On Wed, Oct 18, 2023 at 09:30:03AM +0800, Zhiguo Jiang wrote:
> If the dirty folio is not reclaimed in the shrink process, it do
> not need to unmap, which can save shrinking time during traversaling
> the dirty folio.

Don't we have to unmap it first in order to make sure that all the
dirty bits from the PTEs have been transferred to the folio dirty bit?

答复: [PATCH] mm: vmscan: the dirty folio unmap redundantly

[江志国]

Hi Matthew Wilcox:

>On Wed, Oct 18, 2023 at 09:30:03AM +0800, Zhiguo Jiang wrote:
>> If the dirty folio is not reclaimed in the shrink process, it do not 
>> need to unmap, which can save shrinking time during traversaling the 
>> dirty folio.
>
>Don't we have to unmap it first in order to make sure that all the dirty bits from the PTEs have been transferred to the folio dirty bit?

Yes, if the PTE has the dirty bit, try_to_unmap will transfer it to the folio dirty bit. But the another condition is that the folio dirty bit has been already saved before try_to_unmap. For this situation and the PGDAT_DIRTY flag is not set, the dirty folio can skip unmap to save the shrink time.

The patch submitted previously did not consider the dirty bit trasfferred by unmap, so the original part can be reserved. Please help to continue the review.


Thanks


-----邮件原件-----
发件人: Matthew Wilcox <willy@infradead.org> 
发送时间: 2023年10月18日 22:12
收件人: 江志国 <justinjiang@vivo.com>
抄送: Andrew Morton <akpm@linux-foundation.org>; linux-mm@kvack.org; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com>
主题: Re: [PATCH] mm: vmscan: the dirty folio unmap redundantly

[Some people who received this message don't often get email from willy@infradead.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

On Wed, Oct 18, 2023 at 09:30:03AM +0800, Zhiguo Jiang wrote:
> If the dirty folio is not reclaimed in the shrink process, it do not 
> need to unmap, which can save shrinking time during traversaling the 
> dirty folio.

Don't we have to unmap it first in order to make sure that all the dirty bits from the PTEs have been transferred to the folio dirty bit?

Re: [PATCH] mm: vmscan: the dirty folio unmap redundantly

[David Hildenbrand]

On 18.10.23 03:30, Zhiguo Jiang wrote:
> If the dirty folio is not reclaimed in the shrink process, it do
> not need to unmap, which can save shrinking time during traversaling
> the dirty folio.

Hi,

I really cannot understand what you mean with "the dirty folio unmap 
redundantly". No clue what this patch is supposed to tackle by staring 
at the patch subject.


This patch is supposed to improve performance. Can you provide some 
proof that it does and that we should even care about this change?

> 
> Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>
> ---
>   mm/vmscan.c | 72 +++++++++++++++++++++++++++--------------------------
>   1 file changed, 37 insertions(+), 35 deletions(-)
>   mode change 100644 => 100755 mm/vmscan.c
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 2cc0cb41fb32..cf555cdfcefc
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1261,6 +1261,43 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>   			enum ttu_flags flags = TTU_BATCH_FLUSH;
>   			bool was_swapbacked = folio_test_swapbacked(folio);
>   
> +			if (folio_test_dirty(folio)) {
> +				/*
> +				 * Only kswapd can writeback filesystem folios
> +				 * to avoid risk of stack overflow. But avoid
> +				 * injecting inefficient single-folio I/O into
> +				 * flusher writeback as much as possible: only
> +				 * write folios when we've encountered many
> +				 * dirty folios, and when we've already scanned
> +				 * the rest of the LRU for clean folios and see
> +				 * the same dirty folios again (with the reclaim
> +				 * flag set).
> +				 */
> +				if (folio_is_file_lru(folio) &&
> +					(!current_is_kswapd() ||
> +					 !folio_test_reclaim(folio) ||
> +					 !test_bit(PGDAT_DIRTY, &pgdat->flags))) {
> +					/*
> +					 * Immediately reclaim when written back.
> +					 * Similar in principle to folio_deactivate()
> +					 * except we already have the folio isolated
> +					 * and know it's dirty
> +					 */
> +					node_stat_mod_folio(folio, NR_VMSCAN_IMMEDIATE,
> +							nr_pages);
> +					folio_set_reclaim(folio);
> +
> +					goto activate_locked;
> +				}
> +
> +				if (references == FOLIOREF_RECLAIM_CLEAN)
> +					goto keep_locked;
> +				if (!may_enter_fs(folio, sc->gfp_mask))
> +					goto keep_locked;
> +				if (!sc->may_writepage)
> +					goto keep_locked;
> +			}
> +
>   			if (folio_test_pmd_mappable(folio))
>   				flags |= TTU_SPLIT_HUGE_PMD;
>   
> @@ -1286,41 +1323,6 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>   
>   		mapping = folio_mapping(folio);
>   		if (folio_test_dirty(folio)) {

Can you elaborate why we want to remove below code? It would have made 
sense to me to duplicate the code in an early check before unmap, if the 
folio is already dirty before checking all PTEs. But why can we remove 
that post-unmap code?

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm: vmscan: the dirty folio unmap redundantly

[zhiguojiang]

在 2023/10/19 21:03, David Hildenbrand 写道:
> [你通常不会收到来自 david@redhat.com 的电子邮件。请访问 
> https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要]
>
> On 18.10.23 03:30, Zhiguo Jiang wrote:
>> If the dirty folio is not reclaimed in the shrink process, it do
>> not need to unmap, which can save shrinking time during traversaling
>> the dirty folio.
>
> Hi,
>
> I really cannot understand what you mean with "the dirty folio unmap
> redundantly". No clue what this patch is supposed to tackle by staring
> at the patch subject.
>
>
> This patch is supposed to improve performance. Can you provide some
> proof that it does and that we should even care about this change?

Hi，
What I understand is that in the shrink_folio_list() the sources of the file
dirty folio include two ways below:
1. The dirty folio is from the incoming parameter folio_list,
    which is the inactive file lru.
2. The dirty folio is from the PTE dirty bit transferred by
    the try_to_unmap().

Currently, both sources of dirty pages are determined after unmap
to determine whether they support pageout and recyling.

For the first source of the dirty folio, if the dirty folio does not
support pageout, the dirty folio can skip unmap in advance to reduce
recyling time.

This patch is not well considered.
The v2 new patch will be submitted later, Please help to continue review.

Thanks

Jiang Zhiguo
>
>>
>> Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>
>> ---
>>   mm/vmscan.c | 72 +++++++++++++++++++++++++++--------------------------
>>   1 file changed, 37 insertions(+), 35 deletions(-)
>>   mode change 100644 => 100755 mm/vmscan.c
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 2cc0cb41fb32..cf555cdfcefc
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1261,6 +1261,43 @@ static unsigned int shrink_folio_list(struct 
>> list_head *folio_list,
>>                       enum ttu_flags flags = TTU_BATCH_FLUSH;
>>                       bool was_swapbacked = 
>> folio_test_swapbacked(folio);
>>
>> +                     if (folio_test_dirty(folio)) {
>> +                             /*
>> +                              * Only kswapd can writeback filesystem 
>> folios
>> +                              * to avoid risk of stack overflow. But 
>> avoid
>> +                              * injecting inefficient single-folio 
>> I/O into
>> +                              * flusher writeback as much as 
>> possible: only
>> +                              * write folios when we've encountered 
>> many
>> +                              * dirty folios, and when we've already 
>> scanned
>> +                              * the rest of the LRU for clean folios 
>> and see
>> +                              * the same dirty folios again (with 
>> the reclaim
>> +                              * flag set).
>> +                              */
>> +                             if (folio_is_file_lru(folio) &&
>> +                                     (!current_is_kswapd() ||
>> + !folio_test_reclaim(folio) ||
>> +                                      !test_bit(PGDAT_DIRTY, 
>> &pgdat->flags))) {
>> +                                     /*
>> +                                      * Immediately reclaim when 
>> written back.
>> +                                      * Similar in principle to 
>> folio_deactivate()
>> +                                      * except we already have the 
>> folio isolated
>> +                                      * and know it's dirty
>> +                                      */
>> +                                     node_stat_mod_folio(folio, 
>> NR_VMSCAN_IMMEDIATE,
>> +                                                     nr_pages);
>> +                                     folio_set_reclaim(folio);
>> +
>> +                                     goto activate_locked;
>> +                             }
>> +
>> +                             if (references == FOLIOREF_RECLAIM_CLEAN)
>> +                                     goto keep_locked;
>> +                             if (!may_enter_fs(folio, sc->gfp_mask))
>> +                                     goto keep_locked;
>> +                             if (!sc->may_writepage)
>> +                                     goto keep_locked;
>> +                     }
>> +
>>                       if (folio_test_pmd_mappable(folio))
>>                               flags |= TTU_SPLIT_HUGE_PMD;
>>
>> @@ -1286,41 +1323,6 @@ static unsigned int shrink_folio_list(struct 
>> list_head *folio_list,
>>
>>               mapping = folio_mapping(folio);
>>               if (folio_test_dirty(folio)) {
>
> Can you elaborate why we want to remove below code? It would have made
> sense to me to duplicate the code in an early check before unmap, if the
> folio is already dirty before checking all PTEs. But why can we remove
> that post-unmap code?
>
> -- 
> Cheers,
>
> David / dhildenb
>