1. Patchew
  2. linux
  3. [PATCH v2 0/3] mm: memory_failure: unmap poisoned filio during migrate properly
  4. View patch

[PATCH v2 3/3] mm: memory-hotplug: check folio ref count first in do_migrate_rang

Wupeng Ma posted 3 patches 11 months ago
There is a newer version of this series
[PATCH v2 3/3] mm: memory-hotplug: check folio ref count first in do_migrate_rang
Posted by Wupeng Ma 11 months ago 
From: Ma Wupeng <mawupeng1@huawei.com>

If a folio has an increased reference count, folio_try_get() will acquire
it, perform necessary operations, and then release it. In the case of a
poisoned folio without an elevated reference count (which is unlikely for
memory-failure), folio_try_get() will simply bypass it.

Therefore, relocate the folio_try_get() function, responsible for checking
and acquiring this reference count at first.

Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
---
 mm/memory_hotplug.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 2815bd4ea483..3fb75ee185c6 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1786,6 +1786,9 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		page = pfn_to_page(pfn);
 		folio = page_folio(page);
 
+		if (!folio_try_get(folio))
+			continue;
+
 		/*
 		 * No reference or lock is held on the folio, so it might
 		 * be modified concurrently (e.g. split).  As such,
@@ -1795,12 +1798,6 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		if (folio_test_large(folio))
 			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
 
-		/*
-		 * HWPoison pages have elevated reference counts so the migration would
-		 * fail on them. It also doesn't make any sense to migrate them in the
-		 * first place. Still try to unmap such a page in case it is still mapped
-		 * (keep the unmap as the catch all safety net).
-		 */
 		if (folio_test_hwpoison(folio) ||
 		    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
 			if (WARN_ON(folio_test_lru(folio)))
@@ -1811,12 +1808,9 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 				folio_unlock(folio);
 			}
 
-			continue;
+			goto put_folio;
 		}
 
-		if (!folio_try_get(folio))
-			continue;
-
 		if (unlikely(page_folio(page) != folio))
 			goto put_folio;
 
-- 
2.43.0
Re: [PATCH v2 3/3] mm: memory-hotplug: check folio ref count first in do_migrate_rang
Posted by David Hildenbrand 10 months, 4 weeks ago 
On 16.01.25 07:16, Wupeng Ma wrote:
> From: Ma Wupeng <mawupeng1@huawei.com>
> 
> If a folio has an increased reference count, folio_try_get() will acquire
> it, perform necessary operations, and then release it. In the case of a
> poisoned folio without an elevated reference count (which is unlikely for
> memory-failure), folio_try_get() will simply bypass it.
> 
> Therefore, relocate the folio_try_get() function, responsible for checking
> and acquiring this reference count at first.
> 
> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
> ---
>   mm/memory_hotplug.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 2815bd4ea483..3fb75ee185c6 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1786,6 +1786,9 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   		page = pfn_to_page(pfn);
>   		folio = page_folio(page);
>   
> +		if (!folio_try_get(folio))
> +			continue;
> +

I would only move it in front of the folio_test_hwpoison() check for 
now. Note that with this patch as is the comment below would be wrong

>   		/*
>   		 * No reference or lock is held on the folio, so it might

^

I would move this patch before the current #2, so the folio_lock() looks 
less weird.

-- 
Cheers,

David / dhildenb
Re: [PATCH v2 3/3] mm: memory-hotplug: check folio ref count first in do_migrate_rang
Posted by mawupeng 10 months, 4 weeks ago 

On 2025/1/20 16:01, David Hildenbrand wrote:
> On 16.01.25 07:16, Wupeng Ma wrote:
>> From: Ma Wupeng <mawupeng1@huawei.com>
>>
>> If a folio has an increased reference count, folio_try_get() will acquire
>> it, perform necessary operations, and then release it. In the case of a
>> poisoned folio without an elevated reference count (which is unlikely for
>> memory-failure), folio_try_get() will simply bypass it.
>>
>> Therefore, relocate the folio_try_get() function, responsible for checking
>> and acquiring this reference count at first.
>>
>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>> ---
>>   mm/memory_hotplug.c | 14 ++++----------
>>   1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 2815bd4ea483..3fb75ee185c6 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1786,6 +1786,9 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>           page = pfn_to_page(pfn);
>>           folio = page_folio(page);
>>   +        if (!folio_try_get(folio))
>> +            continue;
>> +
> 
> I would only move it in front of the folio_test_hwpoison() check for now. Note that with this patch as is the comment below would be wrong

Thanks for notice this.

Move it in front of the folio_test_hwpoison() do seems better.

> 
>>           /*
>>            * No reference or lock is held on the folio, so it might
> 
> ^
> 
> I would move this patch before the current #2, so the folio_lock() looks less weird.
> 

Ok, will be done.

Thanks.
Re: [PATCH v2 3/3] mm: memory-hotplug: check folio ref count first in do_migrate_rang
Posted by Miaohe Lin 10 months, 4 weeks ago 
On 2025/1/16 14:16, Wupeng Ma wrote:
> From: Ma Wupeng <mawupeng1@huawei.com>
> 
> If a folio has an increased reference count, folio_try_get() will acquire
> it, perform necessary operations, and then release it. In the case of a
> poisoned folio without an elevated reference count (which is unlikely for
> memory-failure), folio_try_get() will simply bypass it.
> 
> Therefore, relocate the folio_try_get() function, responsible for checking
> and acquiring this reference count at first.
> 
> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
> ---
>  mm/memory_hotplug.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 2815bd4ea483..3fb75ee185c6 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1786,6 +1786,9 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  		page = pfn_to_page(pfn);
>  		folio = page_folio(page);
>  
> +		if (!folio_try_get(folio))
> +			continue;
> +
>  		/*
>  		 * No reference or lock is held on the folio, so it might
>  		 * be modified concurrently (e.g. split).  As such,
> @@ -1795,12 +1798,6 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  		if (folio_test_large(folio))
>  			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>  
> -		/*
> -		 * HWPoison pages have elevated reference counts so the migration would
> -		 * fail on them. It also doesn't make any sense to migrate them in the
> -		 * first place. Still try to unmap such a page in case it is still mapped
> -		 * (keep the unmap as the catch all safety net).
> -		 */
>  		if (folio_test_hwpoison(folio) ||
>  		    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>  			if (WARN_ON(folio_test_lru(folio)))
> @@ -1811,12 +1808,9 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  				folio_unlock(folio);
>  			}
>  
> -			continue;
> +			goto put_folio;
>  		}
>  
> -		if (!folio_try_get(folio))
> -			continue;
> -
>  		if (unlikely(page_folio(page) != folio))
>  			goto put_folio;

Will it be necessary to move this check above folio_test_hwpoison trunk too?

Thanks.
.
Re: [PATCH v2 3/3] mm: memory-hotplug: check folio ref count first in do_migrate_rang
Posted by mawupeng 10 months, 3 weeks ago 

On 2025/1/20 14:32, Miaohe Lin wrote:
> On 2025/1/16 14:16, Wupeng Ma wrote:
>> From: Ma Wupeng <mawupeng1@huawei.com>
>>
>> If a folio has an increased reference count, folio_try_get() will acquire
>> it, perform necessary operations, and then release it. In the case of a
>> poisoned folio without an elevated reference count (which is unlikely for
>> memory-failure), folio_try_get() will simply bypass it.
>>
>> Therefore, relocate the folio_try_get() function, responsible for checking
>> and acquiring this reference count at first.
>>
>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>> ---
>>  mm/memory_hotplug.c | 14 ++++----------
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 2815bd4ea483..3fb75ee185c6 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1786,6 +1786,9 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>  		page = pfn_to_page(pfn);
>>  		folio = page_folio(page);
>>  
>> +		if (!folio_try_get(folio))
>> +			continue;
>> +
>>  		/*
>>  		 * No reference or lock is held on the folio, so it might
>>  		 * be modified concurrently (e.g. split).  As such,
>> @@ -1795,12 +1798,6 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>  		if (folio_test_large(folio))
>>  			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>  
>> -		/*
>> -		 * HWPoison pages have elevated reference counts so the migration would
>> -		 * fail on them. It also doesn't make any sense to migrate them in the
>> -		 * first place. Still try to unmap such a page in case it is still mapped
>> -		 * (keep the unmap as the catch all safety net).
>> -		 */
>>  		if (folio_test_hwpoison(folio) ||
>>  		    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>>  			if (WARN_ON(folio_test_lru(folio)))
>> @@ -1811,12 +1808,9 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>  				folio_unlock(folio);
>>  			}
>>  
>> -			continue;
>> +			goto put_folio;
>>  		}
>>  
>> -		if (!folio_try_get(folio))
>> -			continue;
>> -
>>  		if (unlikely(page_folio(page) != folio))
>>  			goto put_folio;
> 
> Will it be necessary to move this check above folio_test_hwpoison trunk too?

Thanks.

AFAICT  we can do this, I'll move this in the next patch. there is no need to handle this page if
the state of this folio changes.

> 
> Thanks.
> .

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.