Re: [PATCH v2 1/5] mm: compaction: get reference before non LRU movable folio isolation

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

On 2024/8/31 22:04, Matthew Wilcox wrote:
> On Thu, Aug 29, 2024 at 10:54:52PM +0800, Kefeng Wang wrote:
>> Non-LRU movable folio isolation will fail if it can't grab a reference
>> in isolate_movable_page(), so folio_get_nontail_page() could be called
>> ahead to unify the handling of non-LRU movable/LRU folio isolation a bit,
>> this is also prepare to convert isolate_movable_page() to take a folio.
>> Since the reference count of the non-LRU movable folio is increased,
>> a folio_put() is needed whether the folio is isolated or not.
> 
> There's a reason I stopped where I did when converting this function
> to use folios.  Usually I would explain, but I think it would do you
> good to think about why for a bit.

Hm, I don't find the reason,

The major change is that we move folio_get_nontail_page ahead, so we
may try add a reference for each page, it always fails to isolate
with/without this changes, so I suppose that there is no issue here,
for other cases, PageBuddy/PageHuge is also handled before !PageLRU,
although the check is lockless, we will re-check under lock. so is
some page's reference can't try to grab or there are some special
handling for movable page.

The minimized changes could be something like below, just add a new
folio_get_nontail_page() before isolate_movable_page(), other patches
don't need to be changed.

diff --git a/mm/compaction.c b/mm/compaction.c
index a2b16b08cbbf..68331ba331e5 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1091,9 +1091,13 @@ isolate_migratepages_block(struct compact_control 
*cc, unsigned long low_pfn,
                                         locked = NULL;
                                 }

-                               if (isolate_movable_page(page, mode)) {
-                                       folio = page_folio(page);
-                                       goto isolate_success;
+                               folio = folio_get_nontail_page(page);
+                               if (folio) {
+                                       if 
(isolate_movable_page(&folio->page, mode)) {
+                                               folio_put(folio);
+                                               goto isolate_success;
+                                       }
+                                       goto isolate_fail_put;
                                 }
                         }


but I am not clear about the reason why this has issue, could you share
more tips, thank.


> 
> Andrew, please drop these patches.
> 
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/compaction.c | 38 +++++++++++++++++++-------------------
>>   1 file changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index a2b16b08cbbf..8998574da065 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1074,41 +1074,41 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>>   			}
>>   		}
>>   
>> +		/*
>> +		 * Be careful not to clear lru flag until after we're
>> +		 * sure the folio is not being freed elsewhere -- the
>> +		 * folio release code relies on it.
>> +		 */
>> +		folio = folio_get_nontail_page(page);
>> +		if (unlikely(!folio))
>> +			goto isolate_fail;
>> +
>>   		/*
>>   		 * 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
>> +		 * It's possible to migrate LRU and non-lru movable folioss.
>> +		 * Skip any other type of folios.
>>   		 */
>> -		if (!PageLRU(page)) {
>> +		if (!folio_test_lru(folio)) {
>>   			/*
>> -			 * __PageMovable can return false positive so we need
>> -			 * to verify it under page_lock.
>> +			 * __folio_test_movable 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_page(&folio->page, 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
>>
>