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

[PATCH 0/4] 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().

Kefeng Wang (4):
  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

 mm/internal.h       | 11 ++++++++
 mm/memory-failure.c | 64 +++++++++++++++++++--------------------------
 mm/memory_hotplug.c | 61 ++++++++++++++++++------------------------
 mm/migrate.c        | 27 +++++++++++++++++++
 4 files changed, 90 insertions(+), 73 deletions(-)

-- 
2.27.0

[PATCH 1/4] 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.

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 7a3bcc6d95e7..c5bd24c4fa3a 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1069,6 +1069,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);
 
@@ -1089,6 +1091,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 8c765329829f..99d92565cbb0 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 1/4] mm: memory-failure: add unmap_posioned_folio()

[David Hildenbrand]

On 25.07.24 03:16, Kefeng Wang wrote:
> Add unmap_posioned_folio() helper which will be reused by
> do_migrate_range() from memory hotplug soon.
> 
> 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 7a3bcc6d95e7..c5bd24c4fa3a 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1069,6 +1069,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);
>   
> @@ -1089,6 +1091,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
> +

Was wondering if we could come up with a better name (something that 
starts with folio_*), but wasn't able to come up with something I liked 
more.

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

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/4] mm: memory-failure: add unmap_posioned_folio()

[Kefeng Wang]

On 2024/7/30 18:20, David Hildenbrand wrote:
> On 25.07.24 03:16, Kefeng Wang wrote:
>> Add unmap_posioned_folio() helper which will be reused by
>> do_migrate_range() from memory hotplug soon.
>>
>> 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 7a3bcc6d95e7..c5bd24c4fa3a 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -1069,6 +1069,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);
>> @@ -1089,6 +1091,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
>> +
> 
> Was wondering if we could come up with a better name (something that 
> starts with folio_*), but wasn't able to come up with something I liked 
> more.

Uh, pool at naming, I can't find a better one...
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 
Thanks.

[PATCH 2/4] 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 dead loop
still occur if offline a hwpoison hugetlb, luckly, after the commit
e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove memory
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 deed 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() and
but fails to migrated. 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 | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 66267c26ca1b..ccaf4c480aed 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1788,28 +1788,33 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_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;
-
 		/*
 		 * HWPoison pages have elevated reference counts so the migration would
 		 * fail on them. It also doesn't make any sense to migrate them in the
 		 * first place. Still try to unmap such a page in case it is still mapped
-		 * (e.g. current hwpoison implementation doesn't unmap KSM pages but keep
-		 * the unmap as the catch all safety net).
+		 * (keep the unmap as the catch all safety net).
 		 */
-		if (PageHWPoison(page)) {
+		if (unlikely(PageHWPoison(page))) {
+			folio = page_folio(page);
+
 			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);
+
+			if (folio_test_large(folio))
+				pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
 			continue;
 		}
 
+		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;
+
 		if (!get_page_unless_zero(page))
 			continue;
 		/*
-- 
2.27.0

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[David Hildenbrand]

On 25.07.24 03:16, Kefeng Wang wrote:
> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
> pages to be offlined") don't handle the hugetlb pages, the dead loop
> still occur if offline a hwpoison hugetlb, luckly, after the commit
> e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove memory
> 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 deed loop issue is fixed.

did you mean "endless loop" ?

> 
> 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() and
> but fails to migrated. 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.

But what's the benefit here besides slightly faster handling in an 
absolute corner case (I strongly suspect that we don't care)?

> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   mm/memory_hotplug.c | 27 ++++++++++++++++-----------
>   1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 66267c26ca1b..ccaf4c480aed 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1788,28 +1788,33 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_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;
> -
>   		/*
>   		 * HWPoison pages have elevated reference counts so the migration would
>   		 * fail on them. It also doesn't make any sense to migrate them in the
>   		 * first place. Still try to unmap such a page in case it is still mapped
> -		 * (e.g. current hwpoison implementation doesn't unmap KSM pages but keep
> -		 * the unmap as the catch all safety net).
> +		 * (keep the unmap as the catch all safety net).
>   		 */
> -		if (PageHWPoison(page)) {
> +		if (unlikely(PageHWPoison(page))) {

We're not checking the head page here, will this work reliably for 
hugetlb? (I recall some difference in per-page hwpoison handling between 
hugetlb and THP due to the vmemmap optimization)


-- 
Cheers,

David / dhildenb

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[Kefeng Wang]

On 2024/7/30 18:26, David Hildenbrand wrote:
> On 25.07.24 03:16, Kefeng Wang wrote:
>> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
>> pages to be offlined") don't handle the hugetlb pages, the dead loop
>> still occur if offline a hwpoison hugetlb, luckly, after the commit
>> e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove memory
>> 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 deed loop issue is fixed.
> 
> did you mean "endless loop" ?

Exactly, will fix the words.

> 
>>
>> 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() and
>> but fails to migrated. 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.
> 
> But what's the benefit here besides slightly faster handling in an 
> absolute corner case (I strongly suspect that we don't care)?

Yes, it is a very corner case, the goal is to move isolate_hugetlb()
after HWpoison check, then to unify isolation and folio conversion
(patch4). But we must correctly handle the hugetlb unmap when meet
a hwpoisoned page.

> 
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/memory_hotplug.c | 27 ++++++++++++++++-----------
>>   1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 66267c26ca1b..ccaf4c480aed 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1788,28 +1788,33 @@ static void do_migrate_range(unsigned long 
>> start_pfn, unsigned long end_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;
>> -
>>           /*
>>            * HWPoison pages have elevated reference counts so the 
>> migration would
>>            * fail on them. It also doesn't make any sense to migrate 
>> them in the
>>            * first place. Still try to unmap such a page in case it is 
>> still mapped
>> -         * (e.g. current hwpoison implementation doesn't unmap KSM 
>> pages but keep
>> -         * the unmap as the catch all safety net).
>> +         * (keep the unmap as the catch all safety net).
>>            */
>> -        if (PageHWPoison(page)) {
>> +        if (unlikely(PageHWPoison(page))) {
> 
> We're not checking the head page here, will this work reliably for 
> hugetlb? (I recall some difference in per-page hwpoison handling between 
> hugetlb and THP due to the vmemmap optimization)

Before this changes, the hwposioned hugetlb page won't try to unmap in
do_migrate_range(), we hope it already unmapped in memory_failure(), as 
mentioned from comments, there maybe fail to unmap, so a new safeguard 
to try to unmap it again here, but we don't need to guarantee it.

The unmap_posioned_folio() used to correctly handle hugetlb pages in 
shared mappings if we met a hwpoisoned page(maybe headpage/may subpage).

> 
> 

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[David Hildenbrand]

>>
>> We're not checking the head page here, will this work reliably for
>> hugetlb? (I recall some difference in per-page hwpoison handling between
>> hugetlb and THP due to the vmemmap optimization)
> 
> Before this changes, the hwposioned hugetlb page won't try to unmap in
> do_migrate_range(), we hope it already unmapped in memory_failure(), as
> mentioned from comments, there maybe fail to unmap, so a new safeguard
> to try to unmap it again here, but we don't need to guarantee it.

Thanks for clarifying!

But I do wonder if the PageHWPoison() is the right thing to do for hugetlb.

IIUC, hugetlb requires folio_test_hwpoison() -- testing the head page 
not the subpage. Reason is that due to the vmemmap optimization we might 
not be able to modify subpages to set hwpoison.

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[Kefeng Wang]

On 2024/8/2 4:10, David Hildenbrand wrote:
> 
>>>
>>> We're not checking the head page here, will this work reliably for
>>> hugetlb? (I recall some difference in per-page hwpoison handling between
>>> hugetlb and THP due to the vmemmap optimization)
>>
>> Before this changes, the hwposioned hugetlb page won't try to unmap in
>> do_migrate_range(), we hope it already unmapped in memory_failure(), as
>> mentioned from comments, there maybe fail to unmap, so a new safeguard
>> to try to unmap it again here, but we don't need to guarantee it.
> 
> Thanks for clarifying!
> 
> But I do wonder if the PageHWPoison() is the right thing to do for hugetlb.
> 
> IIUC, hugetlb requires folio_test_hwpoison() -- testing the head page 
> not the subpage. Reason is that due to the vmemmap optimization we might 
> not be able to modify subpages to set hwpoison.

Yes, HVO is special(only head page with hwpoison), since we always want
to check head page here (next pfn = head_pfn + nr), so it might be
enough to only use PageHWpoison, but just in case, adding hwpoison check
for the head page,

	if (unlikely(PageHWPoison(page) || folio_test_hwpoison(folio)))

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[David Hildenbrand]

On 02.08.24 09:50, Kefeng Wang wrote:
> 
> 
> On 2024/8/2 4:10, David Hildenbrand wrote:
>>
>>>>
>>>> We're not checking the head page here, will this work reliably for
>>>> hugetlb? (I recall some difference in per-page hwpoison handling between
>>>> hugetlb and THP due to the vmemmap optimization)
>>>
>>> Before this changes, the hwposioned hugetlb page won't try to unmap in
>>> do_migrate_range(), we hope it already unmapped in memory_failure(), as
>>> mentioned from comments, there maybe fail to unmap, so a new safeguard
>>> to try to unmap it again here, but we don't need to guarantee it.
>>
>> Thanks for clarifying!
>>
>> But I do wonder if the PageHWPoison() is the right thing to do for hugetlb.
>>
>> IIUC, hugetlb requires folio_test_hwpoison() -- testing the head page
>> not the subpage. Reason is that due to the vmemmap optimization we might
>> not be able to modify subpages to set hwpoison.
> 
> Yes, HVO is special(only head page with hwpoison), since we always want
> to check head page here (next pfn = head_pfn + nr), so it might be
> enough to only use PageHWpoison, but just in case, adding hwpoison check
> for the head page,
> 
> 	if (unlikely(PageHWPoison(page) || folio_test_hwpoison(folio)))

I also do wonder if we have to check for large folios 
folio_test_has_hwpoison():
if any subpage is poisoned, not just the current page.

I don't think folio_set_has_hwpoisoned() is used for hugetlb.

What a mess.


-- 
Cheers,

David / dhildenb

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[Miaohe Lin]

On 2024/8/7 19:14, David Hildenbrand wrote:
> On 07.08.24 09:39, Miaohe Lin wrote:
>> On 2024/8/6 17:29, David Hildenbrand wrote:
>>> On 02.08.24 09:50, Kefeng Wang wrote:
>>>>
>>>>
>>>> On 2024/8/2 4:10, David Hildenbrand wrote:
>>>>>
>>>>>>>
>>>>>>> We're not checking the head page here, will this work reliably for
>>>>>>> hugetlb? (I recall some difference in per-page hwpoison handling between
>>>>>>> hugetlb and THP due to the vmemmap optimization)
>>>>>>
>>>>>> Before this changes, the hwposioned hugetlb page won't try to unmap in
>>>>>> do_migrate_range(), we hope it already unmapped in memory_failure(), as
>>>>>> mentioned from comments, there maybe fail to unmap, so a new safeguard
>>>>>> to try to unmap it again here, but we don't need to guarantee it.
>>>>>
>>>>> Thanks for clarifying!
>>>>>
>>>>> But I do wonder if the PageHWPoison() is the right thing to do for hugetlb.
>>>>>
>>>>> IIUC, hugetlb requires folio_test_hwpoison() -- testing the head page
>>>>> not the subpage. Reason is that due to the vmemmap optimization we might
>>>>> not be able to modify subpages to set hwpoison.
>>>>
>>>> Yes, HVO is special(only head page with hwpoison), since we always want
>>>> to check head page here (next pfn = head_pfn + nr), so it might be
>>>> enough to only use PageHWpoison, but just in case, adding hwpoison check
>>>> for the head page,
>>>>
>>>>      if (unlikely(PageHWPoison(page) || folio_test_hwpoison(folio)))
>>>
>>> I also do wonder if we have to check for large folios folio_test_has_hwpoison():
>>> if any subpage is poisoned, not just the current page.
>>>
>>
>> IMHO, below if condition [1] should be fine to check for any hwpoisoned folio:
>>
>>   if (folio_test_hwpoison(folio) ||
>>        (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>>
>> 1. For raw pages, folio_test_hwpoison(folio) works fine.
>> 2. For thp (memory_failure fails to split it in first place), folio_test_has_hwpoisoned(folio) works fine.
>> 3. For hugetlb, we always have hwpoison flag set for folio. So folio_test_hwpoison(folio) works fine.

It seems I missed one corner case. When memory_failure meets an isolated thp, get_hwpoison_page() will return EIO and
thp won't have has_hwpoison flag set. Above pattern might not work with it. :(

>>
>> But folio might not be the right hwpoisoned page, i.e. subpages might be hwpoisoned instead.
>> Or am I miss something?
> 
> Yes, but we can only migrate full folios, and if any subpage is poisoned we're in trouble and have to effectively force-unmap it?

Yes, I agree with you.

> 
> At least that's my understanding :)

Thanks.
.

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[David Hildenbrand]

On 25.07.24 03:16, Kefeng Wang wrote:
> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
> pages to be offlined") don't handle the hugetlb pages, the dead loop
> still occur if offline a hwpoison hugetlb, luckly, after the commit
> e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove memory
> 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 deed 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() and
> but fails to migrated. 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 | 27 ++++++++++++++++-----------
>   1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 66267c26ca1b..ccaf4c480aed 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1788,28 +1788,33 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_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;
> -
>   		/*
>   		 * HWPoison pages have elevated reference counts so the migration would
>   		 * fail on them. It also doesn't make any sense to migrate them in the
>   		 * first place. Still try to unmap such a page in case it is still mapped
> -		 * (e.g. current hwpoison implementation doesn't unmap KSM pages but keep
> -		 * the unmap as the catch all safety net).
> +		 * (keep the unmap as the catch all safety net).
>   		 */
> -		if (PageHWPoison(page)) {
> +		if (unlikely(PageHWPoison(page))) {
> +			folio = page_folio(page);
> +
>   			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);
> +
> +			if (folio_test_large(folio))
> +				pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>   			continue;
>   		}
>   
> +		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;

If we can use a folio in the PageHWPoison() case, can we use one here as 
well? I know that it's all unreliable when not holding a folio 
reference, and we have to be a bit careful.

It feels like using folios here would mostly be fine, because things 
like PageHuge() already use folios internally.

And using it in the PageHWPoison() but not here looks a bit odd.

The important part is that we don't segfault if we'd overshoot our target.

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[Kefeng Wang]

On 2024/8/2 4:14, David Hildenbrand wrote:
> On 25.07.24 03:16, Kefeng Wang wrote:
>> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
>> pages to be offlined") don't handle the hugetlb pages, the dead loop
>> still occur if offline a hwpoison hugetlb, luckly, after the commit
>> e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove memory
>> 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 deed 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() and
>> but fails to migrated. 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 | 27 ++++++++++++++++-----------
>>   1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 66267c26ca1b..ccaf4c480aed 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1788,28 +1788,33 @@ static void do_migrate_range(unsigned long 
>> start_pfn, unsigned long end_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;
>> -
>>           /*
>>            * HWPoison pages have elevated reference counts so the 
>> migration would
>>            * fail on them. It also doesn't make any sense to migrate 
>> them in the
>>            * first place. Still try to unmap such a page in case it is 
>> still mapped
>> -         * (e.g. current hwpoison implementation doesn't unmap KSM 
>> pages but keep
>> -         * the unmap as the catch all safety net).
>> +         * (keep the unmap as the catch all safety net).
>>            */
>> -        if (PageHWPoison(page)) {
>> +        if (unlikely(PageHWPoison(page))) {
>> +            folio = page_folio(page);
>> +
>>               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);
>> +
>> +            if (folio_test_large(folio))
>> +                pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>               continue;
>>           }
>> +        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;
> 
> If we can use a folio in the PageHWPoison() case, can we use one here as 
> well? I know that it's all unreliable when not holding a folio 
> reference, and we have to be a bit careful.

Using a folio here is part of patch4, I want to unify hugetlb/thp(or 
large folio) with "pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1" 
when large folio after get a ref.

> 
> It feels like using folios here would mostly be fine, because things 
> like PageHuge() already use folios internally.
> 
> And using it in the PageHWPoison() but not here looks a bit odd.

We will convert to use folio in the following patch.

> 
> The important part is that we don't segfault if we'd overshoot our target.
> 

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[Kefeng Wang]

Hi David, I have some question,

On 2024/8/2 16:02, Kefeng Wang wrote:
> 
...
>>>            */
>>> -        if (PageHWPoison(page)) {
>>> +        if (unlikely(PageHWPoison(page))) {
>>> +            folio = page_folio(page);
>>> +
>>>               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);
>>> +
>>> +            if (folio_test_large(folio))
>>> +                pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>>               continue;
>>>           }
>>> +        if (PageHuge(page)) {
>>> +            pfn = page_to_pfn(head) + compound_nr(head) - 1;
>>> +            isolate_hugetlb(folio, &source);
>>> +            continue;
>>> +        } else if (PageTransHuge(page))

If the page is a tail page, we will BUG_ON(DEBUG_VM enabled) here, but
it seems that we don't guarantee the page won't be a tail page.

>>> +            pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;

thp_nr_pages() need a head page, I think it should use head here, so we
can directly use folio_nr_pages().

>>
>> If we can use a folio in the PageHWPoison() case, can we use one here 
>> as well? I know that it's all unreliable when not holding a folio 
>> reference, and we have to be a bit careful.
> 
> Using a folio here is part of patch4, I want to unify hugetlb/thp(or 
> large folio) with "pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1" 
> when large folio after get a ref.

Think it again, even the folio don't hold a ref(splitting concurrently
or something else), folio_nr_pages return incorrect, it won't cause 
issue since we will loop and find movable pages again in 
scan_movable_pages() and try to isolate pages, so directly use

if (folio_test_large(folio)) {
	pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
	if (folio_test_hugetlb(folio))
		isolate_hugetlb(folio, &source);
}

Correct me if I am wrong, thanks.

> 
>>
>> It feels like using folios here would mostly be fine, because things 
>> like PageHuge() already use folios internally.
>>
>> And using it in the PageHWPoison() but not here looks a bit odd.
> 
> We will convert to use folio in the following patch.
> 
>>
>> The important part is that we don't segfault if we'd overshoot our 
>> target.
>>
> 

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[David Hildenbrand]

On 06.08.24 05:44, Kefeng Wang wrote:
> Hi David, I have some question,
> 
> On 2024/8/2 16:02, Kefeng Wang wrote:
>>
> ...
>>>>             */
>>>> -        if (PageHWPoison(page)) {
>>>> +        if (unlikely(PageHWPoison(page))) {
>>>> +            folio = page_folio(page);
>>>> +
>>>>                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);
>>>> +
>>>> +            if (folio_test_large(folio))
>>>> +                pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>>>                continue;
>>>>            }
>>>> +        if (PageHuge(page)) {
>>>> +            pfn = page_to_pfn(head) + compound_nr(head) - 1;
>>>> +            isolate_hugetlb(folio, &source);
>>>> +            continue;
>>>> +        } else if (PageTransHuge(page))
> 
> If the page is a tail page, we will BUG_ON(DEBUG_VM enabled) here, but
> it seems that we don't guarantee the page won't be a tail page.

Maybe at some point we might want to remove these sanity checks or have 
explicit, expected-to-be-racy folio functions.

Like folio_test_hugetlb_racy(), folio_test_large_racy(), 
folio_nr_pages_racy().

Because the VM_DEBUG checks for folio_test_large() etc. actually make 
sense in other context where we know that concurrent splitting is 
impossible.

But maybe part of the puzzle will be in the future that we want to do a 
RCU read lock here and perform freeing/splitting under RCU, when we'll 
also have to alloc/free the "struct folio".

> 
>>>> +            pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
> 
> thp_nr_pages() need a head page, I think it should use head here, so we
> can directly use folio_nr_pages().
> 
>>>
>>> If we can use a folio in the PageHWPoison() case, can we use one here
>>> as well? I know that it's all unreliable when not holding a folio
>>> reference, and we have to be a bit careful.
>>
>> Using a folio here is part of patch4, I want to unify hugetlb/thp(or
>> large folio) with "pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1"
>> when large folio after get a ref.
> 
> Think it again, even the folio don't hold a ref(splitting concurrently
> or something else), folio_nr_pages return incorrect, it won't cause
> issue since we will loop and find movable pages again in
> scan_movable_pages() and try to isolate pages, so directly use
> 
> if (folio_test_large(folio)) {
> 	pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
> 	if (folio_test_hugetlb(folio))
> 		isolate_hugetlb(folio, &source);
> }

Likely we should add a comment here that a large folio might get split 
concurrently and that folio_nr_pages() might read garbage. But out loop 
should handle that and we would revisit the split folio later.


-- 
Cheers,

David / dhildenb

Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

[David Hildenbrand]

On 02.08.24 10:02, Kefeng Wang wrote:
> 
> 
> On 2024/8/2 4:14, David Hildenbrand wrote:
>> On 25.07.24 03:16, Kefeng Wang wrote:
>>> The commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned
>>> pages to be offlined") don't handle the hugetlb pages, the dead loop
>>> still occur if offline a hwpoison hugetlb, luckly, after the commit
>>> e591ef7d96d6 ("mm,hwpoison,hugetlb,memory_hotplug: hotremove memory
>>> 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 deed 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() and
>>> but fails to migrated. 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 | 27 ++++++++++++++++-----------
>>>    1 file changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 66267c26ca1b..ccaf4c480aed 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1788,28 +1788,33 @@ static void do_migrate_range(unsigned long
>>> start_pfn, unsigned long end_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;
>>> -
>>>            /*
>>>             * HWPoison pages have elevated reference counts so the
>>> migration would
>>>             * fail on them. It also doesn't make any sense to migrate
>>> them in the
>>>             * first place. Still try to unmap such a page in case it is
>>> still mapped
>>> -         * (e.g. current hwpoison implementation doesn't unmap KSM
>>> pages but keep
>>> -         * the unmap as the catch all safety net).
>>> +         * (keep the unmap as the catch all safety net).
>>>             */
>>> -        if (PageHWPoison(page)) {
>>> +        if (unlikely(PageHWPoison(page))) {
>>> +            folio = page_folio(page);
>>> +
>>>                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);
>>> +
>>> +            if (folio_test_large(folio))
>>> +                pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>>                continue;
>>>            }
>>> +        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;
>>
>> If we can use a folio in the PageHWPoison() case, can we use one here as
>> well? I know that it's all unreliable when not holding a folio
>> reference, and we have to be a bit careful.
> 
> Using a folio here is part of patch4, I want to unify hugetlb/thp(or
> large folio) with "pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1"
> when large folio after get a ref.

The thing is that it looks weird in the context of this patch to use a 
folio on path a but not on path b.

-- 
Cheers,

David / dhildenb

[PATCH 3/4] 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.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/internal.h       |  2 ++
 mm/memory-failure.c | 21 +--------------------
 mm/migrate.c        | 27 +++++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index c5bd24c4fa3a..2484191c0764 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -403,6 +403,8 @@ extern unsigned long highest_memmap_pfn;
  */
 #define MAX_RECLAIM_RETRIES 16
 
+bool isolate_folio_to_list(struct folio *folio, struct list_head *list);
+
 /*
  * in mm/vmscan.c:
  */
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 99d92565cbb0..7e0f143bd51a 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2659,26 +2659,7 @@ EXPORT_SYMBOL(unpoison_memory);
 
 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));
-		}
-	}
+	bool isolated = isolate_folio_to_list(folio, pagelist);
 
 	/*
 	 * If we succeed to isolate the folio, we grabbed another refcount on
diff --git a/mm/migrate.c b/mm/migrate.c
index e7296c0fb5d5..51cab1103502 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -177,6 +177,33 @@ void putback_movable_pages(struct list_head *l)
 	}
 }
 
+/* Must be called with an elevated refcount on the 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;
+}
+
 /*
  * Restore a potential migration pte to a working pte entry
  */
-- 
2.27.0

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

[kernel test robot]

Hi Kefeng,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Kefeng-Wang/mm-memory-failure-add-unmap_posioned_folio/20240725-091923
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240725011647.1306045-4-wangkefeng.wang%40huawei.com
patch subject: [PATCH 3/4] mm: migrate: add isolate_folio_to_list()
config: x86_64-randconfig-004-20240726 (https://download.01.org/0day-ci/archive/20240726/202407262129.FJbkroy1-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240726/202407262129.FJbkroy1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407262129.FJbkroy1-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: mm/memory-failure.o: in function `mf_isolate_folio':
>> mm/memory-failure.c:2662:(.text+0x2e8a): undefined reference to `isolate_folio_to_list'


vim +2662 mm/memory-failure.c

  2659	
  2660	static bool mf_isolate_folio(struct folio *folio, struct list_head *pagelist)
  2661	{
> 2662		bool isolated = isolate_folio_to_list(folio, pagelist);
  2663	
  2664		/*
  2665		 * If we succeed to isolate the folio, we grabbed another refcount on
  2666		 * the folio, so we can safely drop the one we got from get_any_page().
  2667		 * If we failed to isolate the folio, it means that we cannot go further
  2668		 * and we will return an error, so drop the reference we got from
  2669		 * get_any_page() as well.
  2670		 */
  2671		folio_put(folio);
  2672		return isolated;
  2673	}
  2674	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Kefeng Wang]

On 2024/7/26 22:21, kernel test robot wrote:
> Hi Kefeng,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on akpm-mm/mm-everything]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Kefeng-Wang/mm-memory-failure-add-unmap_posioned_folio/20240725-091923
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> patch link:    https://lore.kernel.org/r/20240725011647.1306045-4-wangkefeng.wang%40huawei.com
> patch subject: [PATCH 3/4] mm: migrate: add isolate_folio_to_list()
> config: x86_64-randconfig-004-20240726 (https://download.01.org/0day-ci/archive/20240726/202407262129.FJbkroy1-lkp@intel.com/config)
> compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240726/202407262129.FJbkroy1-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202407262129.FJbkroy1-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>     ld: mm/memory-failure.o: in function `mf_isolate_folio':
>>> mm/memory-failure.c:2662:(.text+0x2e8a): undefined reference to `isolate_folio_to_list'
> 
> 

OK, no CONFIG_MIGRATION, will fix in next version,

> vim +2662 mm/memory-failure.c
> 
>    2659	
>    2660	static bool mf_isolate_folio(struct folio *folio, struct list_head *pagelist)

and will kill mf_ioslate_folio() too, we could directly use 
isolate_folio_to_list() in soft_offline_in_use_page().


Thanks.

>    2661	{
>> 2662		bool isolated = isolate_folio_to_list(folio, pagelist);
>    2663	
>    2664		/*
>    2665		 * If we succeed to isolate the folio, we grabbed another refcount on
>    2666		 * the folio, so we can safely drop the one we got from get_any_page().
>    2667		 * If we failed to isolate the folio, it means that we cannot go further
>    2668		 * and we will return an error, so drop the reference we got from
>    2669		 * get_any_page() as well.
>    2670		 */
>    2671		folio_put(folio);
>    2672		return isolated;
>    2673	}
>    2674	
> 

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

[David Hildenbrand]

On 25.07.24 03:16, 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.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---

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

-- 
Cheers,

David / dhildenb

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

[Kefeng Wang]

Move isolate_hugetlb() after grab a reference, and 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 | 48 +++++++++++++++------------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index ccaf4c480aed..057037766efa 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1773,20 +1773,17 @@ 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;
 	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;
 
 		if (!pfn_valid(pfn))
 			continue;
 		page = pfn_to_page(pfn);
-		folio = page_folio(page);
-		head = &folio->page;
 
 		/*
 		 * HWPoison pages have elevated reference counts so the migration would
@@ -1808,36 +1805,22 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 			continue;
 		}
 
-		if (PageHuge(page)) {
-			pfn = page_to_pfn(head) + compound_nr(head) - 1;
-			isolate_hugetlb(folio, &source);
+		folio = folio_get_nontail_page(page);
+		if (!folio)
 			continue;
-		} else if (PageTransHuge(page))
-			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
-
-		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 {
+		/* Skip free folios, deal with hugetlb, LRU and non-lru movable folios. */
+		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 (folio_test_large(folio))
+			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
+
+		folio_put(folio);
 	}
 	if (!list_empty(&source)) {
 		nodemask_t nmask = node_states[N_MEMORY];
@@ -1852,7 +1835,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
@@ -1865,11 +1848,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 4/4] mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

[David Hildenbrand]

On 25.07.24 03:16, Kefeng Wang wrote:
> Move isolate_hugetlb() after grab a reference, and 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().

Will this work with free hugetlb folios that have a refcount of 0 and
get_page_unless_zero() would fail?


-- 
Cheers,

David / dhildenb

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

[Kefeng Wang]

On 2024/7/30 18:31, David Hildenbrand wrote:
> On 25.07.24 03:16, Kefeng Wang wrote:
>> Move isolate_hugetlb() after grab a reference, and 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().
> 
> Will this work with free hugetlb folios that have a refcount of 0 and
> get_page_unless_zero() would fail?

Before this changes, isolate_hugetlb() will fail to isolate a ref=0 
folio(call folio_try_get()) and now get_page_unless_zero() will fail 
too, so no behavior change, maybe I miss something?

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

[David Hildenbrand]

On 31.07.24 07:13, Kefeng Wang wrote:
> 
> 
> On 2024/7/30 18:31, David Hildenbrand wrote:
>> On 25.07.24 03:16, Kefeng Wang wrote:
>>> Move isolate_hugetlb() after grab a reference, and 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().
>>
>> Will this work with free hugetlb folios that have a refcount of 0 and
>> get_page_unless_zero() would fail?
> 
> Before this changes, isolate_hugetlb() will fail to isolate a ref=0
> folio(call folio_try_get()) and now get_page_unless_zero() will fail
> too, so no behavior change, maybe I miss something?

Good point, isolate_hugetlb() will simply do nothing and we skip the 
folio for this round. Let me take a closer look.

-- 
Cheers,

David / dhildenb

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

[David Hildenbrand]

On 25.07.24 03:16, Kefeng Wang wrote:
> Move isolate_hugetlb() after grab a reference, and 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 | 48 +++++++++++++++------------------------------
>   1 file changed, 16 insertions(+), 32 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index ccaf4c480aed..057037766efa 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1773,20 +1773,17 @@ 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;
>   	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;
>   
>   		if (!pfn_valid(pfn))
>   			continue;
>   		page = pfn_to_page(pfn);
> -		folio = page_folio(page);
> -		head = &folio->page;
>   
>   		/*
>   		 * HWPoison pages have elevated reference counts so the migration would
> @@ -1808,36 +1805,22 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   			continue;
>   		}
>   
> -		if (PageHuge(page)) {
> -			pfn = page_to_pfn(head) + compound_nr(head) - 1;
> -			isolate_hugetlb(folio, &source);
> +		folio = folio_get_nontail_page(page);
> +		if (!folio)
>   			continue;

There is one interesting case: 1 GiB hugetlb folios can span multiple 
memory blocks (e.g., 128 MiB). Offlining individual blocks must work.

If you do the folio_get_nontail_page() we'd not be able to offline a 
memory block in the middle anymore, because we'd never try even 
isolating it.

So likely we have to try getting the head page of a large folio instead 
(as a fallback if this fails?) and continue from there.

In case of an free hugetlb tail page we would now iterate each 
individual page instead of simply jumping forward like the old code 
would have done. I think we want to maintain that behavior as well?

> -		} else if (PageTransHuge(page))
> -			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
> -
> -		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 {
> +		/* Skip free folios, deal with hugetlb, LRU and non-lru movable folios. */

Can you clarify what "skip free folios" means? For free folios the 
folio_get_nontail_page() shouldn't have succeeded. Did you mean if the 
folio got freed in the meantime?

> +		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");
>   			}
>   		}


-- 
Cheers,

David / dhildenb

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

[Kefeng Wang]

On 2024/8/2 4:23, David Hildenbrand wrote:
> On 25.07.24 03:16, Kefeng Wang wrote:
>> Move isolate_hugetlb() after grab a reference, and 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 | 48 +++++++++++++++------------------------------
>>   1 file changed, 16 insertions(+), 32 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index ccaf4c480aed..057037766efa 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1773,20 +1773,17 @@ 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;
>>       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;
>>           if (!pfn_valid(pfn))
>>               continue;
>>           page = pfn_to_page(pfn);
>> -        folio = page_folio(page);
>> -        head = &folio->page;
>>           /*
>>            * HWPoison pages have elevated reference counts so the 
>> migration would
>> @@ -1808,36 +1805,22 @@ static void do_migrate_range(unsigned long 
>> start_pfn, unsigned long end_pfn)
>>               continue;
>>           }
>> -        if (PageHuge(page)) {
>> -            pfn = page_to_pfn(head) + compound_nr(head) - 1;
>> -            isolate_hugetlb(folio, &source);
>> +        folio = folio_get_nontail_page(page);
>> +        if (!folio)
>>               continue;
> 
> There is one interesting case: 1 GiB hugetlb folios can span multiple 
> memory blocks (e.g., 128 MiB). Offlining individual blocks must work.
> 
> If you do the folio_get_nontail_page() we'd not be able to offline a 
> memory block in the middle anymore, because we'd never try even 
> isolating it.

Indeed, will test this case.

> 
> So likely we have to try getting the head page of a large folio instead 
> (as a fallback if this fails?) and continue from there.
> 
> In case of an free hugetlb tail page we would now iterate each 
> individual page instead of simply jumping forward like the old code 
> would have done. I think we want to maintain that behavior as well?

Yes, only can occur when the first hugetlb page if start_pfn is not head 
page, will reconsider of this part

> 
>> -        } else if (PageTransHuge(page))
>> -            pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
>> -
>> -        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 {
>> +        /* Skip free folios, deal with hugetlb, LRU and non-lru 
>> movable folios. */
> 
> Can you clarify what "skip free folios" means? For free folios the 
> folio_get_nontail_page() shouldn't have succeeded. Did you mean if the 
> folio got freed in the meantime?

I think we could drop the comments here, the original comment is added
by commit 0c0e61958965 ("memory unplug: page offline"), and there is no
increase the reference to page, but with commit 700c2a46e882 ("mem-
hotplug: call isolate_lru_page with elevated refcount"), the comment
could be dropped since the folio can't be freed here.


> 
>> +        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");
>>               }
>>           }
> 
>