[PATCH] mm/vmscan: don't try to reclaim freed folios

[PATCH] mm/vmscan: don't try to reclaim freed folios

[Miaohe Lin]

If folios were freed from under us, there's no need to reclaim them. Skip
these folios to save lots of cpu cycles and avoid possible unnecessary
disk IO.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/vmscan.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index f7d9a683e3a7..646dd1efad32 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1556,12 +1556,18 @@ static unsigned int shrink_page_list(struct list_head *page_list,
 		folio = lru_to_folio(page_list);
 		list_del(&folio->lru);
 
+		nr_pages = folio_nr_pages(folio);
+		if (folio_ref_count(folio) == 1) {
+			/* folio was freed from under us. So we are done. */
+			WARN_ON(!folio_put_testzero(folio));
+			goto free_it;
+		}
+
 		if (!folio_trylock(folio))
 			goto keep;
 
 		VM_BUG_ON_FOLIO(folio_test_active(folio), folio);
 
-		nr_pages = folio_nr_pages(folio);
 
 		/* Account the number of base pages */
 		sc->nr_scanned += nr_pages;
-- 
2.23.0

Re: [PATCH] mm/vmscan: don't try to reclaim freed folios

[Matthew Wilcox]

On Fri, May 27, 2022 at 04:04:51PM +0800, Miaohe Lin wrote:
> If folios were freed from under us, there's no need to reclaim them. Skip
> these folios to save lots of cpu cycles and avoid possible unnecessary
> disk IO.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/vmscan.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index f7d9a683e3a7..646dd1efad32 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1556,12 +1556,18 @@ static unsigned int shrink_page_list(struct list_head *page_list,
>  		folio = lru_to_folio(page_list);
>  		list_del(&folio->lru);
>  
> +		nr_pages = folio_nr_pages(folio);
> +		if (folio_ref_count(folio) == 1) {
> +			/* folio was freed from under us. So we are done. */
> +			WARN_ON(!folio_put_testzero(folio));

What?  No.  This can absolutely happen.  We have a refcount on the folio,
which means that any other thread can temporarily raise the refcount,
so this WARN_ON can trigger.  Also, we don't hold the folio locked,
or an extra reference, so nr_pages is unstable because it can be split.

> +			goto free_it;
> +		}
> +
>  		if (!folio_trylock(folio))
>  			goto keep;
>  
>  		VM_BUG_ON_FOLIO(folio_test_active(folio), folio);
>  
> -		nr_pages = folio_nr_pages(folio);
>  
>  		/* Account the number of base pages */
>  		sc->nr_scanned += nr_pages;
> -- 
> 2.23.0
> 
> 

Re: [PATCH] mm/vmscan: don't try to reclaim freed folios

[Miaohe Lin]

On 2022/5/27 23:02, Matthew Wilcox wrote:
> On Fri, May 27, 2022 at 04:04:51PM +0800, Miaohe Lin wrote:
>> If folios were freed from under us, there's no need to reclaim them. Skip
>> these folios to save lots of cpu cycles and avoid possible unnecessary
>> disk IO.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/vmscan.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index f7d9a683e3a7..646dd1efad32 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1556,12 +1556,18 @@ static unsigned int shrink_page_list(struct list_head *page_list,
>>  		folio = lru_to_folio(page_list);
>>  		list_del(&folio->lru);
>>  
>> +		nr_pages = folio_nr_pages(folio);
>> +		if (folio_ref_count(folio) == 1) {
>> +			/* folio was freed from under us. So we are done. */
>> +			WARN_ON(!folio_put_testzero(folio));
> 
> What?  No.  This can absolutely happen.  We have a refcount on the folio,
> which means that any other thread can temporarily raise the refcount,

IIUC, the folio is only in the isolated page_list now and it's not in the page cache, swap cache, pagetable or
under any use. So there should be no way that any other thread can temporarily raise the refcount when
folio_ref_count == 1. Or am I miss something?

> so this WARN_ON can trigger.  Also, we don't hold the folio locked,
> or an extra reference, so nr_pages is unstable because it can be split.

Yes, you're right. When folio_ref_count != 1, nr_pages is unstable. Will fix it if v2 is possible. :)

Thanks a lot for review and comment!

> 
>> +			goto free_it;
>> +		}
>> +
>>  		if (!folio_trylock(folio))
>>  			goto keep;
>>  
>>  		VM_BUG_ON_FOLIO(folio_test_active(folio), folio);
>>  
>> -		nr_pages = folio_nr_pages(folio);
>>  
>>  		/* Account the number of base pages */
>>  		sc->nr_scanned += nr_pages;
>> -- 
>> 2.23.0
>>
>>
> 
> .
> 

Re: [PATCH] mm/vmscan: don't try to reclaim freed folios

[Matthew Wilcox]

On Sat, May 28, 2022 at 10:52:11AM +0800, Miaohe Lin wrote:
> On 2022/5/27 23:02, Matthew Wilcox wrote:
> > What?  No.  This can absolutely happen.  We have a refcount on the folio,
> > which means that any other thread can temporarily raise the refcount,
> 
> IIUC, the folio is only in the isolated page_list now and it's not in the page cache, swap cache, pagetable or
> under any use. So there should be no way that any other thread can temporarily raise the refcount when
> folio_ref_count == 1. Or am I miss something?

Take a look at something like GUP (fast).  If this page _was_ mapped to
userspace, something like this can happen:

Thread A	Thread B
load PTE
		unmap page
		refcount goes to 1
		vmscan sees the page
try_get_ref
		refcount is now 2.  WARN_ON.

Thread A will see that the PTE has changed and will now drop its
reference, but Thread B already spat out the WARN.

A similar thing can happen with the page cache.

If this is a worthwhile optimisation (does it happen often that we find
a refcount == 1 page?), we could do something like ...

		if (folio_ref_freeze(folio, 1)) {
			nr_pages = folio_nr_pages(folio);
			goto free_it;
		}

... or ...

		if (folio_ref_count(folio) == 1 &&
		    folio_ref_freeze(folio, 1)) {

... if we want to test-and-test-and-clear

But this function is far too complicated already.  I really want to
see numbers that proves the extra complexity is worth it.

Re: [PATCH] mm/vmscan: don't try to reclaim freed folios

[Miaohe Lin]

On 2022/5/28 11:13, Matthew Wilcox wrote:
> On Sat, May 28, 2022 at 10:52:11AM +0800, Miaohe Lin wrote:
>> On 2022/5/27 23:02, Matthew Wilcox wrote:
>>> What?  No.  This can absolutely happen.  We have a refcount on the folio,
>>> which means that any other thread can temporarily raise the refcount,
>>
>> IIUC, the folio is only in the isolated page_list now and it's not in the page cache, swap cache, pagetable or
>> under any use. So there should be no way that any other thread can temporarily raise the refcount when
>> folio_ref_count == 1. Or am I miss something?
> 
> Take a look at something like GUP (fast).  If this page _was_ mapped to
> userspace, something like this can happen:
> 
> Thread A	Thread B
> load PTE
> 		unmap page
> 		refcount goes to 1
> 		vmscan sees the page
> try_get_ref
> 		refcount is now 2.  WARN_ON.
> 
> Thread A will see that the PTE has changed and will now drop its
> reference, but Thread B already spat out the WARN.
> 
> A similar thing can happen with the page cache.

Oh, I see. Many thanks for your patient explanation! :)

> 
> If this is a worthwhile optimisation (does it happen often that we find
> a refcount == 1 page?), we could do something like ...

No, It should be rare.

> 
> 		if (folio_ref_freeze(folio, 1)) {
> 			nr_pages = folio_nr_pages(folio);
> 			goto free_it;
> 		}
> 
> ... or ...
> 
> 		if (folio_ref_count(folio) == 1 &&
> 		    folio_ref_freeze(folio, 1)) {
> 
> ... if we want to test-and-test-and-clear

These proposed code changes look good to me.

> 
> But this function is far too complicated already.  I really want to
> see numbers that proves the extra complexity is worth it.

This optimization can save lots of cpu cycles and avoid possible disk I/O in
that specified case. But that is a somewhat rare case. So there's no numbers
that proves the extra complexity is worth it.

Should I drop this patch or proceed with the proposed code changes above in
next version? :)

Many thanks!

> 
> 
> .
> 

Re: [PATCH] mm/vmscan: don't try to reclaim freed folios

[Miaohe Lin]

On 2022/5/27 23:02, Matthew Wilcox wrote:
> On Fri, May 27, 2022 at 04:04:51PM +0800, Miaohe Lin wrote:
>> If folios were freed from under us, there's no need to reclaim them. Skip
>> these folios to save lots of cpu cycles and avoid possible unnecessary
>> disk IO.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/vmscan.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index f7d9a683e3a7..646dd1efad32 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1556,12 +1556,18 @@ static unsigned int shrink_page_list(struct list_head *page_list,
>>  		folio = lru_to_folio(page_list);
>>  		list_del(&folio->lru);
>>  
>> +		nr_pages = folio_nr_pages(folio);
>> +		if (folio_ref_count(folio) == 1) {
>> +			/* folio was freed from under us. So we are done. */
>> +			WARN_ON(!folio_put_testzero(folio));
> 
> What?  No.  This can absolutely happen.  We have a refcount on the folio,
> which means that any other thread can temporarily raise the refcount,
> so this WARN_ON can trigger.  Also, we don't hold the folio locked,
> or an extra reference, so nr_pages is unstable because it can be split.

When I reread the code, I found caller holds an extra reference to the folio when
calling isolate_lru_pages(), so folio can't be split and thus nr_pages should be
stable indeed? Or am I miss something again?

Thanks!

> 
>> +			goto free_it;
>> +		}
>> +
>>  		if (!folio_trylock(folio))
>>  			goto keep;
>>  
>>  		VM_BUG_ON_FOLIO(folio_test_active(folio), folio);
>>  
>> -		nr_pages = folio_nr_pages(folio);
>>  
>>  		/* Account the number of base pages */
>>  		sc->nr_scanned += nr_pages;
>> -- 
>> 2.23.0
>>
>>
> 
> .
>