[PATCH resend v2 0/5] mm: memory_hotplug: improve do_migrate_range()

[PATCH resend v2 0/5] mm: memory_hotplug: improve do_migrate_range()

[Kefeng Wang]

Unify hwpoisoned page handling and isolation of HugeTLB/LRU/non-LRU
movable page, also convert to use folios in do_migrate_range().

v2-resend:
- fix isolate_hugetlb() build error in patch1

v2:
- address comments from David(eg, fix HWPoison check/use a folio
  for pfn calculation firstly)
- fix lkp build errors for isolate_folio_to_list()
- drop unnecessary comments and don't grab one more ref for hugetlb 

Kefeng Wang (5):
  mm: memory_hotplug: remove head variable in do_migrate_range()
  mm: memory-failure: add unmap_posioned_folio()
  mm: memory_hotplug: check hwpoisoned page firstly in
    do_migrate_range()
  mm: migrate: add isolate_folio_to_list()
  mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

 include/linux/migrate.h |  3 ++
 mm/internal.h           |  9 +++++
 mm/memory-failure.c     | 89 +++++++++++++++++------------------------
 mm/memory_hotplug.c     | 62 ++++++++++++++--------------
 mm/migrate.c            | 27 +++++++++++++
 5 files changed, 105 insertions(+), 85 deletions(-)

-- 
2.27.0

[PATCH v2 1/5] mm: memory_hotplug: remove head variable in do_migrate_range()

[Kefeng Wang]

Directly use a folio for HugeTLB and THP when calculate the next pfn,
then remove unused head variable.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/memory_hotplug.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index df291f2e509d..dc19b0e28fbc 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1773,7 +1773,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
 static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 {
 	unsigned long pfn;
-	struct page *page, *head;
+	struct page *page;
 	LIST_HEAD(source);
 	static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL,
 				      DEFAULT_RATELIMIT_BURST);
@@ -1786,14 +1786,20 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 			continue;
 		page = pfn_to_page(pfn);
 		folio = page_folio(page);
-		head = &folio->page;
 
-		if (PageHuge(page)) {
-			pfn = page_to_pfn(head) + compound_nr(head) - 1;
-			isolate_hugetlb(folio, &source);
-			continue;
-		} else if (PageTransHuge(page))
-			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
+		/*
+		 * The folio hold no reference or lock, it might be changed
+		 * concurrently(eg, split), folio_nr_pages() may read garbage,
+		 * but out loop could handle that as it revisits the split
+		 * folio later.
+		 */
+		if (folio_test_large(folio)) {
+			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
+			if (folio_test_hugetlb(folio)) {
+				isolate_hugetlb(folio, &source);
+				continue;
+			}
+		}
 
 		/*
 		 * HWPoison pages have elevated reference counts so the migration would
-- 
2.27.0

Re: [PATCH v2 1/5] mm: memory_hotplug: remove head variable in do_migrate_range()

[Jonathan Cameron]

On Sat, 17 Aug 2024 16:49:37 +0800
Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> Directly use a folio for HugeTLB and THP when calculate the next pfn,
> then remove unused head variable.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>

Hi,

I was curious so had a read and have a quick comment
on the comment.


> ---
>  mm/memory_hotplug.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index df291f2e509d..dc19b0e28fbc 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1773,7 +1773,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
>  static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  {
>  	unsigned long pfn;
> -	struct page *page, *head;
> +	struct page *page;
>  	LIST_HEAD(source);
>  	static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL,
>  				      DEFAULT_RATELIMIT_BURST);
> @@ -1786,14 +1786,20 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  			continue;
>  		page = pfn_to_page(pfn);
>  		folio = page_folio(page);
> -		head = &folio->page;
>  
> -		if (PageHuge(page)) {
> -			pfn = page_to_pfn(head) + compound_nr(head) - 1;
> -			isolate_hugetlb(folio, &source);
> -			continue;
> -		} else if (PageTransHuge(page))
> -			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
> +		/*
> +		 * The folio hold no reference or lock, it might be changed
> +		 * concurrently(eg, split), folio_nr_pages() may read garbage,
> +		 * but out loop could handle that as it revisits the split
> +		 * folio later.
> +		 */
Perhaps reword this (it confused me a bit) I'm not sure I've retained
the intended meaning!

		* No reference or lock is held on the folio, so it might
		* be modified concurrently (e.g. split).  As such,
		* folio_nr_pages() may read garbage.  This is fine as the
		* outer loop will revisit the split folio later.


> +		if (folio_test_large(folio)) {
> +			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
> +			if (folio_test_hugetlb(folio)) {
> +				isolate_hugetlb(folio, &source);
> +				continue;
> +			}
> +		}
>  
>  		/*
>  		 * HWPoison pages have elevated reference counts so the migration would

Re: [PATCH v2 1/5] mm: memory_hotplug: remove head variable in do_migrate_range()

[Kefeng Wang]

On 2024/8/19 17:28, Jonathan Cameron wrote:
> On Sat, 17 Aug 2024 16:49:37 +0800
> Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
>> Directly use a folio for HugeTLB and THP when calculate the next pfn,
>> then remove unused head variable.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> 
> Hi,
> 
> I was curious so had a read and have a quick comment
> on the comment.
> 
> 
>> ---
>>   mm/memory_hotplug.c | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index df291f2e509d..dc19b0e28fbc 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1773,7 +1773,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
>>   static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>   {
>>   	unsigned long pfn;
>> -	struct page *page, *head;
>> +	struct page *page;
>>   	LIST_HEAD(source);
>>   	static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL,
>>   				      DEFAULT_RATELIMIT_BURST);
>> @@ -1786,14 +1786,20 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>   			continue;
>>   		page = pfn_to_page(pfn);
>>   		folio = page_folio(page);
>> -		head = &folio->page;
>>   
>> -		if (PageHuge(page)) {
>> -			pfn = page_to_pfn(head) + compound_nr(head) - 1;
>> -			isolate_hugetlb(folio, &source);
>> -			continue;
>> -		} else if (PageTransHuge(page))
>> -			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
>> +		/*
>> +		 * The folio hold no reference or lock, it might be changed
>> +		 * concurrently(eg, split), folio_nr_pages() may read garbage,
>> +		 * but out loop could handle that as it revisits the split
>> +		 * folio later.
>> +		 */
> Perhaps reword this (it confused me a bit) I'm not sure I've retained
> the intended meaning!
> 
> 		* No reference or lock is held on the folio, so it might
> 		* be modified concurrently (e.g. split).  As such,
> 		* folio_nr_pages() may read garbage.  This is fine as the
> 		* outer loop will revisit the split folio later.

Thanks. hope Andrew could squash it if no more comments about this
patchset, or I will resend a new version with comment changes.


> 
> 
>> +		if (folio_test_large(folio)) {
>> +			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>> +			if (folio_test_hugetlb(folio)) {
>> +				isolate_hugetlb(folio, &source);
>> +				continue;
>> +			}
>> +		}
>>   
>>   		/*
>>   		 * HWPoison pages have elevated reference counts so the migration would
> 

Re: [PATCH v2 1/5] mm: memory_hotplug: remove head variable in do_migrate_range()

[Miaohe Lin]

On 2024/8/19 17:28, Jonathan Cameron wrote:
> On Sat, 17 Aug 2024 16:49:37 +0800
> Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
>> Directly use a folio for HugeTLB and THP when calculate the next pfn,
>> then remove unused head variable.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> 
> Hi,
> 
> I was curious so had a read and have a quick comment
> on the comment.
> 
> 
>> ---
>>  mm/memory_hotplug.c | 22 ++++++++++++++--------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index df291f2e509d..dc19b0e28fbc 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1773,7 +1773,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
>>  static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>  {
>>  	unsigned long pfn;
>> -	struct page *page, *head;
>> +	struct page *page;
>>  	LIST_HEAD(source);
>>  	static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL,
>>  				      DEFAULT_RATELIMIT_BURST);
>> @@ -1786,14 +1786,20 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>  			continue;
>>  		page = pfn_to_page(pfn);
>>  		folio = page_folio(page);
>> -		head = &folio->page;
>>  
>> -		if (PageHuge(page)) {
>> -			pfn = page_to_pfn(head) + compound_nr(head) - 1;
>> -			isolate_hugetlb(folio, &source);
>> -			continue;
>> -		} else if (PageTransHuge(page))
>> -			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
>> +		/*
>> +		 * The folio hold no reference or lock, it might be changed
>> +		 * concurrently(eg, split), folio_nr_pages() may read garbage,
>> +		 * but out loop could handle that as it revisits the split
>> +		 * folio later.
>> +		 */
> Perhaps reword this (it confused me a bit) I'm not sure I've retained
> the intended meaning!
> 
> 		* No reference or lock is held on the folio, so it might
> 		* be modified concurrently (e.g. split).  As such,
> 		* folio_nr_pages() may read garbage.  This is fine as the
> 		* outer loop will revisit the split folio later.

With above comment changed, this patch looks good to me.
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

Thanks.
.

Re: [PATCH v2 1/5] mm: memory_hotplug: remove head variable in do_migrate_range()

[David Hildenbrand]

On 17.08.24 10:49, Kefeng Wang wrote:
> Directly use a folio for HugeTLB and THP when calculate the next pfn,
> then remove unused head variable.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   mm/memory_hotplug.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index df291f2e509d..dc19b0e28fbc 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1773,7 +1773,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
>   static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   {
>   	unsigned long pfn;
> -	struct page *page, *head;
> +	struct page *page;
>   	LIST_HEAD(source);
>   	static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL,
>   				      DEFAULT_RATELIMIT_BURST);
> @@ -1786,14 +1786,20 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   			continue;
>   		page = pfn_to_page(pfn);
>   		folio = page_folio(page);
> -		head = &folio->page;
>   
> -		if (PageHuge(page)) {
> -			pfn = page_to_pfn(head) + compound_nr(head) - 1;
> -			isolate_hugetlb(folio, &source);
> -			continue;
> -		} else if (PageTransHuge(page))
> -			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
> +		/*
> +		 * The folio hold no reference or lock, it might be changed
> +		 * concurrently(eg, split), folio_nr_pages() may read garbage,
> +		 * but out loop could handle that as it revisits the split
> +		 * folio later.
> +		 */
> +		if (folio_test_large(folio)) {
> +			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
> +			if (folio_test_hugetlb(folio)) {
> +				isolate_hugetlb(folio, &source);
> +				continue;
> +			}
> +		}
>   
>   		/*
>   		 * HWPoison pages have elevated reference counts so the migration would

With the comment cleanup

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

[PATCH v2 2/5] mm: memory-failure: add unmap_posioned_folio()

[Kefeng Wang]

Add unmap_posioned_folio() helper which will be reused by
do_migrate_range() from memory hotplug soon.

Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/internal.h       |  9 +++++++++
 mm/memory-failure.c | 43 ++++++++++++++++++++++++++-----------------
 2 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index d7aac802efa5..74490b8ac63d 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1045,6 +1045,8 @@ static inline int find_next_best_node(int node, nodemask_t *used_node_mask)
 /*
  * mm/memory-failure.c
  */
+#ifdef CONFIG_MEMORY_FAILURE
+int unmap_posioned_folio(struct folio *folio, enum ttu_flags ttu);
 void shake_folio(struct folio *folio);
 extern int hwpoison_filter(struct page *p);
 
@@ -1065,6 +1067,13 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p,
 		     unsigned long ksm_addr);
 unsigned long page_mapped_in_vma(struct page *page, struct vm_area_struct *vma);
 
+#else
+static inline int unmap_posioned_folio(struct folio *folio, enum ttu_flags ttu)
+{
+	return 0;
+}
+#endif
+
 extern unsigned long  __must_check vm_mmap_pgoff(struct file *, unsigned long,
         unsigned long, unsigned long,
         unsigned long, unsigned long);
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 353254537b54..93848330de1f 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1554,6 +1554,30 @@ static int get_hwpoison_page(struct page *p, unsigned long flags)
 	return ret;
 }
 
+int unmap_posioned_folio(struct folio *folio, enum ttu_flags ttu)
+{
+	if (folio_test_hugetlb(folio) && !folio_test_anon(folio)) {
+		struct address_space *mapping;
+		/*
+		 * For hugetlb pages in shared mappings, try_to_unmap
+		 * could potentially call huge_pmd_unshare.  Because of
+		 * this, take semaphore in write mode here and set
+		 * TTU_RMAP_LOCKED to indicate we have taken the lock
+		 * at this higher level.
+		 */
+		mapping = hugetlb_folio_mapping_lock_write(folio);
+		if (!mapping)
+			return -EAGAIN;
+
+		try_to_unmap(folio, ttu|TTU_RMAP_LOCKED);
+		i_mmap_unlock_write(mapping);
+	} else {
+		try_to_unmap(folio, ttu);
+	}
+
+	return 0;
+}
+
 /*
  * Do all that is necessary to remove user space mappings. Unmap
  * the pages and send SIGBUS to the processes if the data was dirty.
@@ -1615,23 +1639,8 @@ static bool hwpoison_user_mappings(struct folio *folio, struct page *p,
 	 */
 	collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
 
-	if (folio_test_hugetlb(folio) && !folio_test_anon(folio)) {
-		/*
-		 * For hugetlb pages in shared mappings, try_to_unmap
-		 * could potentially call huge_pmd_unshare.  Because of
-		 * this, take semaphore in write mode here and set
-		 * TTU_RMAP_LOCKED to indicate we have taken the lock
-		 * at this higher level.
-		 */
-		mapping = hugetlb_folio_mapping_lock_write(folio);
-		if (mapping) {
-			try_to_unmap(folio, ttu|TTU_RMAP_LOCKED);
-			i_mmap_unlock_write(mapping);
-		} else
-			pr_info("%#lx: could not lock mapping for mapped huge page\n", pfn);
-	} else {
-		try_to_unmap(folio, ttu);
-	}
+	if (unmap_posioned_folio(folio, ttu))
+		pr_info("%#lx: could not lock mapping for mapped huge page\n", pfn);
 
 	unmap_success = !folio_mapped(folio);
 	if (!unmap_success)
-- 
2.27.0

Re: [PATCH v2 2/5] mm: memory-failure: add unmap_posioned_folio()

[Miaohe Lin]

On 2024/8/17 16:49, Kefeng Wang wrote:
> Add unmap_posioned_folio() helper which will be reused by
> do_migrate_range() from memory hotplug soon.
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  mm/internal.h       |  9 +++++++++
>  mm/memory-failure.c | 43 ++++++++++++++++++++++++++-----------------
>  2 files changed, 35 insertions(+), 17 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index d7aac802efa5..74490b8ac63d 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1045,6 +1045,8 @@ static inline int find_next_best_node(int node, nodemask_t *used_node_mask)
>  /*
>   * mm/memory-failure.c
>   */
> +#ifdef CONFIG_MEMORY_FAILURE
> +int unmap_posioned_folio(struct folio *folio, enum ttu_flags ttu);
>  void shake_folio(struct folio *folio);
>  extern int hwpoison_filter(struct page *p);
>  
> @@ -1065,6 +1067,13 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p,
>  		     unsigned long ksm_addr);
>  unsigned long page_mapped_in_vma(struct page *page, struct vm_area_struct *vma);
>  
> +#else
> +static inline int unmap_posioned_folio(struct folio *folio, enum ttu_flags ttu)
> +{
> +	return 0;
> +}
> +#endif
> +
>  extern unsigned long  __must_check vm_mmap_pgoff(struct file *, unsigned long,
>          unsigned long, unsigned long,
>          unsigned long, unsigned long);
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 353254537b54..93848330de1f 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1554,6 +1554,30 @@ static int get_hwpoison_page(struct page *p, unsigned long flags)
>  	return ret;
>  }
>  
> +int unmap_posioned_folio(struct folio *folio, enum ttu_flags ttu)

Should it be unmap_poisoned_folio ? i.e. s/posioned/poisoned/ :)

> +{
> +	if (folio_test_hugetlb(folio) && !folio_test_anon(folio)) {
> +		struct address_space *mapping;
> +		/*
> +		 * For hugetlb pages in shared mappings, try_to_unmap
> +		 * could potentially call huge_pmd_unshare.  Because of
> +		 * this, take semaphore in write mode here and set
> +		 * TTU_RMAP_LOCKED to indicate we have taken the lock
> +		 * at this higher level.
> +		 */
> +		mapping = hugetlb_folio_mapping_lock_write(folio);
> +		if (!mapping)
> +			return -EAGAIN;
> +
> +		try_to_unmap(folio, ttu|TTU_RMAP_LOCKED);
> +		i_mmap_unlock_write(mapping);
> +	} else {
> +		try_to_unmap(folio, ttu);
> +	}
> +
> +	return 0;

It seems return value is not really needed? We will check folio_mapped below.

> +}
> +
>  /*
>   * Do all that is necessary to remove user space mappings. Unmap
>   * the pages and send SIGBUS to the processes if the data was dirty.
> @@ -1615,23 +1639,8 @@ static bool hwpoison_user_mappings(struct folio *folio, struct page *p,
>  	 */
>  	collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>  
> -	if (folio_test_hugetlb(folio) && !folio_test_anon(folio)) {
> -		/*
> -		 * For hugetlb pages in shared mappings, try_to_unmap
> -		 * could potentially call huge_pmd_unshare.  Because of
> -		 * this, take semaphore in write mode here and set
> -		 * TTU_RMAP_LOCKED to indicate we have taken the lock
> -		 * at this higher level.
> -		 */
> -		mapping = hugetlb_folio_mapping_lock_write(folio);
> -		if (mapping) {
> -			try_to_unmap(folio, ttu|TTU_RMAP_LOCKED);
> -			i_mmap_unlock_write(mapping);
> -		} else
> -			pr_info("%#lx: could not lock mapping for mapped huge page\n", pfn);
> -	} else {
> -		try_to_unmap(folio, ttu);
> -	}
> +	if (unmap_posioned_folio(folio, ttu))
> +		pr_info("%#lx: could not lock mapping for mapped huge page\n", pfn);

It might be better to pr_info inside the unmap_posioned_folio? unmap_posioned_folio might fail due
to other reasons in the future anyway.

Thanks.
.

Re: [PATCH v2 2/5] mm: memory-failure: add unmap_posioned_folio()

[Kefeng Wang]

On 2024/8/21 15:40, Miaohe Lin wrote:
> On 2024/8/17 16:49, Kefeng Wang wrote:
>> Add unmap_posioned_folio() helper which will be reused by
>> do_migrate_range() from memory hotplug soon.
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/internal.h       |  9 +++++++++
>>   mm/memory-failure.c | 43 ++++++++++++++++++++++++++-----------------
>>   2 files changed, 35 insertions(+), 17 deletions(-)
>>
>> diff --git a/mm/internal.h b/mm/internal.h
>> index d7aac802efa5..74490b8ac63d 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -1045,6 +1045,8 @@ static inline int find_next_best_node(int node, nodemask_t *used_node_mask)
>>   /*
>>    * mm/memory-failure.c
>>    */
>> +#ifdef CONFIG_MEMORY_FAILURE
>> +int unmap_posioned_folio(struct folio *folio, enum ttu_flags ttu);
>>   void shake_folio(struct folio *folio);
>>   extern int hwpoison_filter(struct page *p);
>>   
>> @@ -1065,6 +1067,13 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p,
>>   		     unsigned long ksm_addr);
>>   unsigned long page_mapped_in_vma(struct page *page, struct vm_area_struct *vma);
>>   
>> +#else
>> +static inline int unmap_posioned_folio(struct folio *folio, enum ttu_flags ttu)
>> +{
>> +	return 0;
>> +}
>> +#endif
>> +
>>   extern unsigned long  __must_check vm_mmap_pgoff(struct file *, unsigned long,
>>           unsigned long, unsigned long,
>>           unsigned long, unsigned long);
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 353254537b54..93848330de1f 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1554,6 +1554,30 @@ static int get_hwpoison_page(struct page *p, unsigned long flags)
>>   	return ret;
>>   }
>>   
>> +int unmap_posioned_folio(struct folio *folio, enum ttu_flags ttu)
> 
> Should it be unmap_poisoned_folio ? i.e. s/posioned/poisoned/ :)

Oh, my bad.

> 
>> +{
>> +	if (folio_test_hugetlb(folio) && !folio_test_anon(folio)) {
>> +		struct address_space *mapping;
>> +		/*
>> +		 * For hugetlb pages in shared mappings, try_to_unmap
>> +		 * could potentially call huge_pmd_unshare.  Because of
>> +		 * this, take semaphore in write mode here and set
>> +		 * TTU_RMAP_LOCKED to indicate we have taken the lock
>> +		 * at this higher level.
>> +		 */
>> +		mapping = hugetlb_folio_mapping_lock_write(folio);
>> +		if (!mapping)
>> +			return -EAGAIN;
>> +
>> +		try_to_unmap(folio, ttu|TTU_RMAP_LOCKED);
>> +		i_mmap_unlock_write(mapping);
>> +	} else {
>> +		try_to_unmap(folio, ttu);
>> +	}
>> +
>> +	return 0;
> 
> It seems return value is not really needed? We will check folio_mapped below.

It is just to keep the same as before, but

> 
>> +}
>> +
>>   /*
>>    * Do all that is necessary to remove user space mappings. Unmap
>>    * the pages and send SIGBUS to the processes if the data was dirty.
>> @@ -1615,23 +1639,8 @@ static bool hwpoison_user_mappings(struct folio *folio, struct page *p,
>>   	 */
>>   	collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>>   
>> -	if (folio_test_hugetlb(folio) && !folio_test_anon(folio)) {
>> -		/*
>> -		 * For hugetlb pages in shared mappings, try_to_unmap
>> -		 * could potentially call huge_pmd_unshare.  Because of
>> -		 * this, take semaphore in write mode here and set
>> -		 * TTU_RMAP_LOCKED to indicate we have taken the lock
>> -		 * at this higher level.
>> -		 */
>> -		mapping = hugetlb_folio_mapping_lock_write(folio);
>> -		if (mapping) {
>> -			try_to_unmap(folio, ttu|TTU_RMAP_LOCKED);
>> -			i_mmap_unlock_write(mapping);
>> -		} else
>> -			pr_info("%#lx: could not lock mapping for mapped huge page\n", pfn);
>> -	} else {
>> -		try_to_unmap(folio, ttu);
>> -	}
>> +	if (unmap_posioned_folio(folio, ttu))
>> +		pr_info("%#lx: could not lock mapping for mapped huge page\n", pfn);
> 
> It might be better to pr_info inside the unmap_posioned_folio? unmap_posioned_folio might fail due
> to other reasons in the future anyway.

think it again, we have another check and print when recheck
folio_mapped(), so killing the return value and this print for hugepage
is better, and in memory-hotplug, we don't pr_warn for this case.

I will remove return and this print if no objection.

> 
> Thanks.
> .

[PATCH v2 3/5] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[Kefeng Wang]

The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
pages to be offlined") don't handle the hugetlb pages, the endless
loop still occur if offline a hwpoison hugetlb, luckly, with the
commit e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove
memory section with hwpoisoned hugepage") section with hwpoisoned
hugepage"), the HPageMigratable of hugetlb page will be clear, and
the hwpoison hugetlb page will be skipped in scan_movable_pages(),
so the endless loop issue is fixed.

However if the HPageMigratable() check passed(without reference and
lock), the hugetlb page may be hwpoisoned, it won't cause issue since
the hwpoisoned page will be handled correctly in the next movable
pages scan loop, and it will be isolated in do_migrate_range() but
fails to migrate. In order to avoid the unnecessary isolation and
unify all hwpoisoned page handling, let's unconditionally check hwpoison
firstly, and if it is a hwpoisoned hugetlb page, try to unmap it as
the catch all safety net like normal page does.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/memory_hotplug.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index dc19b0e28fbc..02a0d4fbc3fe 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1793,13 +1793,8 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		 * but out loop could handle that as it revisits the split
 		 * folio later.
 		 */
-		if (folio_test_large(folio)) {
+		if (folio_test_large(folio))
 			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
-			if (folio_test_hugetlb(folio)) {
-				isolate_hugetlb(folio, &source);
-				continue;
-			}
-		}
 
 		/*
 		 * HWPoison pages have elevated reference counts so the migration would
@@ -1808,11 +1803,17 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		 * (e.g. current hwpoison implementation doesn't unmap KSM pages but keep
 		 * the unmap as the catch all safety net).
 		 */
-		if (PageHWPoison(page)) {
+		if (folio_test_hwpoison(folio) ||
+		    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
 			if (WARN_ON(folio_test_lru(folio)))
 				folio_isolate_lru(folio);
 			if (folio_mapped(folio))
-				try_to_unmap(folio, TTU_IGNORE_MLOCK);
+				unmap_posioned_folio(folio, TTU_IGNORE_MLOCK);
+			continue;
+		}
+
+		if (folio_test_hugetlb(folio)) {
+			isolate_hugetlb(folio, &source);
 			continue;
 		}
 
-- 
2.27.0

Re: [PATCH v2 3/5] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[Miaohe Lin]

On 2024/8/17 16:49, Kefeng Wang wrote:
> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
> pages to be offlined") don't handle the hugetlb pages, the endless
> loop still occur if offline a hwpoison hugetlb, luckly, with the
> commit e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove
> memory section with hwpoisoned hugepage") section with hwpoisoned
> hugepage"), the HPageMigratable of hugetlb page will be clear, and

It should be commit e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove memory section
with hwpoisoned hugepage")? Above "section with hwpoisoned")" is duplicated.

Also s/be clear/be cleared/ ?

> the hwpoison hugetlb page will be skipped in scan_movable_pages(),
> so the endless loop issue is fixed.
> 
> However if the HPageMigratable() check passed(without reference and
> lock), the hugetlb page may be hwpoisoned, it won't cause issue since
> the hwpoisoned page will be handled correctly in the next movable
> pages scan loop, and it will be isolated in do_migrate_range() but
> fails to migrate. In order to avoid the unnecessary isolation and
> unify all hwpoisoned page handling, let's unconditionally check hwpoison
> firstly, and if it is a hwpoisoned hugetlb page, try to unmap it as
> the catch all safety net like normal page does.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  mm/memory_hotplug.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index dc19b0e28fbc..02a0d4fbc3fe 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1793,13 +1793,8 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  		 * but out loop could handle that as it revisits the split
>  		 * folio later.
>  		 */
> -		if (folio_test_large(folio)) {
> +		if (folio_test_large(folio))
>  			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
> -			if (folio_test_hugetlb(folio)) {
> -				isolate_hugetlb(folio, &source);
> -				continue;
> -			}
> -		}
>  
>  		/*
>  		 * HWPoison pages have elevated reference counts so the migration would
> @@ -1808,11 +1803,17 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  		 * (e.g. current hwpoison implementation doesn't unmap KSM pages but keep
>  		 * the unmap as the catch all safety net).
>  		 */
> -		if (PageHWPoison(page)) {
> +		if (folio_test_hwpoison(folio) ||
> +		    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>  			if (WARN_ON(folio_test_lru(folio)))
>  				folio_isolate_lru(folio);
>  			if (folio_mapped(folio))
> -				try_to_unmap(folio, TTU_IGNORE_MLOCK);
> +				unmap_posioned_folio(folio, TTU_IGNORE_MLOCK);
> +			continue;
> +		}
> +
> +		if (folio_test_hugetlb(folio)) {
> +			isolate_hugetlb(folio, &source);

While you're here, should we pr_warn "failed to isolate pfn xx" for hugetlb folios too as
we already done for raw pages and thp folios?

Thanks.
.

Re: [PATCH v2 3/5] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[Kefeng Wang]

On 2024/8/22 14:52, Miaohe Lin wrote:
> On 2024/8/17 16:49, Kefeng Wang wrote:
>> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
>> pages to be offlined") don't handle the hugetlb pages, the endless
>> loop still occur if offline a hwpoison hugetlb, luckly, with the
>> commit e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove
>> memory section with hwpoisoned hugepage") section with hwpoisoned
>> hugepage"), the HPageMigratable of hugetlb page will be clear, and
> 
> It should be commit e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove memory section
> with hwpoisoned hugepage")? Above "section with hwpoisoned")" is duplicated.
> 
> Also s/be clear/be cleared/ ?

Acked, thanks for carefully review.

> 
>> the hwpoison hugetlb page will be skipped in scan_movable_pages(),
>> so the endless loop issue is fixed.
>>
>> However if the HPageMigratable() check passed(without reference and
>> lock), the hugetlb page may be hwpoisoned, it won't cause issue since
>> the hwpoisoned page will be handled correctly in the next movable
>> pages scan loop, and it will be isolated in do_migrate_range() but
>> fails to migrate. In order to avoid the unnecessary isolation and
>> unify all hwpoisoned page handling, let's unconditionally check hwpoison
>> firstly, and if it is a hwpoisoned hugetlb page, try to unmap it as
>> the catch all safety net like normal page does.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/memory_hotplug.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index dc19b0e28fbc..02a0d4fbc3fe 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1793,13 +1793,8 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>   		 * but out loop could handle that as it revisits the split
>>   		 * folio later.
>>   		 */
>> -		if (folio_test_large(folio)) {
>> +		if (folio_test_large(folio))
>>   			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>> -			if (folio_test_hugetlb(folio)) {
>> -				isolate_hugetlb(folio, &source);
>> -				continue;
>> -			}
>> -		}
>>   
>>   		/*
>>   		 * HWPoison pages have elevated reference counts so the migration would
>> @@ -1808,11 +1803,17 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>   		 * (e.g. current hwpoison implementation doesn't unmap KSM pages but keep
>>   		 * the unmap as the catch all safety net).
>>   		 */
>> -		if (PageHWPoison(page)) {
>> +		if (folio_test_hwpoison(folio) ||
>> +		    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>>   			if (WARN_ON(folio_test_lru(folio)))
>>   				folio_isolate_lru(folio);
>>   			if (folio_mapped(folio))
>> -				try_to_unmap(folio, TTU_IGNORE_MLOCK);
>> +				unmap_posioned_folio(folio, TTU_IGNORE_MLOCK);
>> +			continue;
>> +		}
>> +
>> +		if (folio_test_hugetlb(folio)) {
>> +			isolate_hugetlb(folio, &source);
> 
> While you're here, should we pr_warn "failed to isolate pfn xx" for hugetlb folios too as
> we already done for raw pages and thp folios?

We will unify folio isolation in final patch, which will print warn for 
hugetlb folio when failed to isolate, so no need to add here.
> 
> Thanks.
> .

Re: [PATCH v2 3/5] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[David Hildenbrand]

On 17.08.24 10:49, Kefeng Wang wrote:
> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
> pages to be offlined") don't handle the hugetlb pages, the endless
> loop still occur if offline a hwpoison hugetlb, luckly, with the
> commit e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove
> memory section with hwpoisoned hugepage") section with hwpoisoned
> hugepage"), the HPageMigratable of hugetlb page will be clear, and
> the hwpoison hugetlb page will be skipped in scan_movable_pages(),
> so the endless loop issue is fixed.
> 
> However if the HPageMigratable() check passed(without reference and
> lock), the hugetlb page may be hwpoisoned, it won't cause issue since
> the hwpoisoned page will be handled correctly in the next movable
> pages scan loop, and it will be isolated in do_migrate_range() but
> fails to migrate. In order to avoid the unnecessary isolation and
> unify all hwpoisoned page handling, let's unconditionally check hwpoison
> firstly, and if it is a hwpoisoned hugetlb page, try to unmap it as
> the catch all safety net like normal page does.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   mm/memory_hotplug.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index dc19b0e28fbc..02a0d4fbc3fe 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1793,13 +1793,8 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   		 * but out loop could handle that as it revisits the split
>   		 * folio later.
>   		 */
> -		if (folio_test_large(folio)) {
> +		if (folio_test_large(folio))
>   			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
> -			if (folio_test_hugetlb(folio)) {
> -				isolate_hugetlb(folio, &source);
> -				continue;
> -			}
> -		}
>   
>   		/*
>   		 * HWPoison pages have elevated reference counts so the migration would
> @@ -1808,11 +1803,17 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   		 * (e.g. current hwpoison implementation doesn't unmap KSM pages but keep
>   		 * the unmap as the catch all safety net).
>   		 */
> -		if (PageHWPoison(page)) {
> +		if (folio_test_hwpoison(folio) ||
> +		    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {

We have the exact same check already in mm/shmem.c now.

Likely this should be factored out ... but no idea what function name we 
should use that won't add even more confusion :D

>   			if (WARN_ON(folio_test_lru(folio)))
>   				folio_isolate_lru(folio);
>   			if (folio_mapped(folio))
> -				try_to_unmap(folio, TTU_IGNORE_MLOCK);
> +				unmap_posioned_folio(folio, TTU_IGNORE_MLOCK);
> +			continue;
> +		}
> +
> +		if (folio_test_hugetlb(folio)) {
> +			isolate_hugetlb(folio, &source);
>   			continue;
>   		}
>   

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/5] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[Kefeng Wang]

On 2024/8/26 22:46, David Hildenbrand wrote:
> On 17.08.24 10:49, Kefeng Wang wrote:
>> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
>> pages to be offlined") don't handle the hugetlb pages, the endless
>> loop still occur if offline a hwpoison hugetlb, luckly, with the
>> commit e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove
>> memory section with hwpoisoned hugepage") section with hwpoisoned
>> hugepage"), the HPageMigratable of hugetlb page will be clear, and
>> the hwpoison hugetlb page will be skipped in scan_movable_pages(),
>> so the endless loop issue is fixed.
>>
>> However if the HPageMigratable() check passed(without reference and
>> lock), the hugetlb page may be hwpoisoned, it won't cause issue since
>> the hwpoisoned page will be handled correctly in the next movable
>> pages scan loop, and it will be isolated in do_migrate_range() but
>> fails to migrate. In order to avoid the unnecessary isolation and
>> unify all hwpoisoned page handling, let's unconditionally check hwpoison
>> firstly, and if it is a hwpoisoned hugetlb page, try to unmap it as
>> the catch all safety net like normal page does.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/memory_hotplug.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index dc19b0e28fbc..02a0d4fbc3fe 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1793,13 +1793,8 @@ static void do_migrate_range(unsigned long 
>> start_pfn, unsigned long end_pfn)
>>            * but out loop could handle that as it revisits the split
>>            * folio later.
>>            */
>> -        if (folio_test_large(folio)) {
>> +        if (folio_test_large(folio))
>>               pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>> -            if (folio_test_hugetlb(folio)) {
>> -                isolate_hugetlb(folio, &source);
>> -                continue;
>> -            }
>> -        }
>>           /*
>>            * HWPoison pages have elevated reference counts so the 
>> migration would
>> @@ -1808,11 +1803,17 @@ static void do_migrate_range(unsigned long 
>> start_pfn, unsigned long end_pfn)
>>            * (e.g. current hwpoison implementation doesn't unmap KSM 
>> pages but keep
>>            * the unmap as the catch all safety net).
>>            */
>> -        if (PageHWPoison(page)) {
>> +        if (folio_test_hwpoison(folio) ||
>> +            (folio_test_large(folio) && 
>> folio_test_has_hwpoisoned(folio))) {
> 
> We have the exact same check already in mm/shmem.c now.
> 
> Likely this should be factored out ... but no idea what function name we 
> should use that won't add even more confusion :D

Maybe folio_has_hwpoison(), and Miaohe may have some suggestion,
but leave it for later.

> 
>>               if (WARN_ON(folio_test_lru(folio)))
>>                   folio_isolate_lru(folio);
>>               if (folio_mapped(folio))
>> -                try_to_unmap(folio, TTU_IGNORE_MLOCK);
>> +                unmap_posioned_folio(folio, TTU_IGNORE_MLOCK);
>> +            continue;
>> +        }
>> +
>> +        if (folio_test_hugetlb(folio)) {
>> +            isolate_hugetlb(folio, &source);
>>               continue;
>>           }
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 

Thanks.

Re: [PATCH v2 3/5] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[Miaohe Lin]

On 2024/8/27 9:13, Kefeng Wang wrote:
> 
> 
> On 2024/8/26 22:46, David Hildenbrand wrote:
>> On 17.08.24 10:49, Kefeng Wang wrote:
>>> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
>>> pages to be offlined") don't handle the hugetlb pages, the endless
>>> loop still occur if offline a hwpoison hugetlb, luckly, with the
>>> commit e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove
>>> memory section with hwpoisoned hugepage") section with hwpoisoned
>>> hugepage"), the HPageMigratable of hugetlb page will be clear, and
>>> the hwpoison hugetlb page will be skipped in scan_movable_pages(),
>>> so the endless loop issue is fixed.
>>>
>>> However if the HPageMigratable() check passed(without reference and
>>> lock), the hugetlb page may be hwpoisoned, it won't cause issue since
>>> the hwpoisoned page will be handled correctly in the next movable
>>> pages scan loop, and it will be isolated in do_migrate_range() but
>>> fails to migrate. In order to avoid the unnecessary isolation and
>>> unify all hwpoisoned page handling, let's unconditionally check hwpoison
>>> firstly, and if it is a hwpoisoned hugetlb page, try to unmap it as
>>> the catch all safety net like normal page does.
>>>
>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> ---
>>>   mm/memory_hotplug.c | 17 +++++++++--------
>>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index dc19b0e28fbc..02a0d4fbc3fe 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1793,13 +1793,8 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>>            * but out loop could handle that as it revisits the split
>>>            * folio later.
>>>            */
>>> -        if (folio_test_large(folio)) {
>>> +        if (folio_test_large(folio))
>>>               pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>> -            if (folio_test_hugetlb(folio)) {
>>> -                isolate_hugetlb(folio, &source);
>>> -                continue;
>>> -            }
>>> -        }
>>>           /*
>>>            * HWPoison pages have elevated reference counts so the migration would
>>> @@ -1808,11 +1803,17 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>>            * (e.g. current hwpoison implementation doesn't unmap KSM pages but keep
>>>            * the unmap as the catch all safety net).
>>>            */
>>> -        if (PageHWPoison(page)) {
>>> +        if (folio_test_hwpoison(folio) ||
>>> +            (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>>
>> We have the exact same check already in mm/shmem.c now.
>>
>> Likely this should be factored out ... but no idea what function name we should use that won't add even more confusion :D
> 
> Maybe folio_has_hwpoison(), and Miaohe may have some suggestion,
> but leave it for later.

Will it be suitable to be named as something like folio_contain_hwpoisoned_page?

Thanks.
.

Re: [PATCH v2 3/5] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[David Hildenbrand]

On 27.08.24 04:12, Miaohe Lin wrote:
> On 2024/8/27 9:13, Kefeng Wang wrote:
>>
>>
>> On 2024/8/26 22:46, David Hildenbrand wrote:
>>> On 17.08.24 10:49, Kefeng Wang wrote:
>>>> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
>>>> pages to be offlined") don't handle the hugetlb pages, the endless
>>>> loop still occur if offline a hwpoison hugetlb, luckly, with the
>>>> commit e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove
>>>> memory section with hwpoisoned hugepage") section with hwpoisoned
>>>> hugepage"), the HPageMigratable of hugetlb page will be clear, and
>>>> the hwpoison hugetlb page will be skipped in scan_movable_pages(),
>>>> so the endless loop issue is fixed.
>>>>
>>>> However if the HPageMigratable() check passed(without reference and
>>>> lock), the hugetlb page may be hwpoisoned, it won't cause issue since
>>>> the hwpoisoned page will be handled correctly in the next movable
>>>> pages scan loop, and it will be isolated in do_migrate_range() but
>>>> fails to migrate. In order to avoid the unnecessary isolation and
>>>> unify all hwpoisoned page handling, let's unconditionally check hwpoison
>>>> firstly, and if it is a hwpoisoned hugetlb page, try to unmap it as
>>>> the catch all safety net like normal page does.
>>>>
>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>> ---
>>>>    mm/memory_hotplug.c | 17 +++++++++--------
>>>>    1 file changed, 9 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>>> index dc19b0e28fbc..02a0d4fbc3fe 100644
>>>> --- a/mm/memory_hotplug.c
>>>> +++ b/mm/memory_hotplug.c
>>>> @@ -1793,13 +1793,8 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>>>             * but out loop could handle that as it revisits the split
>>>>             * folio later.
>>>>             */
>>>> -        if (folio_test_large(folio)) {
>>>> +        if (folio_test_large(folio))
>>>>                pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>>> -            if (folio_test_hugetlb(folio)) {
>>>> -                isolate_hugetlb(folio, &source);
>>>> -                continue;
>>>> -            }
>>>> -        }
>>>>            /*
>>>>             * HWPoison pages have elevated reference counts so the migration would
>>>> @@ -1808,11 +1803,17 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>>>             * (e.g. current hwpoison implementation doesn't unmap KSM pages but keep
>>>>             * the unmap as the catch all safety net).
>>>>             */
>>>> -        if (PageHWPoison(page)) {
>>>> +        if (folio_test_hwpoison(folio) ||
>>>> +            (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>>>
>>> We have the exact same check already in mm/shmem.c now.
>>>
>>> Likely this should be factored out ... but no idea what function name we should use that won't add even more confusion :D
>>
>> Maybe folio_has_hwpoison(), and Miaohe may have some suggestion,
>> but leave it for later.
> 
> Will it be suitable to be named as something like folio_contain_hwpoisoned_page?

That sounds much better. "folio_has_hwpoison" is way to similar to 
"folio_test_has_hwpoisoned".

-- 
Cheers,

David / dhildenb

[PATCH v2 4/5] mm: migrate: add isolate_folio_to_list()

[Kefeng Wang]

Add isolate_folio_to_list() helper to try to isolate HugeTLB,
no-LRU movable and LRU folios to a list, which will be reused by
do_migrate_range() from memory hotplug soon, also drop the
mf_isolate_folio() since we could directly use new helper in
the soft_offline_in_use_page().

Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/linux/migrate.h |  3 +++
 mm/memory-failure.c     | 46 ++++++++++-------------------------------
 mm/migrate.c            | 27 ++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 35 deletions(-)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 644be30b69c8..002e49b2ebd9 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -70,6 +70,7 @@ int migrate_pages(struct list_head *l, new_folio_t new, free_folio_t free,
 		  unsigned int *ret_succeeded);
 struct folio *alloc_migration_target(struct folio *src, unsigned long private);
 bool isolate_movable_page(struct page *page, isolate_mode_t mode);
+bool isolate_folio_to_list(struct folio *folio, struct list_head *list);
 
 int migrate_huge_page_move_mapping(struct address_space *mapping,
 		struct folio *dst, struct folio *src);
@@ -91,6 +92,8 @@ static inline struct folio *alloc_migration_target(struct folio *src,
 	{ return NULL; }
 static inline bool isolate_movable_page(struct page *page, isolate_mode_t mode)
 	{ return false; }
+static inline bool isolate_folio_to_list(struct folio *folio, struct list_head *list)
+	{ return false; }
 
 static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
 				  struct folio *dst, struct folio *src)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 93848330de1f..d8298017bd99 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2659,40 +2659,6 @@ EXPORT_SYMBOL(unpoison_memory);
 #undef pr_fmt
 #define pr_fmt(fmt) "Soft offline: " fmt
 
-static bool mf_isolate_folio(struct folio *folio, struct list_head *pagelist)
-{
-	bool isolated = false;
-
-	if (folio_test_hugetlb(folio)) {
-		isolated = isolate_hugetlb(folio, pagelist);
-	} else {
-		bool lru = !__folio_test_movable(folio);
-
-		if (lru)
-			isolated = folio_isolate_lru(folio);
-		else
-			isolated = isolate_movable_page(&folio->page,
-							ISOLATE_UNEVICTABLE);
-
-		if (isolated) {
-			list_add(&folio->lru, pagelist);
-			if (lru)
-				node_stat_add_folio(folio, NR_ISOLATED_ANON +
-						    folio_is_file_lru(folio));
-		}
-	}
-
-	/*
-	 * If we succeed to isolate the folio, we grabbed another refcount on
-	 * the folio, so we can safely drop the one we got from get_any_page().
-	 * If we failed to isolate the folio, it means that we cannot go further
-	 * and we will return an error, so drop the reference we got from
-	 * get_any_page() as well.
-	 */
-	folio_put(folio);
-	return isolated;
-}
-
 /*
  * soft_offline_in_use_page handles hugetlb-pages and non-hugetlb pages.
  * If the page is a non-dirty unmapped page-cache page, it simply invalidates.
@@ -2744,7 +2710,7 @@ static int soft_offline_in_use_page(struct page *page)
 		return 0;
 	}
 
-	if (mf_isolate_folio(folio, &pagelist)) {
+	if (isolate_folio_to_list(folio, &pagelist)) {
 		ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
 			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, NULL);
 		if (!ret) {
@@ -2766,6 +2732,16 @@ static int soft_offline_in_use_page(struct page *page)
 			pfn, msg_page[huge], page_count(page), &page->flags);
 		ret = -EBUSY;
 	}
+
+	/*
+	 * If we succeed to isolate the folio, we grabbed another refcount on
+	 * the folio, so we can safely drop the one we got from get_any_page().
+	 * If we failed to isolate the folio, it means that we cannot go further
+	 * and we will return an error, so drop the reference we got from
+	 * get_any_page() as well.
+	 */
+	folio_put(folio);
+
 	return ret;
 }
 
diff --git a/mm/migrate.c b/mm/migrate.c
index dbfa910ec24b..53f8429a8ebe 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -178,6 +178,33 @@ void putback_movable_pages(struct list_head *l)
 	}
 }
 
+/* Must be called with an elevated refcount on the non-hugetlb folio */
+bool isolate_folio_to_list(struct folio *folio, struct list_head *list)
+{
+	bool isolated = false;
+
+	if (folio_test_hugetlb(folio)) {
+		isolated = isolate_hugetlb(folio, list);
+	} else {
+		bool lru = !__folio_test_movable(folio);
+
+		if (lru)
+			isolated = folio_isolate_lru(folio);
+		else
+			isolated = isolate_movable_page(&folio->page,
+							ISOLATE_UNEVICTABLE);
+
+		if (isolated) {
+			list_add(&folio->lru, list);
+			if (lru)
+				node_stat_add_folio(folio, NR_ISOLATED_ANON +
+						    folio_is_file_lru(folio));
+		}
+	}
+
+	return isolated;
+}
+
 static bool try_to_map_unused_to_zeropage(struct page_vma_mapped_walk *pvmw,
 					  struct folio *folio,
 					  unsigned long idx)
-- 
2.27.0

Re: [PATCH v2 4/5] mm: migrate: add isolate_folio_to_list()

[Miaohe Lin]

On 2024/8/17 16:49, Kefeng Wang wrote:
> Add isolate_folio_to_list() helper to try to isolate HugeTLB,
> no-LRU movable and LRU folios to a list, which will be reused by
> do_migrate_range() from memory hotplug soon, also drop the
> mf_isolate_folio() since we could directly use new helper in
> the soft_offline_in_use_page().
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>

Thanks for your patch.

> ---
>  include/linux/migrate.h |  3 +++
>  mm/memory-failure.c     | 46 ++++++++++-------------------------------
>  mm/migrate.c            | 27 ++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index 644be30b69c8..002e49b2ebd9 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -70,6 +70,7 @@ int migrate_pages(struct list_head *l, new_folio_t new, free_folio_t free,
>  		  unsigned int *ret_succeeded);
>  struct folio *alloc_migration_target(struct folio *src, unsigned long private);
>  bool isolate_movable_page(struct page *page, isolate_mode_t mode);
> +bool isolate_folio_to_list(struct folio *folio, struct list_head *list);
>  
>  int migrate_huge_page_move_mapping(struct address_space *mapping,
>  		struct folio *dst, struct folio *src);
> @@ -91,6 +92,8 @@ static inline struct folio *alloc_migration_target(struct folio *src,
>  	{ return NULL; }
>  static inline bool isolate_movable_page(struct page *page, isolate_mode_t mode)
>  	{ return false; }
> +static inline bool isolate_folio_to_list(struct folio *folio, struct list_head *list)
> +	{ return false; }
>  
>  static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
>  				  struct folio *dst, struct folio *src)
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 93848330de1f..d8298017bd99 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2659,40 +2659,6 @@ EXPORT_SYMBOL(unpoison_memory);
>  #undef pr_fmt
>  #define pr_fmt(fmt) "Soft offline: " fmt
>  
> -static bool mf_isolate_folio(struct folio *folio, struct list_head *pagelist)
> -{
> -	bool isolated = false;
> -
> -	if (folio_test_hugetlb(folio)) {
> -		isolated = isolate_hugetlb(folio, pagelist);
> -	} else {
> -		bool lru = !__folio_test_movable(folio);
> -
> -		if (lru)
> -			isolated = folio_isolate_lru(folio);
> -		else
> -			isolated = isolate_movable_page(&folio->page,
> -							ISOLATE_UNEVICTABLE);
> -
> -		if (isolated) {
> -			list_add(&folio->lru, pagelist);
> -			if (lru)
> -				node_stat_add_folio(folio, NR_ISOLATED_ANON +
> -						    folio_is_file_lru(folio));
> -		}
> -	}
> -
> -	/*
> -	 * If we succeed to isolate the folio, we grabbed another refcount on
> -	 * the folio, so we can safely drop the one we got from get_any_page().
> -	 * If we failed to isolate the folio, it means that we cannot go further
> -	 * and we will return an error, so drop the reference we got from
> -	 * get_any_page() as well.
> -	 */
> -	folio_put(folio);
> -	return isolated;
> -}
> -
>  /*
>   * soft_offline_in_use_page handles hugetlb-pages and non-hugetlb pages.
>   * If the page is a non-dirty unmapped page-cache page, it simply invalidates.
> @@ -2744,7 +2710,7 @@ static int soft_offline_in_use_page(struct page *page)
>  		return 0;
>  	}
>  
> -	if (mf_isolate_folio(folio, &pagelist)) {
> +	if (isolate_folio_to_list(folio, &pagelist)) {
>  		ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
>  			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, NULL);
>  		if (!ret) {
> @@ -2766,6 +2732,16 @@ static int soft_offline_in_use_page(struct page *page)
>  			pfn, msg_page[huge], page_count(page), &page->flags);
>  		ret = -EBUSY;
>  	}
> +
> +	/*
> +	 * If we succeed to isolate the folio, we grabbed another refcount on
> +	 * the folio, so we can safely drop the one we got from get_any_page().
> +	 * If we failed to isolate the folio, it means that we cannot go further
> +	 * and we will return an error, so drop the reference we got from
> +	 * get_any_page() as well.
> +	 */
> +	folio_put(folio);

Why folio_put() is deferred here? With this change, folio will have extra two refcnt when
calling migrate_pages() above. One is from get_any_page() and another one from folio_isolate_lru().
This would lead to migrate_pages() never success. And my many testcases failed due to this
change.

Thanks.
.

Re: [PATCH v2 4/5] mm: migrate: add isolate_folio_to_list()

[Kefeng Wang]

On 2024/8/20 17:32, Miaohe Lin wrote:
> On 2024/8/17 16:49, Kefeng Wang wrote:
>> Add isolate_folio_to_list() helper to try to isolate HugeTLB,
>> no-LRU movable and LRU folios to a list, which will be reused by
>> do_migrate_range() from memory hotplug soon, also drop the
>> mf_isolate_folio() since we could directly use new helper in
>> the soft_offline_in_use_page().
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> 
> Thanks for your patch.
> 
>> ---
>>   include/linux/migrate.h |  3 +++
>>   mm/memory-failure.c     | 46 ++++++++++-------------------------------
>>   mm/migrate.c            | 27 ++++++++++++++++++++++++
>>   3 files changed, 41 insertions(+), 35 deletions(-)
>>
>> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
>> index 644be30b69c8..002e49b2ebd9 100644
>> --- a/include/linux/migrate.h
>> +++ b/include/linux/migrate.h
>> @@ -70,6 +70,7 @@ int migrate_pages(struct list_head *l, new_folio_t new, free_folio_t free,
>>   		  unsigned int *ret_succeeded);
>>   struct folio *alloc_migration_target(struct folio *src, unsigned long private);
>>   bool isolate_movable_page(struct page *page, isolate_mode_t mode);
>> +bool isolate_folio_to_list(struct folio *folio, struct list_head *list);
>>   
>>   int migrate_huge_page_move_mapping(struct address_space *mapping,
>>   		struct folio *dst, struct folio *src);
>> @@ -91,6 +92,8 @@ static inline struct folio *alloc_migration_target(struct folio *src,
>>   	{ return NULL; }
>>   static inline bool isolate_movable_page(struct page *page, isolate_mode_t mode)
>>   	{ return false; }
>> +static inline bool isolate_folio_to_list(struct folio *folio, struct list_head *list)
>> +	{ return false; }
>>   
>>   static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
>>   				  struct folio *dst, struct folio *src)
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 93848330de1f..d8298017bd99 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -2659,40 +2659,6 @@ EXPORT_SYMBOL(unpoison_memory);
>>   #undef pr_fmt
>>   #define pr_fmt(fmt) "Soft offline: " fmt
>>   
>> -static bool mf_isolate_folio(struct folio *folio, struct list_head *pagelist)
>> -{
>> -	bool isolated = false;
>> -
>> -	if (folio_test_hugetlb(folio)) {
>> -		isolated = isolate_hugetlb(folio, pagelist);
>> -	} else {
>> -		bool lru = !__folio_test_movable(folio);
>> -
>> -		if (lru)
>> -			isolated = folio_isolate_lru(folio);
>> -		else
>> -			isolated = isolate_movable_page(&folio->page,
>> -							ISOLATE_UNEVICTABLE);
>> -
>> -		if (isolated) {
>> -			list_add(&folio->lru, pagelist);
>> -			if (lru)
>> -				node_stat_add_folio(folio, NR_ISOLATED_ANON +
>> -						    folio_is_file_lru(folio));
>> -		}
>> -	}
>> -
>> -	/*
>> -	 * If we succeed to isolate the folio, we grabbed another refcount on
>> -	 * the folio, so we can safely drop the one we got from get_any_page().
>> -	 * If we failed to isolate the folio, it means that we cannot go further
>> -	 * and we will return an error, so drop the reference we got from
>> -	 * get_any_page() as well.
>> -	 */
>> -	folio_put(folio);
>> -	return isolated;
>> -}
>> -
>>   /*
>>    * soft_offline_in_use_page handles hugetlb-pages and non-hugetlb pages.
>>    * If the page is a non-dirty unmapped page-cache page, it simply invalidates.
>> @@ -2744,7 +2710,7 @@ static int soft_offline_in_use_page(struct page *page)
>>   		return 0;
>>   	}
>>   
>> -	if (mf_isolate_folio(folio, &pagelist)) {
>> +	if (isolate_folio_to_list(folio, &pagelist)) {
>>   		ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
>>   			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, NULL);
>>   		if (!ret) {
>> @@ -2766,6 +2732,16 @@ static int soft_offline_in_use_page(struct page *page)
>>   			pfn, msg_page[huge], page_count(page), &page->flags);
>>   		ret = -EBUSY;
>>   	}
>> +
>> +	/*
>> +	 * If we succeed to isolate the folio, we grabbed another refcount on
>> +	 * the folio, so we can safely drop the one we got from get_any_page().
>> +	 * If we failed to isolate the folio, it means that we cannot go further
>> +	 * and we will return an error, so drop the reference we got from
>> +	 * get_any_page() as well.
>> +	 */
>> +	folio_put(folio);
> 
> Why folio_put() is deferred here? With this change, folio will have extra two refcnt when
> calling migrate_pages() above. One is from get_any_page() and another one from folio_isolate_lru().
> This would lead to migrate_pages() never success. And my many testcases failed due to this
> change.

Thanks for your review, missing this, only test memory-hotplug, could 
you try this,

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index d8298017bd99..64a145a0e29f 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2671,6 +2671,7 @@ static int soft_offline_in_use_page(struct page *page)
         struct folio *folio = page_folio(page);
         char const *msg_page[] = {"page", "hugepage"};
         bool huge = folio_test_hugetlb(folio);
+       bool isolated;
         LIST_HEAD(pagelist);
         struct migration_target_control mtc = {
                 .nid = NUMA_NO_NODE,
@@ -2710,7 +2711,18 @@ static int soft_offline_in_use_page(struct page 
*page)
                 return 0;
         }

-       if (isolate_folio_to_list(folio, &pagelist)) {
+       isolated = isolate_folio_to_list(folio, &pagelist);
+
+       /*
+        * If we succeed to isolate the folio, we grabbed another 
refcount on
+        * the folio, so we can safely drop the one we got from 
get_any_page().
+        * If we failed to isolate the folio, it means that we cannot go 
further
+        * and we will return an error, so drop the reference we got from
+        * get_any_page() as well.
+        */
+       folio_put(folio);
+
+       if (isolated) {
                 ret = migrate_pages(&pagelist, alloc_migration_target, 
NULL,
                         (unsigned long)&mtc, MIGRATE_SYNC, 
MR_MEMORY_FAILURE, NULL);
                 if (!ret) {
@@ -2733,15 +2745,6 @@ static int soft_offline_in_use_page(struct page 
*page)
                 ret = -EBUSY;
         }

-       /*
-        * If we succeed to isolate the folio, we grabbed another 
refcount on
-        * the folio, so we can safely drop the one we got from 
get_any_page().
-        * If we failed to isolate the folio, it means that we cannot go 
further
-        * and we will return an error, so drop the reference we got from
-        * get_any_page() as well.
-        */
-       folio_put(folio);
-
         return ret;
  }


> 
> Thanks.
> .

Re: [PATCH v2 4/5] mm: migrate: add isolate_folio_to_list()

[Miaohe Lin]

On 2024/8/20 17:46, Kefeng Wang wrote:
> 
> 
> On 2024/8/20 17:32, Miaohe Lin wrote:
>> On 2024/8/17 16:49, Kefeng Wang wrote:
>>> Add isolate_folio_to_list() helper to try to isolate HugeTLB,
>>> no-LRU movable and LRU folios to a list, which will be reused by
>>> do_migrate_range() from memory hotplug soon, also drop the
>>> mf_isolate_folio() since we could directly use new helper in
>>> the soft_offline_in_use_page().
>>>
>>> Acked-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>
>> Thanks for your patch.
>>>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index d8298017bd99..64a145a0e29f 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2671,6 +2671,7 @@ static int soft_offline_in_use_page(struct page *page)
>         struct folio *folio = page_folio(page);
>         char const *msg_page[] = {"page", "hugepage"};
>         bool huge = folio_test_hugetlb(folio);
> +       bool isolated;
>         LIST_HEAD(pagelist);
>         struct migration_target_control mtc = {
>                 .nid = NUMA_NO_NODE,
> @@ -2710,7 +2711,18 @@ static int soft_offline_in_use_page(struct page *page)
>                 return 0;
>         }
> 
> -       if (isolate_folio_to_list(folio, &pagelist)) {
> +       isolated = isolate_folio_to_list(folio, &pagelist);
> +
> +       /*
> +        * If we succeed to isolate the folio, we grabbed another refcount on
> +        * the folio, so we can safely drop the one we got from get_any_page().
> +        * If we failed to isolate the folio, it means that we cannot go further
> +        * and we will return an error, so drop the reference we got from
> +        * get_any_page() as well.
> +        */
> +       folio_put(folio);
> +
> +       if (isolated) {
>                 ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
>                         (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, NULL);
>                 if (!ret) {
> @@ -2733,15 +2745,6 @@ static int soft_offline_in_use_page(struct page *page)
>                 ret = -EBUSY;
>         }
> 
> -       /*
> -        * If we succeed to isolate the folio, we grabbed another refcount on
> -        * the folio, so we can safely drop the one we got from get_any_page().
> -        * If we failed to isolate the folio, it means that we cannot go further
> -        * and we will return an error, so drop the reference we got from
> -        * get_any_page() as well.
> -        */
> -       folio_put(folio);
> -
>         return ret;
>  }

This works to me.

Thanks.
.

Re: [PATCH v2 4/5] mm: migrate: add isolate_folio_to_list()

[Kefeng Wang]

On 2024/8/21 10:00, Miaohe Lin wrote:
> On 2024/8/20 17:46, Kefeng Wang wrote:
>>
>>
>> On 2024/8/20 17:32, Miaohe Lin wrote:
>>> On 2024/8/17 16:49, Kefeng Wang wrote:
>>>> Add isolate_folio_to_list() helper to try to isolate HugeTLB,
>>>> no-LRU movable and LRU folios to a list, which will be reused by
>>>> do_migrate_range() from memory hotplug soon, also drop the
>>>> mf_isolate_folio() since we could directly use new helper in
>>>> the soft_offline_in_use_page().
>>>>
>>>> Acked-by: David Hildenbrand <david@redhat.com>
>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>
>>> Thanks for your patch.
>>>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index d8298017bd99..64a145a0e29f 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -2671,6 +2671,7 @@ static int soft_offline_in_use_page(struct page *page)
>>          struct folio *folio = page_folio(page);
>>          char const *msg_page[] = {"page", "hugepage"};
>>          bool huge = folio_test_hugetlb(folio);
>> +       bool isolated;
>>          LIST_HEAD(pagelist);
>>          struct migration_target_control mtc = {
>>                  .nid = NUMA_NO_NODE,
>> @@ -2710,7 +2711,18 @@ static int soft_offline_in_use_page(struct page *page)
>>                  return 0;
>>          }
>>
>> -       if (isolate_folio_to_list(folio, &pagelist)) {
>> +       isolated = isolate_folio_to_list(folio, &pagelist);
>> +
>> +       /*
>> +        * If we succeed to isolate the folio, we grabbed another refcount on
>> +        * the folio, so we can safely drop the one we got from get_any_page().
>> +        * If we failed to isolate the folio, it means that we cannot go further
>> +        * and we will return an error, so drop the reference we got from
>> +        * get_any_page() as well.
>> +        */
>> +       folio_put(folio);
>> +
>> +       if (isolated) {
>>                  ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
>>                          (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, NULL);
>>                  if (!ret) {
>> @@ -2733,15 +2745,6 @@ static int soft_offline_in_use_page(struct page *page)
>>                  ret = -EBUSY;
>>          }
>>
>> -       /*
>> -        * If we succeed to isolate the folio, we grabbed another refcount on
>> -        * the folio, so we can safely drop the one we got from get_any_page().
>> -        * If we failed to isolate the folio, it means that we cannot go further
>> -        * and we will return an error, so drop the reference we got from
>> -        * get_any_page() as well.
>> -        */
>> -       folio_put(folio);
>> -
>>          return ret;
>>   }
> 
> This works to me.

Good, my bad for break.

Andrew，please help to squash above changes, thanks.

> 
> Thanks.
> .

Re: [PATCH v2 4/5] mm: migrate: add isolate_folio_to_list()

[Miaohe Lin]

On 2024/8/21 10:14, Kefeng Wang wrote:
> 
> 
> On 2024/8/21 10:00, Miaohe Lin wrote:
>> On 2024/8/20 17:46, Kefeng Wang wrote:
>>>
>>>
>>> On 2024/8/20 17:32, Miaohe Lin wrote:
>>>> On 2024/8/17 16:49, Kefeng Wang wrote:
>>>>> Add isolate_folio_to_list() helper to try to isolate HugeTLB,
>>>>> no-LRU movable and LRU folios to a list, which will be reused by
>>>>> do_migrate_range() from memory hotplug soon, also drop the
>>>>> mf_isolate_folio() since we could directly use new helper in
>>>>> the soft_offline_in_use_page().
>>>>>
>>>>> Acked-by: David Hildenbrand <david@redhat.com>
>>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>>
>>>> Thanks for your patch.
>>>>>
>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>> index d8298017bd99..64a145a0e29f 100644
>>> --- a/mm/memory-failure.c
>>> +++ b/mm/memory-failure.c
>>> @@ -2671,6 +2671,7 @@ static int soft_offline_in_use_page(struct page *page)
>>>          struct folio *folio = page_folio(page);
>>>          char const *msg_page[] = {"page", "hugepage"};
>>>          bool huge = folio_test_hugetlb(folio);
>>> +       bool isolated;
>>>          LIST_HEAD(pagelist);
>>>          struct migration_target_control mtc = {
>>>                  .nid = NUMA_NO_NODE,
>>> @@ -2710,7 +2711,18 @@ static int soft_offline_in_use_page(struct page *page)
>>>                  return 0;
>>>          }
>>>
>>> -       if (isolate_folio_to_list(folio, &pagelist)) {
>>> +       isolated = isolate_folio_to_list(folio, &pagelist);
>>> +
>>> +       /*
>>> +        * If we succeed to isolate the folio, we grabbed another refcount on
>>> +        * the folio, so we can safely drop the one we got from get_any_page().
>>> +        * If we failed to isolate the folio, it means that we cannot go further
>>> +        * and we will return an error, so drop the reference we got from
>>> +        * get_any_page() as well.
>>> +        */
>>> +       folio_put(folio);
>>> +
>>> +       if (isolated) {
>>>                  ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
>>>                          (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, NULL);
>>>                  if (!ret) {
>>> @@ -2733,15 +2745,6 @@ static int soft_offline_in_use_page(struct page *page)
>>>                  ret = -EBUSY;
>>>          }
>>>
>>> -       /*
>>> -        * If we succeed to isolate the folio, we grabbed another refcount on
>>> -        * the folio, so we can safely drop the one we got from get_any_page().
>>> -        * If we failed to isolate the folio, it means that we cannot go further
>>> -        * and we will return an error, so drop the reference we got from
>>> -        * get_any_page() as well.
>>> -        */
>>> -       folio_put(folio);
>>> -
>>>          return ret;
>>>   }
>>
>> This works to me.
> 
> Good, my bad for break.
> 
> Andrew，please help to squash above changes, thanks.

With above changes added, this patch looks good to me.

Acked-by: Miaohe Lin <linmiaohe@huawei.com>
Tested-by: Miaohe Lin <linmiaohe@huawei.com>

Thanks.
.

Re: [PATCH v2 4/5] mm: migrate: add isolate_folio_to_list()

[David Hildenbrand]

>   
> diff --git a/mm/migrate.c b/mm/migrate.c
> index dbfa910ec24b..53f8429a8ebe 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -178,6 +178,33 @@ void putback_movable_pages(struct list_head *l)
>   	}
>   }
>   
> +/* Must be called with an elevated refcount on the non-hugetlb folio */
> +bool isolate_folio_to_list(struct folio *folio, struct list_head *list)
> +{
> +	bool isolated = false;

No need to initialize this to "false".

> +
> +	if (folio_test_hugetlb(folio)) {
> +		isolated = isolate_hugetlb(folio, list);
> +	} else {
> +		bool lru = !__folio_test_movable(folio);
> +
> +		if (lru)
> +			isolated = folio_isolate_lru(folio);
> +		else
> +			isolated = isolate_movable_page(&folio->page,
> +							ISOLATE_UNEVICTABLE);
> +
> +		if (isolated) {
> +			list_add(&folio->lru, list);
> +			if (lru)
> +				node_stat_add_folio(folio, NR_ISOLATED_ANON +
> +						    folio_is_file_lru(folio));
> +		}
> +	}
> +
> +	return isolated;

Revisiting this patch, we should likely do

bool isolated, lru;

if (folio_test_hugetlb(folio))
	return isolate_hugetlb(folio, list);

lru = !__folio_test_movable(folio);
if (lru)
...

if (!isolated)
	return false;

list_add(&folio->lru, list);
if (lru)
	node_stat_add_folio(folio, NR_ISOLATED_ANON +
			    folio_is_file_lru(folio));
return true;


to avoid one indentation level and clean up the code flow a bit.

> +}
> +
>   static bool try_to_map_unused_to_zeropage(struct page_vma_mapped_walk *pvmw,
>   					  struct folio *folio,
>   					  unsigned long idx)

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 4/5] mm: migrate: add isolate_folio_to_list()

[Kefeng Wang]

On 2024/8/26 22:50, David Hildenbrand wrote:
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index dbfa910ec24b..53f8429a8ebe 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -178,6 +178,33 @@ void putback_movable_pages(struct list_head *l)
>>       }
>>   }
>> +/* Must be called with an elevated refcount on the non-hugetlb folio */
>> +bool isolate_folio_to_list(struct folio *folio, struct list_head *list)
>> +{
>> +    bool isolated = false;
> 
> No need to initialize this to "false".
> 
>> +
>> +    if (folio_test_hugetlb(folio)) {
>> +        isolated = isolate_hugetlb(folio, list);
>> +    } else {
>> +        bool lru = !__folio_test_movable(folio);
>> +
>> +        if (lru)
>> +            isolated = folio_isolate_lru(folio);
>> +        else
>> +            isolated = isolate_movable_page(&folio->page,
>> +                            ISOLATE_UNEVICTABLE);
>> +
>> +        if (isolated) {
>> +            list_add(&folio->lru, list);
>> +            if (lru)
>> +                node_stat_add_folio(folio, NR_ISOLATED_ANON +
>> +                            folio_is_file_lru(folio));
>> +        }
>> +    }
>> +
>> +    return isolated;
> 
> Revisiting this patch, we should likely do
> 
> bool isolated, lru;
> 
> if (folio_test_hugetlb(folio))
>      return isolate_hugetlb(folio, list);
> 
> lru = !__folio_test_movable(folio);
> if (lru)
> ...
> 
> if (!isolated)
>      return false;
> 
> list_add(&folio->lru, list);
> if (lru)
>      node_stat_add_folio(folio, NR_ISOLATED_ANON +
>                  folio_is_file_lru(folio));
> return true;
> 
> 
> to avoid one indentation level and clean up the code flow a bit.

Sure, will rewrite according to above pattern.

> 
>> +}
>> +
>>   static bool try_to_map_unused_to_zeropage(struct 
>> page_vma_mapped_walk *pvmw,
>>                         struct folio *folio,
>>                         unsigned long idx)
> 

[PATCH v2 5/5] mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

[Kefeng Wang]

Use the isolate_folio_to_list() to unify hugetlb/LRU/non-LRU
folio isolation, which cleanup code a bit and save a few calls
to compound_head().

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/memory_hotplug.c | 45 +++++++++++++++++----------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 02a0d4fbc3fe..cc9c16db2f8c 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1773,14 +1773,14 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
 static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 {
 	unsigned long pfn;
-	struct page *page;
 	LIST_HEAD(source);
+	struct folio *folio;
 	static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL,
 				      DEFAULT_RATELIMIT_BURST);
 
 	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
-		struct folio *folio;
-		bool isolated;
+		struct page *page;
+		bool huge;
 
 		if (!pfn_valid(pfn))
 			continue;
@@ -1812,34 +1812,22 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 			continue;
 		}
 
-		if (folio_test_hugetlb(folio)) {
-			isolate_hugetlb(folio, &source);
-			continue;
+		huge = folio_test_hugetlb(folio);
+		if (!huge) {
+			folio = folio_get_nontail_page(page);
+			if (!folio)
+				continue;
 		}
 
-		if (!get_page_unless_zero(page))
-			continue;
-		/*
-		 * We can skip free pages. And we can deal with pages on
-		 * LRU and non-lru movable pages.
-		 */
-		if (PageLRU(page))
-			isolated = isolate_lru_page(page);
-		else
-			isolated = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
-		if (isolated) {
-			list_add_tail(&page->lru, &source);
-			if (!__PageMovable(page))
-				inc_node_page_state(page, NR_ISOLATED_ANON +
-						    page_is_file_lru(page));
-
-		} else {
+		if (!isolate_folio_to_list(folio, &source)) {
 			if (__ratelimit(&migrate_rs)) {
 				pr_warn("failed to isolate pfn %lx\n", pfn);
 				dump_page(page, "isolation failed");
 			}
 		}
-		put_page(page);
+
+		if (!huge)
+			folio_put(folio);
 	}
 	if (!list_empty(&source)) {
 		nodemask_t nmask = node_states[N_MEMORY];
@@ -1854,7 +1842,7 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		 * We have checked that migration range is on a single zone so
 		 * we can use the nid of the first page to all the others.
 		 */
-		mtc.nid = page_to_nid(list_first_entry(&source, struct page, lru));
+		mtc.nid = folio_nid(list_first_entry(&source, struct folio, lru));
 
 		/*
 		 * try to allocate from a different node but reuse this node
@@ -1867,11 +1855,12 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		ret = migrate_pages(&source, alloc_migration_target, NULL,
 			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG, NULL);
 		if (ret) {
-			list_for_each_entry(page, &source, lru) {
+			list_for_each_entry(folio, &source, lru) {
 				if (__ratelimit(&migrate_rs)) {
 					pr_warn("migrating pfn %lx failed ret:%d\n",
-						page_to_pfn(page), ret);
-					dump_page(page, "migration failure");
+						folio_pfn(folio), ret);
+					dump_page(&folio->page,
+						  "migration failure");
 				}
 			}
 			putback_movable_pages(&source);
-- 
2.27.0

Re: [PATCH v2 5/5] mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

[Miaohe Lin]

On 2024/8/17 16:49, Kefeng Wang wrote:
> Use the isolate_folio_to_list() to unify hugetlb/LRU/non-LRU
> folio isolation, which cleanup code a bit and save a few calls
> to compound_head().
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  mm/memory_hotplug.c | 45 +++++++++++++++++----------------------------
>  1 file changed, 17 insertions(+), 28 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 02a0d4fbc3fe..cc9c16db2f8c 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1773,14 +1773,14 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
>  static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  {
>  	unsigned long pfn;
> -	struct page *page;
>  	LIST_HEAD(source);
> +	struct folio *folio;
>  	static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL,
>  				      DEFAULT_RATELIMIT_BURST);
>  
>  	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> -		struct folio *folio;
> -		bool isolated;
> +		struct page *page;
> +		bool huge;
>  
>  		if (!pfn_valid(pfn))
>  			continue;
> @@ -1812,34 +1812,22 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  			continue;
>  		}
>  
> -		if (folio_test_hugetlb(folio)) {
> -			isolate_hugetlb(folio, &source);
> -			continue;
> +		huge = folio_test_hugetlb(folio);
> +		if (!huge) {
> +			folio = folio_get_nontail_page(page);
> +			if (!folio)
> +				continue;
>  		}
>  
> -		if (!get_page_unless_zero(page))

Does page always equal to head? Page is used in most cases in this function and head is only used to calculate pfn.
If not, folio can not simply replace page?

> -			continue;
> -		/*
> -		 * We can skip free pages. And we can deal with pages on
> -		 * LRU and non-lru movable pages.
> -		 */
> -		if (PageLRU(page))

I think this check is incorrect. We should check __PageMovable(page) instead. So this patch
fixes this issue too.

Thanks.
.

Re: [PATCH v2 5/5] mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

[Kefeng Wang]

On 2024/8/22 15:20, Miaohe Lin wrote:
> On 2024/8/17 16:49, Kefeng Wang wrote:
>> Use the isolate_folio_to_list() to unify hugetlb/LRU/non-LRU
>> folio isolation, which cleanup code a bit and save a few calls
>> to compound_head().
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/memory_hotplug.c | 45 +++++++++++++++++----------------------------
>>   1 file changed, 17 insertions(+), 28 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 02a0d4fbc3fe..cc9c16db2f8c 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1773,14 +1773,14 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
>>   static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>   {
>>   	unsigned long pfn;
>> -	struct page *page;
>>   	LIST_HEAD(source);
>> +	struct folio *folio;
>>   	static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL,
>>   				      DEFAULT_RATELIMIT_BURST);
>>   
>>   	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
>> -		struct folio *folio;
>> -		bool isolated;
>> +		struct page *page;
>> +		bool huge;
>>   
>>   		if (!pfn_valid(pfn))
>>   			continue;
>> @@ -1812,34 +1812,22 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>   			continue;
>>   		}
>>   
>> -		if (folio_test_hugetlb(folio)) {
>> -			isolate_hugetlb(folio, &source);
>> -			continue;
>> +		huge = folio_test_hugetlb(folio);
>> +		if (!huge) {
>> +			folio = folio_get_nontail_page(page);
>> +			if (!folio)
>> +				continue;
>>   		}
>>   
>> -		if (!get_page_unless_zero(page))
> 
> Does page always equal to head? Page is used in most cases in this function and head is only used to calculate pfn.
> If not, folio can not simply replace page?

Not very clear what your mean, no guarantee for page == head, that
is why we need use get_page_unless_zero or folio_get_nontail_page to get 
the folio, then try to isolate folio, since we only migrate the wholo
folio, I don't know why we can't convert page to folio here.

> 
>> -			continue;
>> -		/*
>> -		 * We can skip free pages. And we can deal with pages on
>> -		 * LRU and non-lru movable pages.
>> -		 */
>> -		if (PageLRU(page))
> 
> I think this check is incorrect. We should check __PageMovable(page) instead. So this patch
> fixes this issue too.
> 
> Thanks.
> .

Re: [PATCH v2 5/5] mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

[David Hildenbrand]

On 17.08.24 10:49, Kefeng Wang wrote:
> Use the isolate_folio_to_list() to unify hugetlb/LRU/non-LRU
> folio isolation, which cleanup code a bit and save a few calls
> to compound_head().
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   mm/memory_hotplug.c | 45 +++++++++++++++++----------------------------
>   1 file changed, 17 insertions(+), 28 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 02a0d4fbc3fe..cc9c16db2f8c 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1773,14 +1773,14 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
>   static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   {
>   	unsigned long pfn;
> -	struct page *page;
>   	LIST_HEAD(source);
> +	struct folio *folio;
>   	static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL,
>   				      DEFAULT_RATELIMIT_BURST);
>   
>   	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> -		struct folio *folio;
> -		bool isolated;
> +		struct page *page;
> +		bool huge;

Please use "hugetlb" if you mean hugetlb :)

>   
>   		if (!pfn_valid(pfn))
>   			continue;
> @@ -1812,34 +1812,22 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   			continue;
>   		}
>   
> -		if (folio_test_hugetlb(folio)) {
> -			isolate_hugetlb(folio, &source);
> -			continue;
> +		huge = folio_test_hugetlb(folio);
> +		if (!huge) {
> +			folio = folio_get_nontail_page(page);
> +			if (!folio)
> +				continue;
>   		}

Hm, remind me why we are changing the hugetlb code to not take a 
reference here? It does look odd.
utback_movable_pages(&source);

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 5/5] mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

[Kefeng Wang]

On 2024/8/26 22:55, David Hildenbrand wrote:
> On 17.08.24 10:49, Kefeng Wang wrote:
>> Use the isolate_folio_to_list() to unify hugetlb/LRU/non-LRU
>> folio isolation, which cleanup code a bit and save a few calls
>> to compound_head().
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/memory_hotplug.c | 45 +++++++++++++++++----------------------------
>>   1 file changed, 17 insertions(+), 28 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 02a0d4fbc3fe..cc9c16db2f8c 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1773,14 +1773,14 @@ static int scan_movable_pages(unsigned long 
>> start, unsigned long end,
>>   static void do_migrate_range(unsigned long start_pfn, unsigned long 
>> end_pfn)
>>   {
>>       unsigned long pfn;
>> -    struct page *page;
>>       LIST_HEAD(source);
>> +    struct folio *folio;
>>       static DEFINE_RATELIMIT_STATE(migrate_rs, 
>> DEFAULT_RATELIMIT_INTERVAL,
>>                         DEFAULT_RATELIMIT_BURST);
>>       for (pfn = start_pfn; pfn < end_pfn; pfn++) {
>> -        struct folio *folio;
>> -        bool isolated;
>> +        struct page *page;
>> +        bool huge;
> 
> Please use "hugetlb" if you mean hugetlb :)

OK.
> 
>>           if (!pfn_valid(pfn))
>>               continue;
>> @@ -1812,34 +1812,22 @@ static void do_migrate_range(unsigned long 
>> start_pfn, unsigned long end_pfn)
>>               continue;
>>           }
>> -        if (folio_test_hugetlb(folio)) {
>> -            isolate_hugetlb(folio, &source);
>> -            continue;
>> +        huge = folio_test_hugetlb(folio);
>> +        if (!huge) {
>> +            folio = folio_get_nontail_page(page);
>> +            if (!folio)
>> +                continue;
>>           }
> 
> Hm, remind me why we are changing the hugetlb code to not take a 
> reference here? It does look odd.

Different from folio_isolate_lru(), isolate_hugetlb() will check folio
and try get a reference of folio after get hugetlb_lock, other hugetlb
operation protected by this big lock too, so no need to take a reference
here.

Re: [PATCH v2 5/5] mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

[David Hildenbrand]

On 27.08.24 03:26, Kefeng Wang wrote:
> 
> 
> On 2024/8/26 22:55, David Hildenbrand wrote:
>> On 17.08.24 10:49, Kefeng Wang wrote:
>>> Use the isolate_folio_to_list() to unify hugetlb/LRU/non-LRU
>>> folio isolation, which cleanup code a bit and save a few calls
>>> to compound_head().
>>>
>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> ---
>>>    mm/memory_hotplug.c | 45 +++++++++++++++++----------------------------
>>>    1 file changed, 17 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 02a0d4fbc3fe..cc9c16db2f8c 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1773,14 +1773,14 @@ static int scan_movable_pages(unsigned long
>>> start, unsigned long end,
>>>    static void do_migrate_range(unsigned long start_pfn, unsigned long
>>> end_pfn)
>>>    {
>>>        unsigned long pfn;
>>> -    struct page *page;
>>>        LIST_HEAD(source);
>>> +    struct folio *folio;
>>>        static DEFINE_RATELIMIT_STATE(migrate_rs,
>>> DEFAULT_RATELIMIT_INTERVAL,
>>>                          DEFAULT_RATELIMIT_BURST);
>>>        for (pfn = start_pfn; pfn < end_pfn; pfn++) {
>>> -        struct folio *folio;
>>> -        bool isolated;
>>> +        struct page *page;
>>> +        bool huge;
>>
>> Please use "hugetlb" if you mean hugetlb :)
> 
> OK.
>>
>>>            if (!pfn_valid(pfn))
>>>                continue;
>>> @@ -1812,34 +1812,22 @@ static void do_migrate_range(unsigned long
>>> start_pfn, unsigned long end_pfn)
>>>                continue;
>>>            }
>>> -        if (folio_test_hugetlb(folio)) {
>>> -            isolate_hugetlb(folio, &source);
>>> -            continue;
>>> +        huge = folio_test_hugetlb(folio);
>>> +        if (!huge) {
>>> +            folio = folio_get_nontail_page(page);
>>> +            if (!folio)
>>> +                continue;
>>>            }
>>
>> Hm, remind me why we are changing the hugetlb code to not take a
>> reference here? It does look odd.
> 
> Different from folio_isolate_lru(), isolate_hugetlb() will check folio
> and try get a reference of folio after get hugetlb_lock, other hugetlb
> operation protected by this big lock too, so no need to take a reference
> here.

But this hugetlb-special casing looks quite ... special TBH.

Is there no way to avoid it?

I'd prefer something like the following, with a comment

if (hugetlb)
	/*
	 * We also want to migrate hugetlb folios that span multiple
          * memory blocks. So use whatever head page we identified.
	 */
	folio = folio_try_get(folio);
else
	folio = folio_get_nontail_page(page);

if (!folio)
	continue;


And then just dropping the reference unconditionally.

Is there a problem with that? Optimizing for hugetlb references during 
migration is not worth the trouble.


But now I wonder, why we not simply unconditionally do a folio_try_get() ...

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 5/5] mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

[Kefeng Wang]

On 2024/8/27 23:10, David Hildenbrand wrote:
> On 27.08.24 03:26, Kefeng Wang wrote:
>>
>>
>> On 2024/8/26 22:55, David Hildenbrand wrote:
>>> On 17.08.24 10:49, Kefeng Wang wrote:
>>>> Use the isolate_folio_to_list() to unify hugetlb/LRU/non-LRU
>>>> folio isolation, which cleanup code a bit and save a few calls
>>>> to compound_head().
>>>>
>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>> ---

...
>>> Hm, remind me why we are changing the hugetlb code to not take a
>>> reference here? It does look odd.
>>
>> Different from folio_isolate_lru(), isolate_hugetlb() will check folio
>> and try get a reference of folio after get hugetlb_lock, other hugetlb
>> operation protected by this big lock too, so no need to take a reference
>> here.
> 
> But this hugetlb-special casing looks quite ... special TBH.
> 
> Is there no way to avoid it?
> 
> I'd prefer something like the following, with a comment
> 
> if (hugetlb)
>      /*
>       * We also want to migrate hugetlb folios that span multiple
>           * memory blocks. So use whatever head page we identified.
>       */
>      folio = folio_try_get(folio);
> else
>      folio = folio_get_nontail_page(page);
> 
> if (!folio)
>      continue;
> 
> 
> And then just dropping the reference unconditionally.
> 
> Is there a problem with that? Optimizing for hugetlb references during 
> migration is not worth the trouble.
> 
> 
> But now I wonder, why we not simply unconditionally do a folio_try_get() 

Yes,  I use folio_get_nontail_page() and folio_put() unconditionally in v1,
this will skip tail page, not consistent with previous behavior for
hugetlb, so I change to current way, but for migration, use
folio_try_get()/folio_put() is enough since we always migrate the whole
folio, it will be more simple. I will push an additional fix to v3,
which just sent a couple of hours ago.

[1] 
https://lore.kernel.org/linux-mm/20240827114728.3212578-1-wangkefeng.wang@huawei.com/T/#t

Re: [PATCH v2 5/5] mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

[David Hildenbrand]

On 27.08.24 17:35, Kefeng Wang wrote:
> 
> 
> On 2024/8/27 23:10, David Hildenbrand wrote:
>> On 27.08.24 03:26, Kefeng Wang wrote:
>>>
>>>
>>> On 2024/8/26 22:55, David Hildenbrand wrote:
>>>> On 17.08.24 10:49, Kefeng Wang wrote:
>>>>> Use the isolate_folio_to_list() to unify hugetlb/LRU/non-LRU
>>>>> folio isolation, which cleanup code a bit and save a few calls
>>>>> to compound_head().
>>>>>
>>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>>> ---
> 
> ...
>>>> Hm, remind me why we are changing the hugetlb code to not take a
>>>> reference here? It does look odd.
>>>
>>> Different from folio_isolate_lru(), isolate_hugetlb() will check folio
>>> and try get a reference of folio after get hugetlb_lock, other hugetlb
>>> operation protected by this big lock too, so no need to take a reference
>>> here.
>>
>> But this hugetlb-special casing looks quite ... special TBH.
>>
>> Is there no way to avoid it?
>>
>> I'd prefer something like the following, with a comment
>>
>> if (hugetlb)
>>       /*
>>        * We also want to migrate hugetlb folios that span multiple
>>            * memory blocks. So use whatever head page we identified.
>>        */
>>       folio = folio_try_get(folio);
>> else
>>       folio = folio_get_nontail_page(page);
>>
>> if (!folio)
>>       continue;
>>
>>
>> And then just dropping the reference unconditionally.
>>
>> Is there a problem with that? Optimizing for hugetlb references during
>> migration is not worth the trouble.
>>
>>
>> But now I wonder, why we not simply unconditionally do a folio_try_get()
> 
> Yes,  I use folio_get_nontail_page() and folio_put() unconditionally in v1,

Yes, that part I remember.

> this will skip tail page, not consistent with previous behavior for
> hugetlb, so I change to current way, but for migration, use
> folio_try_get()/folio_put() is enough since we always migrate the whole
> folio, it will be more simple.

I'm wondering if anything relies on the folio_get_nontail_page() part, 
but I cannot really think "what" that should be. Using folio_try_get() 
would be much simpler: we have a page, we want to migrate it away, to do 
that we have to migrate the folio (wherever that starts or ends).

Also, make sure to test with free hugetlb folios.

> I will push an additional fix to v3,
> which just sent a couple of hours ago.

Feel free to send a fixup for v2 instead.

-- 
Cheers,

David / dhildenb

[PATCH v2 0/5] mm: memory_hotplug: improve do_migrate_range()

[Kefeng Wang]

Unify hwpoisoned page handling and isolation of HugeTLB/LRU/non-LRU
movable page, also convert to use folios in do_migrate_range().

v2:
- address comments from David(eg, fix HWPoison check/use a folio
  for pfn calculation firstly)
- fix lkp build errors for isolate_folio_to_list()
- drop unnecessary comments and don't grab one more ref for hugetlb 

Kefeng Wang (5):
  mm: memory_hotplug: remove head variable in do_migrate_range()
  mm: memory-failure: add unmap_posioned_folio()
  mm: memory_hotplug: check hwpoisoned page firstly in
    do_migrate_range()
  mm: migrate: add isolate_folio_to_list()
  mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

 include/linux/migrate.h |  3 ++
 mm/internal.h           |  9 +++++
 mm/memory-failure.c     | 89 +++++++++++++++++------------------------
 mm/memory_hotplug.c     | 62 ++++++++++++++--------------
 mm/migrate.c            | 27 +++++++++++++
 5 files changed, 105 insertions(+), 85 deletions(-)

-- 
2.27.0

[PATCH v2 4/5] mm: migrate: add isolate_folio_to_list()

[Kefeng Wang]

Add isolate_folio_to_list() helper to try to isolate HugeTLB,
no-LRU movable and LRU folios to a list, which will be reused by
do_migrate_range() from memory hotplug soon, also drop the
mf_isolate_folio() since we could directly use new helper in
the soft_offline_in_use_page().

Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/linux/migrate.h |  3 +++
 mm/memory-failure.c     | 46 ++++++++++-------------------------------
 mm/migrate.c            | 27 ++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 35 deletions(-)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 644be30b69c8..002e49b2ebd9 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -70,6 +70,7 @@ int migrate_pages(struct list_head *l, new_folio_t new, free_folio_t free,
 		  unsigned int *ret_succeeded);
 struct folio *alloc_migration_target(struct folio *src, unsigned long private);
 bool isolate_movable_page(struct page *page, isolate_mode_t mode);
+bool isolate_folio_to_list(struct folio *folio, struct list_head *list);
 
 int migrate_huge_page_move_mapping(struct address_space *mapping,
 		struct folio *dst, struct folio *src);
@@ -91,6 +92,8 @@ static inline struct folio *alloc_migration_target(struct folio *src,
 	{ return NULL; }
 static inline bool isolate_movable_page(struct page *page, isolate_mode_t mode)
 	{ return false; }
+static inline bool isolate_folio_to_list(struct folio *folio, struct list_head *list)
+	{ return false; }
 
 static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
 				  struct folio *dst, struct folio *src)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 93848330de1f..d8298017bd99 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2659,40 +2659,6 @@ EXPORT_SYMBOL(unpoison_memory);
 #undef pr_fmt
 #define pr_fmt(fmt) "Soft offline: " fmt
 
-static bool mf_isolate_folio(struct folio *folio, struct list_head *pagelist)
-{
-	bool isolated = false;
-
-	if (folio_test_hugetlb(folio)) {
-		isolated = isolate_hugetlb(folio, pagelist);
-	} else {
-		bool lru = !__folio_test_movable(folio);
-
-		if (lru)
-			isolated = folio_isolate_lru(folio);
-		else
-			isolated = isolate_movable_page(&folio->page,
-							ISOLATE_UNEVICTABLE);
-
-		if (isolated) {
-			list_add(&folio->lru, pagelist);
-			if (lru)
-				node_stat_add_folio(folio, NR_ISOLATED_ANON +
-						    folio_is_file_lru(folio));
-		}
-	}
-
-	/*
-	 * If we succeed to isolate the folio, we grabbed another refcount on
-	 * the folio, so we can safely drop the one we got from get_any_page().
-	 * If we failed to isolate the folio, it means that we cannot go further
-	 * and we will return an error, so drop the reference we got from
-	 * get_any_page() as well.
-	 */
-	folio_put(folio);
-	return isolated;
-}
-
 /*
  * soft_offline_in_use_page handles hugetlb-pages and non-hugetlb pages.
  * If the page is a non-dirty unmapped page-cache page, it simply invalidates.
@@ -2744,7 +2710,7 @@ static int soft_offline_in_use_page(struct page *page)
 		return 0;
 	}
 
-	if (mf_isolate_folio(folio, &pagelist)) {
+	if (isolate_folio_to_list(folio, &pagelist)) {
 		ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
 			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, NULL);
 		if (!ret) {
@@ -2766,6 +2732,16 @@ static int soft_offline_in_use_page(struct page *page)
 			pfn, msg_page[huge], page_count(page), &page->flags);
 		ret = -EBUSY;
 	}
+
+	/*
+	 * If we succeed to isolate the folio, we grabbed another refcount on
+	 * the folio, so we can safely drop the one we got from get_any_page().
+	 * If we failed to isolate the folio, it means that we cannot go further
+	 * and we will return an error, so drop the reference we got from
+	 * get_any_page() as well.
+	 */
+	folio_put(folio);
+
 	return ret;
 }
 
diff --git a/mm/migrate.c b/mm/migrate.c
index 6e32098ac2dc..7b7b5b16e610 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -178,6 +178,33 @@ void putback_movable_pages(struct list_head *l)
 	}
 }
 
+/* Must be called with an elevated refcount on the non-hugetlb folio */
+bool isolate_folio_to_list(struct folio *folio, struct list_head *list)
+{
+	bool isolated = false;
+
+	if (folio_test_hugetlb(folio)) {
+		isolated = isolate_hugetlb(folio, list);
+	} else {
+		bool lru = !__folio_test_movable(folio);
+
+		if (lru)
+			isolated = folio_isolate_lru(folio);
+		else
+			isolated = isolate_movable_page(&folio->page,
+							ISOLATE_UNEVICTABLE);
+
+		if (isolated) {
+			list_add(&folio->lru, list);
+			if (lru)
+				node_stat_add_folio(folio, NR_ISOLATED_ANON +
+						    folio_is_file_lru(folio));
+		}
+	}
+
+	return isolated;
+}
+
 static bool try_to_map_unused_to_zeropage(struct page_vma_mapped_walk *pvmw,
 					  struct folio *folio,
 					  unsigned long idx)
-- 
2.27.0