[PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Kefeng Wang]

The gigantic page size may larger than memory block size, so memory
offline always fails in this case after commit b2c9e2fbba32 ("mm: make
alloc_contig_range work at pageblock granularity"),

offline_pages
  start_isolate_page_range
    start_isolate_page_range(isolate_before=true)
      isolate [isolate_start, isolate_start + pageblock_nr_pages)
    start_isolate_page_range(isolate_before=false)
      isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
       	__alloc_contig_migrate_range
          isolate_migratepages_range
            isolate_migratepages_block
              isolate_or_dissolve_huge_page
                if (hstate_is_gigantic(h))
                    return -ENOMEM;

In fact, we don't need to migrate page in page range isolation, for
memory offline path, there is do_migrate_range() to move the pages.
For contig allocation, there is another __alloc_contig_migrate_range()
after isolation to migrate the pages. So fix issue by skipping the
__alloc_contig_migrate_range() in isolate_single_pageblock().

Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/page_isolation.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index 39fb8c07aeb7..7e04047977cf 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -403,30 +403,8 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
 			unsigned long head_pfn = page_to_pfn(head);
 			unsigned long nr_pages = compound_nr(head);
 
-			if (head_pfn + nr_pages <= boundary_pfn) {
-				pfn = head_pfn + nr_pages;
-				continue;
-			}
-
-#if defined CONFIG_COMPACTION || defined CONFIG_CMA
-			if (PageHuge(page)) {
-				int page_mt = get_pageblock_migratetype(page);
-				struct compact_control cc = {
-					.nr_migratepages = 0,
-					.order = -1,
-					.zone = page_zone(pfn_to_page(head_pfn)),
-					.mode = MIGRATE_SYNC,
-					.ignore_skip_hint = true,
-					.no_set_skip_hint = true,
-					.gfp_mask = gfp_flags,
-					.alloc_contig = true,
-				};
-				INIT_LIST_HEAD(&cc.migratepages);
-
-				ret = __alloc_contig_migrate_range(&cc, head_pfn,
-							head_pfn + nr_pages, page_mt);
-				if (ret)
-					goto failed;
+			if (head_pfn + nr_pages <= boundary_pfn ||
+			    PageHuge(page)) {
 				pfn = head_pfn + nr_pages;
 				continue;
 			}
@@ -440,7 +418,7 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
 			 */
 			VM_WARN_ON_ONCE_PAGE(PageLRU(page), page);
 			VM_WARN_ON_ONCE_PAGE(__PageMovable(page), page);
-#endif
+
 			goto failed;
 		}
 
-- 
2.27.0

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Andrew Morton]

On Fri, 16 Aug 2024 12:06:25 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> The gigantic page size may larger than memory block size, so memory
> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
> alloc_contig_range work at pageblock granularity"),
> 
> offline_pages
>   start_isolate_page_range
>     start_isolate_page_range(isolate_before=true)
>       isolate [isolate_start, isolate_start + pageblock_nr_pages)
>     start_isolate_page_range(isolate_before=false)
>       isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>        	__alloc_contig_migrate_range
>           isolate_migratepages_range
>             isolate_migratepages_block
>               isolate_or_dissolve_huge_page
>                 if (hstate_is_gigantic(h))
>                     return -ENOMEM;
> 
> In fact, we don't need to migrate page in page range isolation, for
> memory offline path, there is do_migrate_range() to move the pages.
> For contig allocation, there is another __alloc_contig_migrate_range()
> after isolation to migrate the pages. So fix issue by skipping the
> __alloc_contig_migrate_range() in isolate_single_pageblock().
> 
> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")

Should we backport this into -stable kernels?

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Kefeng Wang]

On 2024/8/16 12:58, Andrew Morton wrote:
> On Fri, 16 Aug 2024 12:06:25 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
>> The gigantic page size may larger than memory block size, so memory
>> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
>> alloc_contig_range work at pageblock granularity"),
>>
>> offline_pages
>>    start_isolate_page_range
>>      start_isolate_page_range(isolate_before=true)
>>        isolate [isolate_start, isolate_start + pageblock_nr_pages)
>>      start_isolate_page_range(isolate_before=false)
>>        isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>>         	__alloc_contig_migrate_range
>>            isolate_migratepages_range
>>              isolate_migratepages_block
>>                isolate_or_dissolve_huge_page
>>                  if (hstate_is_gigantic(h))
>>                      return -ENOMEM;
>>
>> In fact, we don't need to migrate page in page range isolation, for
>> memory offline path, there is do_migrate_range() to move the pages.
>> For contig allocation, there is another __alloc_contig_migrate_range()
>> after isolation to migrate the pages. So fix issue by skipping the
>> __alloc_contig_migrate_range() in isolate_single_pageblock().
>>
>> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")
> 
> Should we backport this into -stable kernels?

Better to backport to stable since memory offline always fail in this case.

> 

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[David Hildenbrand]

On 16.08.24 06:06, Kefeng Wang wrote:
> The gigantic page size may larger than memory block size, so memory
> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
> alloc_contig_range work at pageblock granularity"),
> 
> offline_pages
>    start_isolate_page_range
>      start_isolate_page_range(isolate_before=true)
>        isolate [isolate_start, isolate_start + pageblock_nr_pages)
>      start_isolate_page_range(isolate_before=false)
>        isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>         	__alloc_contig_migrate_range
>            isolate_migratepages_range
>              isolate_migratepages_block
>                isolate_or_dissolve_huge_page
>                  if (hstate_is_gigantic(h))
>                      return -ENOMEM;
> 
> In fact, we don't need to migrate page in page range isolation, for
> memory offline path, there is do_migrate_range() to move the pages.
> For contig allocation, there is another __alloc_contig_migrate_range()
> after isolation to migrate the pages. So fix issue by skipping the
> __alloc_contig_migrate_range() in isolate_single_pageblock().
> 
> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   mm/page_isolation.c | 28 +++-------------------------
>   1 file changed, 3 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 39fb8c07aeb7..7e04047977cf 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -403,30 +403,8 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>   			unsigned long head_pfn = page_to_pfn(head);
>   			unsigned long nr_pages = compound_nr(head);
>   
> -			if (head_pfn + nr_pages <= boundary_pfn) {
> -				pfn = head_pfn + nr_pages;
> -				continue;
> -			}
> -
> -#if defined CONFIG_COMPACTION || defined CONFIG_CMA
> -			if (PageHuge(page)) {
> -				int page_mt = get_pageblock_migratetype(page);
> -				struct compact_control cc = {
> -					.nr_migratepages = 0,
> -					.order = -1,
> -					.zone = page_zone(pfn_to_page(head_pfn)),
> -					.mode = MIGRATE_SYNC,
> -					.ignore_skip_hint = true,
> -					.no_set_skip_hint = true,
> -					.gfp_mask = gfp_flags,
> -					.alloc_contig = true,
> -				};
> -				INIT_LIST_HEAD(&cc.migratepages);
> -
> -				ret = __alloc_contig_migrate_range(&cc, head_pfn,
> -							head_pfn + nr_pages, page_mt);
> -				if (ret)
> -					goto failed;

But won't this break alloc_contig_range() then? I would have expected 
that you have to special-case here on the migration reason (MEMORY_OFFLINE).

I remember some dirty details when we're trying to allcoate with a 
single pageblock for alloc_contig_range().

Note that memory offlining always covers pageblocks large than MAX_ORDER 
chunks (which implies full pageblocks) but alloc_contig_range() + CMA 
might only cover (parts of) single pageblocks.

Hoping Zi Yan can review :)

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Kefeng Wang]

On 2024/8/16 18:11, David Hildenbrand wrote:
> On 16.08.24 06:06, Kefeng Wang wrote:
>> The gigantic page size may larger than memory block size, so memory
>> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
>> alloc_contig_range work at pageblock granularity"),
>>
>> offline_pages
>>    start_isolate_page_range
>>      start_isolate_page_range(isolate_before=true)
>>        isolate [isolate_start, isolate_start + pageblock_nr_pages)
>>      start_isolate_page_range(isolate_before=false)
>>        isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>>             __alloc_contig_migrate_range
>>            isolate_migratepages_range
>>              isolate_migratepages_block
>>                isolate_or_dissolve_huge_page
>>                  if (hstate_is_gigantic(h))
>>                      return -ENOMEM;
>>
>> In fact, we don't need to migrate page in page range isolation, for
>> memory offline path, there is do_migrate_range() to move the pages.
>> For contig allocation, there is another __alloc_contig_migrate_range()
>> after isolation to migrate the pages. So fix issue by skipping the
>> __alloc_contig_migrate_range() in isolate_single_pageblock().
>>
>> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock 
>> granularity")
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/page_isolation.c | 28 +++-------------------------
>>   1 file changed, 3 insertions(+), 25 deletions(-)
>>
>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>> index 39fb8c07aeb7..7e04047977cf 100644
>> --- a/mm/page_isolation.c
>> +++ b/mm/page_isolation.c
>> @@ -403,30 +403,8 @@ static int isolate_single_pageblock(unsigned long 
>> boundary_pfn, int flags,
>>               unsigned long head_pfn = page_to_pfn(head);
>>               unsigned long nr_pages = compound_nr(head);
>> -            if (head_pfn + nr_pages <= boundary_pfn) {
>> -                pfn = head_pfn + nr_pages;
>> -                continue;
>> -            }
>> -
>> -#if defined CONFIG_COMPACTION || defined CONFIG_CMA
>> -            if (PageHuge(page)) {
>> -                int page_mt = get_pageblock_migratetype(page);
>> -                struct compact_control cc = {
>> -                    .nr_migratepages = 0,
>> -                    .order = -1,
>> -                    .zone = page_zone(pfn_to_page(head_pfn)),
>> -                    .mode = MIGRATE_SYNC,
>> -                    .ignore_skip_hint = true,
>> -                    .no_set_skip_hint = true,
>> -                    .gfp_mask = gfp_flags,
>> -                    .alloc_contig = true,
>> -                };
>> -                INIT_LIST_HEAD(&cc.migratepages);
>> -
>> -                ret = __alloc_contig_migrate_range(&cc, head_pfn,
>> -                            head_pfn + nr_pages, page_mt);
>> -                if (ret)
>> -                    goto failed;
> 
> But won't this break alloc_contig_range() then? I would have expected 
> that you have to special-case here on the migration reason 
> (MEMORY_OFFLINE).
> 

Yes, this is what I did in rfc, only skip migration for offline path.
but Zi Yan suggested to remove migration totally[1]

[1] 
https://lore.kernel.org/linux-mm/50FEEE33-49CA-48B5-B4C5-964F1BE25D43@nvidia.com/

> I remember some dirty details when we're trying to allcoate with a 
> single pageblock for alloc_contig_range().
> 
> Note that memory offlining always covers pageblocks large than MAX_ORDER 
> chunks (which implies full pageblocks) but alloc_contig_range() + CMA 
> might only cover (parts of) single pageblocks.
> 
> Hoping Zi Yan can review :)
> 

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 5284 bytes --]

On 16 Aug 2024, at 7:30, Kefeng Wang wrote:

> On 2024/8/16 18:11, David Hildenbrand wrote:
>> On 16.08.24 06:06, Kefeng Wang wrote:
>>> The gigantic page size may larger than memory block size, so memory
>>> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
>>> alloc_contig_range work at pageblock granularity"),
>>>
>>> offline_pages
>>>    start_isolate_page_range
>>>      start_isolate_page_range(isolate_before=true)
>>>        isolate [isolate_start, isolate_start + pageblock_nr_pages)
>>>      start_isolate_page_range(isolate_before=false)
>>>        isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>>>             __alloc_contig_migrate_range
>>>            isolate_migratepages_range
>>>              isolate_migratepages_block
>>>                isolate_or_dissolve_huge_page
>>>                  if (hstate_is_gigantic(h))
>>>                      return -ENOMEM;
>>>
>>> In fact, we don't need to migrate page in page range isolation, for
>>> memory offline path, there is do_migrate_range() to move the pages.
>>> For contig allocation, there is another __alloc_contig_migrate_range()
>>> after isolation to migrate the pages. So fix issue by skipping the
>>> __alloc_contig_migrate_range() in isolate_single_pageblock().
>>>
>>> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")
>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> ---
>>>   mm/page_isolation.c | 28 +++-------------------------
>>>   1 file changed, 3 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>> index 39fb8c07aeb7..7e04047977cf 100644
>>> --- a/mm/page_isolation.c
>>> +++ b/mm/page_isolation.c
>>> @@ -403,30 +403,8 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>>>               unsigned long head_pfn = page_to_pfn(head);
>>>               unsigned long nr_pages = compound_nr(head);
>>> -            if (head_pfn + nr_pages <= boundary_pfn) {
>>> -                pfn = head_pfn + nr_pages;
>>> -                continue;
>>> -            }
>>> -
>>> -#if defined CONFIG_COMPACTION || defined CONFIG_CMA
>>> -            if (PageHuge(page)) {
>>> -                int page_mt = get_pageblock_migratetype(page);
>>> -                struct compact_control cc = {
>>> -                    .nr_migratepages = 0,
>>> -                    .order = -1,
>>> -                    .zone = page_zone(pfn_to_page(head_pfn)),
>>> -                    .mode = MIGRATE_SYNC,
>>> -                    .ignore_skip_hint = true,
>>> -                    .no_set_skip_hint = true,
>>> -                    .gfp_mask = gfp_flags,
>>> -                    .alloc_contig = true,
>>> -                };
>>> -                INIT_LIST_HEAD(&cc.migratepages);
>>> -
>>> -                ret = __alloc_contig_migrate_range(&cc, head_pfn,
>>> -                            head_pfn + nr_pages, page_mt);
>>> -                if (ret)
>>> -                    goto failed;
>>
>> But won't this break alloc_contig_range() then? I would have expected that you have to special-case here on the migration reason (MEMORY_OFFLINE).
>>
>
> Yes, this is what I did in rfc, only skip migration for offline path.
> but Zi Yan suggested to remove migration totally[1]
>
> [1] https://lore.kernel.org/linux-mm/50FEEE33-49CA-48B5-B4C5-964F1BE25D43@nvidia.com/
>
>> I remember some dirty details when we're trying to allcoate with a single pageblock for alloc_contig_range().

Most likely I was overthinking about the situation back then. I thought
PageHuge, PageLRU, and __PageMovable all can be bigger than a pageblock,
but in reality only PageHuge can and the gigantic PageHuge is freed as
order-0. This means MIGRATE_ISOLATE pageblocks will get to the right
free list after __alloc_contig_migrate_range(), the one after
start_isolate_page_range().

David, I know we do not have cross-pageblock PageLRU yet (wait until
someone adds PMD-level mTHP). But I am not sure about __PageMovable,
even if you and Johannes told me that __PageMovable has no compound page.
I wonder what are the use cases for __PageMovable. Is it possible for
a driver to mark its cross-pageblock page __PageMovable and provide
->isolate_page and ->migratepage in its struct address_space_operations?
Or it is unsupported, so I should not need to worry about it.

>>
>> Note that memory offlining always covers pageblocks large than MAX_ORDER chunks (which implies full pageblocks) but alloc_contig_range() + CMA might only cover (parts of) single pageblocks.
>>
>> Hoping Zi Yan can review :)

At the moment, I think this is the right clean up.

Acked-by: Zi Yan <ziy@nvidia.com>


Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[David Hildenbrand]

On 16.08.24 17:06, Zi Yan wrote:
> On 16 Aug 2024, at 7:30, Kefeng Wang wrote:
> 
>> On 2024/8/16 18:11, David Hildenbrand wrote:
>>> On 16.08.24 06:06, Kefeng Wang wrote:
>>>> The gigantic page size may larger than memory block size, so memory
>>>> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
>>>> alloc_contig_range work at pageblock granularity"),
>>>>
>>>> offline_pages
>>>>     start_isolate_page_range
>>>>       start_isolate_page_range(isolate_before=true)
>>>>         isolate [isolate_start, isolate_start + pageblock_nr_pages)
>>>>       start_isolate_page_range(isolate_before=false)
>>>>         isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>>>>              __alloc_contig_migrate_range
>>>>             isolate_migratepages_range
>>>>               isolate_migratepages_block
>>>>                 isolate_or_dissolve_huge_page
>>>>                   if (hstate_is_gigantic(h))
>>>>                       return -ENOMEM;
>>>>
>>>> In fact, we don't need to migrate page in page range isolation, for
>>>> memory offline path, there is do_migrate_range() to move the pages.
>>>> For contig allocation, there is another __alloc_contig_migrate_range()
>>>> after isolation to migrate the pages. So fix issue by skipping the
>>>> __alloc_contig_migrate_range() in isolate_single_pageblock().
>>>>
>>>> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")
>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>> ---
>>>>    mm/page_isolation.c | 28 +++-------------------------
>>>>    1 file changed, 3 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>>> index 39fb8c07aeb7..7e04047977cf 100644
>>>> --- a/mm/page_isolation.c
>>>> +++ b/mm/page_isolation.c
>>>> @@ -403,30 +403,8 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>>>>                unsigned long head_pfn = page_to_pfn(head);
>>>>                unsigned long nr_pages = compound_nr(head);
>>>> -            if (head_pfn + nr_pages <= boundary_pfn) {
>>>> -                pfn = head_pfn + nr_pages;
>>>> -                continue;
>>>> -            }
>>>> -
>>>> -#if defined CONFIG_COMPACTION || defined CONFIG_CMA
>>>> -            if (PageHuge(page)) {
>>>> -                int page_mt = get_pageblock_migratetype(page);
>>>> -                struct compact_control cc = {
>>>> -                    .nr_migratepages = 0,
>>>> -                    .order = -1,
>>>> -                    .zone = page_zone(pfn_to_page(head_pfn)),
>>>> -                    .mode = MIGRATE_SYNC,
>>>> -                    .ignore_skip_hint = true,
>>>> -                    .no_set_skip_hint = true,
>>>> -                    .gfp_mask = gfp_flags,
>>>> -                    .alloc_contig = true,
>>>> -                };
>>>> -                INIT_LIST_HEAD(&cc.migratepages);
>>>> -
>>>> -                ret = __alloc_contig_migrate_range(&cc, head_pfn,
>>>> -                            head_pfn + nr_pages, page_mt);
>>>> -                if (ret)
>>>> -                    goto failed;
>>>
>>> But won't this break alloc_contig_range() then? I would have expected that you have to special-case here on the migration reason (MEMORY_OFFLINE).
>>>
>>
>> Yes, this is what I did in rfc, only skip migration for offline path.
>> but Zi Yan suggested to remove migration totally[1]
>>
>> [1] https://lore.kernel.org/linux-mm/50FEEE33-49CA-48B5-B4C5-964F1BE25D43@nvidia.com/
>>
>>> I remember some dirty details when we're trying to allcoate with a single pageblock for alloc_contig_range().
> 
> Most likely I was overthinking about the situation back then. I thought

I'm more than happy if we can remove that code here :)

> PageHuge, PageLRU, and __PageMovable all can be bigger than a pageblock,
> but in reality only PageHuge can and the gigantic PageHuge is freed as
> order-0. 

Does that still hold with Yu's patches to allocate/free gigantic pages 
from CMA using compound pages that are on the list (and likely already 
in mm-unstable)? I did not look at the freeing path of that patchset. As 
the buddy doesn't understand anything larger than MAX_ORDER, I would 
assume that we are fine.

I assume the real issue is when we have a movable allocation (folio) 
that spans multiple pageblocks. For example, when MAX_ORDER is large 
than a single pageblock, like it is on x86.

Besides gigantic pages, I wonder if that can happen. Likely currently 
really only with hugetlb.


This means MIGRATE_ISOLATE pageblocks will get to the right
> free list after __alloc_contig_migrate_range(), the one after
> start_isolate_page_range().
> 
> David, I know we do not have cross-pageblock PageLRU yet (wait until
> someone adds PMD-level mTHP). But I am not sure about __PageMovable,
> even if you and Johannes told me that __PageMovable has no compound page.

I think it's all order-0. Likely we should sanity check that somewhere 
(when setting a folio-page movable?).

For example, the vmware balloon handles 2M pages differently than 4k 
pages. Only the latter is movable.

> I wonder what are the use cases for __PageMovable. Is it possible for
> a driver to mark its cross-pageblock page __PageMovable and provide
> ->isolate_page and ->migratepage in its struct address_space_operations?
> Or it is unsupported, so I should not need to worry about it.

I never tried. We should document and enforce/sanity check that it only 
works with order-0 for now.

> 
>>>
>>> Note that memory offlining always covers pageblocks large than MAX_ORDER chunks (which implies full pageblocks) but alloc_contig_range() + CMA might only cover (parts of) single pageblocks.
>>>
>>> Hoping Zi Yan can review :)
> 
> At the moment, I think this is the right clean up.

I think we want to have some way to catch when it changes. For example, 
can we warn if we find a LRU folio here that is large than a single 
pageblock?

Also, I think we have to document why it works with hugetlb gigantic 
folios / large CMA allocations somewhere (the order-0 stuff you note 
above). Maybe as part of this changelog.

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Yu Zhao]

On Fri, Aug 16, 2024 at 2:12 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 16.08.24 17:06, Zi Yan wrote:
> > On 16 Aug 2024, at 7:30, Kefeng Wang wrote:
> >
> >> On 2024/8/16 18:11, David Hildenbrand wrote:
> >>> On 16.08.24 06:06, Kefeng Wang wrote:
> >>>> The gigantic page size may larger than memory block size, so memory
> >>>> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
> >>>> alloc_contig_range work at pageblock granularity"),
> >>>>
> >>>> offline_pages
> >>>>     start_isolate_page_range
> >>>>       start_isolate_page_range(isolate_before=true)
> >>>>         isolate [isolate_start, isolate_start + pageblock_nr_pages)
> >>>>       start_isolate_page_range(isolate_before=false)
> >>>>         isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
> >>>>              __alloc_contig_migrate_range
> >>>>             isolate_migratepages_range
> >>>>               isolate_migratepages_block
> >>>>                 isolate_or_dissolve_huge_page
> >>>>                   if (hstate_is_gigantic(h))
> >>>>                       return -ENOMEM;
> >>>>
> >>>> In fact, we don't need to migrate page in page range isolation, for
> >>>> memory offline path, there is do_migrate_range() to move the pages.
> >>>> For contig allocation, there is another __alloc_contig_migrate_range()
> >>>> after isolation to migrate the pages. So fix issue by skipping the
> >>>> __alloc_contig_migrate_range() in isolate_single_pageblock().
> >>>>
> >>>> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")
> >>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> >>>> ---
> >>>>    mm/page_isolation.c | 28 +++-------------------------
> >>>>    1 file changed, 3 insertions(+), 25 deletions(-)
> >>>>
> >>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> >>>> index 39fb8c07aeb7..7e04047977cf 100644
> >>>> --- a/mm/page_isolation.c
> >>>> +++ b/mm/page_isolation.c
> >>>> @@ -403,30 +403,8 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
> >>>>                unsigned long head_pfn = page_to_pfn(head);
> >>>>                unsigned long nr_pages = compound_nr(head);
> >>>> -            if (head_pfn + nr_pages <= boundary_pfn) {
> >>>> -                pfn = head_pfn + nr_pages;
> >>>> -                continue;
> >>>> -            }
> >>>> -
> >>>> -#if defined CONFIG_COMPACTION || defined CONFIG_CMA
> >>>> -            if (PageHuge(page)) {
> >>>> -                int page_mt = get_pageblock_migratetype(page);
> >>>> -                struct compact_control cc = {
> >>>> -                    .nr_migratepages = 0,
> >>>> -                    .order = -1,
> >>>> -                    .zone = page_zone(pfn_to_page(head_pfn)),
> >>>> -                    .mode = MIGRATE_SYNC,
> >>>> -                    .ignore_skip_hint = true,
> >>>> -                    .no_set_skip_hint = true,
> >>>> -                    .gfp_mask = gfp_flags,
> >>>> -                    .alloc_contig = true,
> >>>> -                };
> >>>> -                INIT_LIST_HEAD(&cc.migratepages);
> >>>> -
> >>>> -                ret = __alloc_contig_migrate_range(&cc, head_pfn,
> >>>> -                            head_pfn + nr_pages, page_mt);
> >>>> -                if (ret)
> >>>> -                    goto failed;
> >>>
> >>> But won't this break alloc_contig_range() then? I would have expected that you have to special-case here on the migration reason (MEMORY_OFFLINE).
> >>>
> >>
> >> Yes, this is what I did in rfc, only skip migration for offline path.
> >> but Zi Yan suggested to remove migration totally[1]
> >>
> >> [1] https://lore.kernel.org/linux-mm/50FEEE33-49CA-48B5-B4C5-964F1BE25D43@nvidia.com/
> >>
> >>> I remember some dirty details when we're trying to allcoate with a single pageblock for alloc_contig_range().
> >
> > Most likely I was overthinking about the situation back then. I thought
>
> I'm more than happy if we can remove that code here :)
>
> > PageHuge, PageLRU, and __PageMovable all can be bigger than a pageblock,
> > but in reality only PageHuge can and the gigantic PageHuge is freed as
> > order-0.
>
> Does that still hold with Yu's patches to allocate/free gigantic pages
> from CMA using compound pages that are on the list (and likely already
> in mm-unstable)?

Gigantic folios are now freed at pageblock granularity rather than
order-0, as Zi himself stated during the review :)

https://lore.kernel.org/linux-mm/29B680F7-E14D-4CD7-802B-5BBE1E1A3F92@nvidia.com/

> I did not look at the freeing path of that patchset. As
> the buddy doesn't understand anything larger than MAX_ORDER, I would
> assume that we are fine.

Correct.


> I assume the real issue is when we have a movable allocation (folio)
> that spans multiple pageblocks. For example, when MAX_ORDER is large
> than a single pageblock, like it is on x86.
>
> Besides gigantic pages, I wonder if that can happen. Likely currently
> really only with hugetlb.
>
>
> This means MIGRATE_ISOLATE pageblocks will get to the right
> > free list after __alloc_contig_migrate_range(), the one after
> > start_isolate_page_range().
> >
> > David, I know we do not have cross-pageblock PageLRU yet (wait until
> > someone adds PMD-level mTHP). But I am not sure about __PageMovable,
> > even if you and Johannes told me that __PageMovable has no compound page.
>
> I think it's all order-0. Likely we should sanity check that somewhere
> (when setting a folio-page movable?).
>
> For example, the vmware balloon handles 2M pages differently than 4k
> pages. Only the latter is movable.
>
> > I wonder what are the use cases for __PageMovable. Is it possible for
> > a driver to mark its cross-pageblock page __PageMovable and provide
> > ->isolate_page and ->migratepage in its struct address_space_operations?
> > Or it is unsupported, so I should not need to worry about it.
>
> I never tried. We should document and enforce/sanity check that it only
> works with order-0 for now.
>
> >
> >>>
> >>> Note that memory offlining always covers pageblocks large than MAX_ORDER chunks (which implies full pageblocks) but alloc_contig_range() + CMA might only cover (parts of) single pageblocks.
> >>>
> >>> Hoping Zi Yan can review :)
> >
> > At the moment, I think this is the right clean up.
>
> I think we want to have some way to catch when it changes. For example,
> can we warn if we find a LRU folio here that is large than a single
> pageblock?
>
> Also, I think we have to document why it works with hugetlb gigantic
> folios / large CMA allocations somewhere (the order-0 stuff you note
> above). Maybe as part of this changelog.
>
> --
> Cheers,
>
> David / dhildenb
>
>

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 7786 bytes --]

On 16 Aug 2024, at 16:12, David Hildenbrand wrote:

> On 16.08.24 17:06, Zi Yan wrote:
>> On 16 Aug 2024, at 7:30, Kefeng Wang wrote:
>>
>>> On 2024/8/16 18:11, David Hildenbrand wrote:
>>>> On 16.08.24 06:06, Kefeng Wang wrote:
>>>>> The gigantic page size may larger than memory block size, so memory
>>>>> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
>>>>> alloc_contig_range work at pageblock granularity"),
>>>>>
>>>>> offline_pages
>>>>>     start_isolate_page_range
>>>>>       start_isolate_page_range(isolate_before=true)
>>>>>         isolate [isolate_start, isolate_start + pageblock_nr_pages)
>>>>>       start_isolate_page_range(isolate_before=false)
>>>>>         isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>>>>>              __alloc_contig_migrate_range
>>>>>             isolate_migratepages_range
>>>>>               isolate_migratepages_block
>>>>>                 isolate_or_dissolve_huge_page
>>>>>                   if (hstate_is_gigantic(h))
>>>>>                       return -ENOMEM;
>>>>>
>>>>> In fact, we don't need to migrate page in page range isolation, for
>>>>> memory offline path, there is do_migrate_range() to move the pages.
>>>>> For contig allocation, there is another __alloc_contig_migrate_range()
>>>>> after isolation to migrate the pages. So fix issue by skipping the
>>>>> __alloc_contig_migrate_range() in isolate_single_pageblock().
>>>>>
>>>>> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")
>>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>>> ---
>>>>>    mm/page_isolation.c | 28 +++-------------------------
>>>>>    1 file changed, 3 insertions(+), 25 deletions(-)
>>>>>
>>>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>>>> index 39fb8c07aeb7..7e04047977cf 100644
>>>>> --- a/mm/page_isolation.c
>>>>> +++ b/mm/page_isolation.c
>>>>> @@ -403,30 +403,8 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>>>>>                unsigned long head_pfn = page_to_pfn(head);
>>>>>                unsigned long nr_pages = compound_nr(head);
>>>>> -            if (head_pfn + nr_pages <= boundary_pfn) {
>>>>> -                pfn = head_pfn + nr_pages;
>>>>> -                continue;
>>>>> -            }
>>>>> -
>>>>> -#if defined CONFIG_COMPACTION || defined CONFIG_CMA
>>>>> -            if (PageHuge(page)) {
>>>>> -                int page_mt = get_pageblock_migratetype(page);
>>>>> -                struct compact_control cc = {
>>>>> -                    .nr_migratepages = 0,
>>>>> -                    .order = -1,
>>>>> -                    .zone = page_zone(pfn_to_page(head_pfn)),
>>>>> -                    .mode = MIGRATE_SYNC,
>>>>> -                    .ignore_skip_hint = true,
>>>>> -                    .no_set_skip_hint = true,
>>>>> -                    .gfp_mask = gfp_flags,
>>>>> -                    .alloc_contig = true,
>>>>> -                };
>>>>> -                INIT_LIST_HEAD(&cc.migratepages);
>>>>> -
>>>>> -                ret = __alloc_contig_migrate_range(&cc, head_pfn,
>>>>> -                            head_pfn + nr_pages, page_mt);
>>>>> -                if (ret)
>>>>> -                    goto failed;
>>>>
>>>> But won't this break alloc_contig_range() then? I would have expected that you have to special-case here on the migration reason (MEMORY_OFFLINE).
>>>>
>>>
>>> Yes, this is what I did in rfc, only skip migration for offline path.
>>> but Zi Yan suggested to remove migration totally[1]
>>>
>>> [1] https://lore.kernel.org/linux-mm/50FEEE33-49CA-48B5-B4C5-964F1BE25D43@nvidia.com/
>>>
>>>> I remember some dirty details when we're trying to allcoate with a single pageblock for alloc_contig_range().
>>
>> Most likely I was overthinking about the situation back then. I thought
>
> I'm more than happy if we can remove that code here :)
>
>> PageHuge, PageLRU, and __PageMovable all can be bigger than a pageblock,
>> but in reality only PageHuge can and the gigantic PageHuge is freed as
>> order-0.
>
> Does that still hold with Yu's patches to allocate/free gigantic pages from CMA using compound pages that are on the list (and likely already in mm-unstable)? I did not look at the freeing path of that patchset. As the buddy doesn't understand anything larger than MAX_ORDER, I would assume that we are fine.
>
> I assume the real issue is when we have a movable allocation (folio) that spans multiple pageblocks. For example, when MAX_ORDER is large than a single pageblock, like it is on x86.
>
> Besides gigantic pages, I wonder if that can happen. Likely currently really only with hugetlb.

It is still OK after I checked Yu’s patch. The patch uses split_large_buddy()
to free pages in pageblock granularity. That prevents pageblocks with different
migratetypes being merged.

>
>
> This means MIGRATE_ISOLATE pageblocks will get to the right
>> free list after __alloc_contig_migrate_range(), the one after
>> start_isolate_page_range().
>>
>> David, I know we do not have cross-pageblock PageLRU yet (wait until
>> someone adds PMD-level mTHP). But I am not sure about __PageMovable,
>> even if you and Johannes told me that __PageMovable has no compound page.
>
> I think it's all order-0. Likely we should sanity check that somewhere (when setting a folio-page movable?).
>
> For example, the vmware balloon handles 2M pages differently than 4k pages. Only the latter is movable.

Got it.

>
>> I wonder what are the use cases for __PageMovable. Is it possible for
>> a driver to mark its cross-pageblock page __PageMovable and provide
>> ->isolate_page and ->migratepage in its struct address_space_operations?
>> Or it is unsupported, so I should not need to worry about it.
>
> I never tried. We should document and enforce/sanity check that it only works with order-0 for now.

I tried when I was developing the commit b2c9e2fbba32 ("mm: make alloc_contig_range
work at pageblock granularity") and it worked (see https://github.com/x-y-z/kernel-modules/blob/pageblock_test/pref-test.c#L52). That led to the complicated
code in isolate_single_pageblock().

>
>>
>>>>
>>>> Note that memory offlining always covers pageblocks large than MAX_ORDER chunks (which implies full pageblocks) but alloc_contig_range() + CMA might only cover (parts of) single pageblocks.
>>>>
>>>> Hoping Zi Yan can review :)
>>
>> At the moment, I think this is the right clean up.
>
> I think we want to have some way to catch when it changes. For example, can we warn if we find a LRU folio here that is large than a single pageblock?

Definitely. We already have

VM_WARN_ON_ONCE_PAGE(PageLRU(page), page);
VM_WARN_ON_ONCE_PAGE(__PageMovable(page), page);

when last time Johannes did the clean up.

I agree that we will need some WARN_ON_ONCE in __SetPageMovable to check
if any compound page is passed in.

For > pageblock_order PageLRU, maybe a check in __folio_rmap_sanity_checks()?

>
> Also, I think we have to document why it works with hugetlb gigantic folios / large CMA allocations somewhere (the order-0 stuff you note above). Maybe as part of this changelog.

I agree.


Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[David Hildenbrand]

On 16.08.24 13:30, Kefeng Wang wrote:
> 
> 
> On 2024/8/16 18:11, David Hildenbrand wrote:
>> On 16.08.24 06:06, Kefeng Wang wrote:
>>> The gigantic page size may larger than memory block size, so memory
>>> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
>>> alloc_contig_range work at pageblock granularity"),
>>>
>>> offline_pages
>>>     start_isolate_page_range
>>>       start_isolate_page_range(isolate_before=true)
>>>         isolate [isolate_start, isolate_start + pageblock_nr_pages)
>>>       start_isolate_page_range(isolate_before=false)
>>>         isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>>>              __alloc_contig_migrate_range
>>>             isolate_migratepages_range
>>>               isolate_migratepages_block
>>>                 isolate_or_dissolve_huge_page
>>>                   if (hstate_is_gigantic(h))
>>>                       return -ENOMEM;
>>>
>>> In fact, we don't need to migrate page in page range isolation, for
>>> memory offline path, there is do_migrate_range() to move the pages.
>>> For contig allocation, there is another __alloc_contig_migrate_range()
>>> after isolation to migrate the pages. So fix issue by skipping the
>>> __alloc_contig_migrate_range() in isolate_single_pageblock().
>>>
>>> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock
>>> granularity")
>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> ---
>>>    mm/page_isolation.c | 28 +++-------------------------
>>>    1 file changed, 3 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>> index 39fb8c07aeb7..7e04047977cf 100644
>>> --- a/mm/page_isolation.c
>>> +++ b/mm/page_isolation.c
>>> @@ -403,30 +403,8 @@ static int isolate_single_pageblock(unsigned long
>>> boundary_pfn, int flags,
>>>                unsigned long head_pfn = page_to_pfn(head);
>>>                unsigned long nr_pages = compound_nr(head);
>>> -            if (head_pfn + nr_pages <= boundary_pfn) {
>>> -                pfn = head_pfn + nr_pages;
>>> -                continue;
>>> -            }
>>> -
>>> -#if defined CONFIG_COMPACTION || defined CONFIG_CMA
>>> -            if (PageHuge(page)) {
>>> -                int page_mt = get_pageblock_migratetype(page);
>>> -                struct compact_control cc = {
>>> -                    .nr_migratepages = 0,
>>> -                    .order = -1,
>>> -                    .zone = page_zone(pfn_to_page(head_pfn)),
>>> -                    .mode = MIGRATE_SYNC,
>>> -                    .ignore_skip_hint = true,
>>> -                    .no_set_skip_hint = true,
>>> -                    .gfp_mask = gfp_flags,
>>> -                    .alloc_contig = true,
>>> -                };
>>> -                INIT_LIST_HEAD(&cc.migratepages);
>>> -
>>> -                ret = __alloc_contig_migrate_range(&cc, head_pfn,
>>> -                            head_pfn + nr_pages, page_mt);
>>> -                if (ret)
>>> -                    goto failed;
>>
>> But won't this break alloc_contig_range() then? I would have expected
>> that you have to special-case here on the migration reason
>> (MEMORY_OFFLINE).
>>
> 
> Yes, this is what I did in rfc, only skip migration for offline path.
> but Zi Yan suggested to remove migration totally[1]

Please distill some of that in the patch description. Right now you only 
talk about memory offlining and don't cover why alloc_contig_range() is 
fine as well with this change.

Let me explore the details in the meantime ... :)

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Kefeng Wang]

On 2024/8/17 3:45, David Hildenbrand wrote:
> On 16.08.24 13:30, Kefeng Wang wrote:
>>
>>
>> On 2024/8/16 18:11, David Hildenbrand wrote:
>>> On 16.08.24 06:06, Kefeng Wang wrote:
>>>> The gigantic page size may larger than memory block size, so memory
>>>> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
>>>> alloc_contig_range work at pageblock granularity"),
>>>>
>>>> offline_pages
>>>>     start_isolate_page_range
>>>>       start_isolate_page_range(isolate_before=true)
>>>>         isolate [isolate_start, isolate_start + pageblock_nr_pages)
>>>>       start_isolate_page_range(isolate_before=false)
>>>>         isolate [isolate_end - pageblock_nr_pages, isolate_end) 
>>>> pageblock
>>>>              __alloc_contig_migrate_range
>>>>             isolate_migratepages_range
>>>>               isolate_migratepages_block
>>>>                 isolate_or_dissolve_huge_page
>>>>                   if (hstate_is_gigantic(h))
>>>>                       return -ENOMEM;
>>>>
>>>> In fact, we don't need to migrate page in page range isolation, for
>>>> memory offline path, there is do_migrate_range() to move the pages.
>>>> For contig allocation, there is another __alloc_contig_migrate_range()
>>>> after isolation to migrate the pages. So fix issue by skipping the
>>>> __alloc_contig_migrate_range() in isolate_single_pageblock().

...

> 
> Please distill some of that in the patch description. Right now you only 
> talk about memory offlining and don't cover why alloc_contig_range() is 
> fine as well with this change.

Borrowing some word from Zi,


PageHuge(gigantic) can bigger than a pageblock, the gigantic PageHuge is 
freed as order-0. This means MIGRATE_ISOLATE pageblocks will get to the 
right free list after __alloc_contig_migrate_range(), the one after
start_isolate_page_range() for alloc_contig_range(), this is same as in
memory offline, it has own path to isolate/migrate used page and 
dissolve the free hugepages, so the migration code in 
isolate_single_pageblock() is not needed, let's cleanup it and which 
also fix the above the issue.

Please correct me or help to write better description, thanks.

> 
> Let me explore the details in the meantime ... :)
> 

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 2968 bytes --]

On 17 Aug 2024, at 2:13, Kefeng Wang wrote:

> On 2024/8/17 3:45, David Hildenbrand wrote:
>> On 16.08.24 13:30, Kefeng Wang wrote:
>>>
>>>
>>> On 2024/8/16 18:11, David Hildenbrand wrote:
>>>> On 16.08.24 06:06, Kefeng Wang wrote:
>>>>> The gigantic page size may larger than memory block size, so memory
>>>>> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
>>>>> alloc_contig_range work at pageblock granularity"),
>>>>>
>>>>> offline_pages
>>>>>     start_isolate_page_range
>>>>>       start_isolate_page_range(isolate_before=true)
>>>>>         isolate [isolate_start, isolate_start + pageblock_nr_pages)
>>>>>       start_isolate_page_range(isolate_before=false)
>>>>>         isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>>>>>              __alloc_contig_migrate_range
>>>>>             isolate_migratepages_range
>>>>>               isolate_migratepages_block
>>>>>                 isolate_or_dissolve_huge_page
>>>>>                   if (hstate_is_gigantic(h))
>>>>>                       return -ENOMEM;
>>>>>
>>>>> In fact, we don't need to migrate page in page range isolation, for
>>>>> memory offline path, there is do_migrate_range() to move the pages.
>>>>> For contig allocation, there is another __alloc_contig_migrate_range()
>>>>> after isolation to migrate the pages. So fix issue by skipping the
>>>>> __alloc_contig_migrate_range() in isolate_single_pageblock().
>
> ...
>
>>
>> Please distill some of that in the patch description. Right now you only talk about memory offlining and don't cover why alloc_contig_range() is fine as well with this change.
>
> Borrowing some word from Zi,
>
>
> PageHuge(gigantic) can bigger than a pageblock, the gigantic PageHuge is freed as order-0. This means MIGRATE_ISOLATE pageblocks will get to the right free list after __alloc_contig_migrate_range(), the one after
> start_isolate_page_range() for alloc_contig_range(), this is same as in
> memory offline, it has own path to isolate/migrate used page and dissolve the free hugepages, so the migration code in isolate_single_pageblock() is not needed, let's cleanup it and which also fix the above the issue.
>
> Please correct me or help to write better description, thanks.

How about?

Gigantic PageHuge is bigger than a pageblock, but since it is freed as order-0 pages,
its pageblocks after being freed will get to the right free list. There is no need
to have special handling code for them in start_isolate_page_range(). For both
alloc_contig_range() and memory offline cases, the migration code after
start_isolate_page_range() will be able to migrate gigantic PageHuge when possible.
Let's clean up start_isolate_page_range() and fix the aforementioned memory offline
failure issue all together.


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Kefeng Wang]

On 2024/8/18 7:58, Zi Yan wrote:
> On 17 Aug 2024, at 2:13, Kefeng Wang wrote:
> 
>> On 2024/8/17 3:45, David Hildenbrand wrote:
>>> On 16.08.24 13:30, Kefeng Wang wrote:
>>>>
>>>>
>>>> On 2024/8/16 18:11, David Hildenbrand wrote:
>>>>> On 16.08.24 06:06, Kefeng Wang wrote:
>>>>>> The gigantic page size may larger than memory block size, so memory
>>>>>> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
>>>>>> alloc_contig_range work at pageblock granularity"),
>>>>>>
>>>>>> offline_pages
>>>>>>      start_isolate_page_range
>>>>>>        start_isolate_page_range(isolate_before=true)
>>>>>>          isolate [isolate_start, isolate_start + pageblock_nr_pages)
>>>>>>        start_isolate_page_range(isolate_before=false)
>>>>>>          isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>>>>>>               __alloc_contig_migrate_range
>>>>>>              isolate_migratepages_range
>>>>>>                isolate_migratepages_block
>>>>>>                  isolate_or_dissolve_huge_page
>>>>>>                    if (hstate_is_gigantic(h))
>>>>>>                        return -ENOMEM;
>>>>>>
>>>>>> In fact, we don't need to migrate page in page range isolation, for
>>>>>> memory offline path, there is do_migrate_range() to move the pages.
>>>>>> For contig allocation, there is another __alloc_contig_migrate_range()
>>>>>> after isolation to migrate the pages. So fix issue by skipping the
>>>>>> __alloc_contig_migrate_range() in isolate_single_pageblock().
>>
>> ...
>>
>>>
>>> Please distill some of that in the patch description. Right now you only talk about memory offlining and don't cover why alloc_contig_range() is fine as well with this change.
>>
>> Borrowing some word from Zi,
>>
>>
>> PageHuge(gigantic) can bigger than a pageblock, the gigantic PageHuge is freed as order-0. This means MIGRATE_ISOLATE pageblocks will get to the right free list after __alloc_contig_migrate_range(), the one after
>> start_isolate_page_range() for alloc_contig_range(), this is same as in
>> memory offline, it has own path to isolate/migrate used page and dissolve the free hugepages, so the migration code in isolate_single_pageblock() is not needed, let's cleanup it and which also fix the above the issue.
>>
>> Please correct me or help to write better description, thanks.
> 
> How about?
> 
> Gigantic PageHuge is bigger than a pageblock, but since it is freed as order-0 pages,
> its pageblocks after being freed will get to the right free list. There is no need
> to have special handling code for them in start_isolate_page_range(). For both
> alloc_contig_range() and memory offline cases, the migration code after
> start_isolate_page_range() will be able to migrate gigantic PageHuge when possible.
> Let's clean up start_isolate_page_range() and fix the aforementioned memory offline
> failure issue all together.

Thanks Zi, it is better, will update.
> 
> 
> --
> Best Regards,
> Yan, Zi

Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[Andrew Morton]

On Sat, 17 Aug 2024 19:58:07 -0400 Zi Yan <ziy@nvidia.com> wrote:

> > Please correct me or help to write better description, thanks.
> 
> How about?
> 
> Gigantic PageHuge is bigger than a pageblock, but since it is freed as order-0 pages,
> its pageblocks after being freed will get to the right free list. There is no need
> to have special handling code for them in start_isolate_page_range(). For both
> alloc_contig_range() and memory offline cases, the migration code after
> start_isolate_page_range() will be able to migrate gigantic PageHuge when possible.
> Let's clean up start_isolate_page_range() and fix the aforementioned memory offline
> failure issue all together.

Thanks, I updated the changelog with the above.