Re: [PATCH 2/6] mm: memory_hotplug: use more folio in do_migrate_range()

From: Kefeng Wang <wangkefeng.wang@huawei.com>
To: Zi Yan <ziy@nvidia.com>, Matthew Wilcox <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Miaohe Lin <linmiaohe@huawei.com>,
	Naoya Horiguchi <naoya.horiguchi@nec.com>,
	David Hildenbrand <david@redhat.com>,
	Oscar Salvador <osalvador@suse.de>,
	Hugh Dickins <hughd@google.com>, Jonathan Corbet <corbet@lwn.net>,
	<linux-mm@kvack.org>, <linux-doc@vger.kernel.org>,
	Baolin Wang <baolin.wang@linux.alibaba.com>
Subject: Re: [PATCH 2/6] mm: memory_hotplug: use more folio in do_migrate_range()
Date: Thu, 28 Mar 2024 13:06:21 +0800	[thread overview]
Message-ID: <3b29afcd-7c40-44be-8886-72df7336a85f@huawei.com> (raw)
In-Reply-To: <1AF9E259-6189-4558-8929-598679808C06@nvidia.com>

On 2024/3/27 23:10, Zi Yan wrote:
> On 27 Mar 2024, at 10:54, Matthew Wilcox wrote:
> 
>> On Wed, Mar 27, 2024 at 10:45:42AM -0400, Zi Yan wrote:
>>>>   	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
>>>> -		struct folio *folio;
>>>> +		struct page *page, *head;
>>>
>>> You could get rid of head too. It is only used to calculate next pfn,
>>> so pfn = folio_to_pfn(folio) + folio_nr_pages(folio) - 1 would work.
>>>
>>> And the PageHuge(page) and PageTransHuge(page) can be simplified, since
>>> their pfn calculations are the same. Something like:
>>>
>>> if (folio_test_large(folio)) {
>>> 	pfn = folio_to_pfn(folio) + folio_nr_pages(folio) - 1;
>>> 	if (folio_test_hugetlb(folio)) {
>>> 		isolate_hugetlb(folio, &source);
>>> 		continue;
>>> 	}
>>> }
>>
>> How much of this is safe without a refcount on the folio?
> 
> folio_to_pfn() should be fine, isolate_hugetlb() checks the folio
> under hugetlb_lock, but folio_nr_pages() might return a bogus number
> that makes pfn go beyond end_pfn and ends for loop early. The code
> below increases the refcount, so it might be better to move this
> part of the code after refcount is increased.

The PageHWPoison() is per-page check, for hugetlb, it will directly
try to isolate and ignore the PageHWPoison check, I'm not sure about
moveing PageHWPoison ahead, we need to take the i_mmap_lock_write and
add TTU_RMAP_LOCKED for for hugetlb pages in shared mappings when
try_to_unmap(), but now hugetlb pages won't unmap if there is posion
page, if the get_page_unless_zero() is moved ahead, the logical need be
changed a lot, this minimize changes aim to remove isolate_lru/movable_page,
so could we keep it simple, but as your suggested, we could do more
optimization about do_migrate_range() in the next.

Thanks.

> 
> --
> Best Regards,
> Yan, Zi