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

[David Hildenbrand <david@redhat.com>]

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

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

Is there no way to avoid it?

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

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

if (!folio)
	continue;


And then just dropping the reference unconditionally.

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


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

-- 
Cheers,

David / dhildenb