Re: [PATCH 4/6] mm: compaction: use isolate_movable_folio() in isolate_migratepages_block()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/3/28 2:49, Vishal Moola wrote:
> On Wed, Mar 27, 2024 at 10:10:32PM +0800, Kefeng Wang wrote:
>> This moves folio_get_nontail_page() before non-lru movable pages check,
>> and directly call isolate_movable_folio() to save compound_head() calls,
>> since the reference count of the non-lru movable page is increased, a
>> folio_put() is need() whether the folio is isolated or not.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/compaction.c | 30 +++++++++++++++---------------
>>   1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 807b58e6eb68..74ac65daaed1 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1097,41 +1097,41 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>>   			}
>>   		}
>>   
>> +		/*
>> +		 * Be careful not to clear PageLRU until after we're
>> +		 * sure the page is not being freed elsewhere -- the
>> +		 * page release code relies on it.
>> +		 */
>> +		folio = folio_get_nontail_page(page);
>> +		if (unlikely(!folio))
>> +			goto isolate_fail;
>> +
> 
> If you wanted to move this, I think this should be part of your first
> patch (or prior to it). It would make your first patch be more sensible as

ok, will re-order the patches.

> is. You could then also consider making isolate_movable_folio() more similar
> to folio_isolate_lru() if you really wanted to.

Maybe just rename it folio_isolate_movable and no more changes now.

Thanks.

> 
>>   		/*
>>   		 * Check may be lockless but that's ok as we recheck later.
>>   		 * It's possible to migrate LRU and non-lru movable pages.
>>   		 * Skip any other type of page
>>   		 */
>> -		if (!PageLRU(page)) {
>> +		if (!folio_test_lru(folio)) {
>>   			/*
>>   			 * __PageMovable can return false positive so we need
>>   			 * to verify it under page_lock.
>>   			 */
>> -			if (unlikely(__PageMovable(page)) &&
>> -					!PageIsolated(page)) {
>> +			if (unlikely(__folio_test_movable(folio)) &&
>> +					!folio_test_isolated(folio)) {
>>   				if (locked) {
>>   					unlock_page_lruvec_irqrestore(locked, flags);
>>   					locked = NULL;
>>   				}
>>   
>> -				if (isolate_movable_page(page, mode)) {
>> -					folio = page_folio(page);
>> +				if (isolate_movable_folio(folio, mode)) {
>> +					folio_put(folio);
>>   					goto isolate_success;
>>   				}
>>   			}
>>   
>> -			goto isolate_fail;
>> +			goto isolate_fail_put;
>>   		}
>>   
>> -		/*
>> -		 * Be careful not to clear PageLRU until after we're
>> -		 * sure the page is not being freed elsewhere -- the
>> -		 * page release code relies on it.
>> -		 */
>> -		folio = folio_get_nontail_page(page);
>> -		if (unlikely(!folio))
>> -			goto isolate_fail;
>> -
>>   		/*
>>   		 * Migration will fail if an anonymous page is pinned in memory,
>>   		 * so avoid taking lru_lock and isolating it unnecessarily in an
>> -- 
>> 2.27.0
>>
>>