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

Yes, that part I remember.

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

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

Also, make sure to test with free hugetlb folios.

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

Feel free to send a fixup for v2 instead.

-- 
Cheers,

David / dhildenb