[PATCH v2 0/5] mm: convert to folio_isolate_movable()

[PATCH v2 0/5] mm: convert to folio_isolate_movable()

[Kefeng Wang]

Convert to the new folio_isolate_movable() to get rid of
isolate_movable_page().

Based on next-20240829.

v2:
- get page reference before isolate_movable_page, per Baolin
- since isolate_movable_page() callers with an elevated refcount
  of folio, directly use folio_get() and check folio reference in
  folio_isolate_movable(), per Vishal

Kefeng Wang (5):
  mm: compaction: get reference before non LRU movable folio isolation
  mm: migrate: add folio_isolate_movable()
  mm: migrate: convert to folio_isolate_movable()
  mm: compaction: convert to folio_isolate_movable()
  mm: migrate: remove isolate_movable_page()

 include/linux/migrate.h |  5 +++--
 mm/compaction.c         | 38 ++++++++++++++++----------------
 mm/migrate.c            | 49 +++++++++++++++++++----------------------
 3 files changed, 45 insertions(+), 47 deletions(-)

-- 
2.27.0

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

[Kefeng Wang]

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.

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

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

[Matthew Wilcox]

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.

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
> 

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

[Andrew Morton]

On Sat, 31 Aug 2024 15:04:16 +0100 Matthew Wilcox <willy@infradead.org> 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.

Can we have a code comment explaining this thinking to other readers?

> Andrew, please drop these patches.

Sure.

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

[Kefeng Wang]

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
>>
> 

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

[Kefeng Wang]

On 2024/9/2 18:44, Kefeng Wang wrote:
> 
> 
> 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.

Hi Matthew, could you explain it, many thanks.

> 
> 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
>>>
>>
> 

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

[Matthew Wilcox]

On Mon, Sep 02, 2024 at 06:44:09PM +0800, Kefeng Wang wrote:
> 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,

You haven't considered the effect on others.  Taking the refcount on a
page will necessarily dirty the cacheline.  This is compaction code, so
someone else may have this page allocated.  The check is done without a
refcount in order to minimise the effect if this page cannot be migrated.

Try doing this on a NUMA system to really see the effects.

More broadly, the problem is that you're sending patches faster than I
can review them, and Andrew is picking them up.  I don't know what to
do about that.

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

[Kefeng Wang]

On 2024/9/5 3:02, Matthew Wilcox wrote:
> On Mon, Sep 02, 2024 at 06:44:09PM +0800, Kefeng Wang wrote:
>> 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,
> 
> You haven't considered the effect on others.  Taking the refcount on a
> page will necessarily dirty the cacheline.  This is compaction code, so
> someone else may have this page allocated.  The check is done without a
> refcount in order to minimise the effect if this page cannot be migrated.
> 

Indeed, I asked whether the taking the refcount on a page
unconditionally is accepted or not in v1, but I should wait for more
time to see if there are more comments, thanks for your explanation,
will consider not only the current code modification, but also other
impacts of changes as much as possible.

> Try doing this on a NUMA system to really see the effects.
> 
> More broadly, the problem is that you're sending patches faster than I
> can review them, and Andrew is picking them up.  I don't know what to
> do about that.

OK, I will slowdown for the new version to collect more comments.

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

[Baolin Wang]

On 2024/8/29 22:54, 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.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>

The changes look good to me. However, need to address Matthew's concerns 
first.

[PATCH v2 2/5] mm: migrate: add folio_isolate_movable()

[Kefeng Wang]

Like isolate_lru_page(), make isolate_movable_page() as a wrapper
around folio_isolate_movable(), since isolate_movable_page() always
fails on a tail page, return immediately for a tail page in the
warpper, and the wrapper will be removed once all callers are
converted to folio_isolate_movable().

Note all isolate_movable_page() users increased page reference, so
replace redundant folio_get_nontail_page() with folio_get() and add
a reference count check into folio_isolate_movable().

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/linux/migrate.h |  4 +++
 mm/migrate.c            | 54 +++++++++++++++++++++++------------------
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 002e49b2ebd9..0a33f751596c 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -70,6 +70,7 @@ int migrate_pages(struct list_head *l, new_folio_t new, free_folio_t free,
 		  unsigned int *ret_succeeded);
 struct folio *alloc_migration_target(struct folio *src, unsigned long private);
 bool isolate_movable_page(struct page *page, isolate_mode_t mode);
+bool folio_isolate_movable(struct folio *folio, isolate_mode_t mode);
 bool isolate_folio_to_list(struct folio *folio, struct list_head *list);
 
 int migrate_huge_page_move_mapping(struct address_space *mapping,
@@ -92,6 +93,9 @@ static inline struct folio *alloc_migration_target(struct folio *src,
 	{ return NULL; }
 static inline bool isolate_movable_page(struct page *page, isolate_mode_t mode)
 	{ return false; }
+static inline bool folio_isolate_movable(struct folio *folio,
+					 isolate_mode_t mode)
+	{ return false; }
 static inline bool isolate_folio_to_list(struct folio *folio, struct list_head *list)
 	{ return false; }
 
diff --git a/mm/migrate.c b/mm/migrate.c
index 6f9c62c746be..704102cc3951 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -58,31 +58,30 @@
 
 #include "internal.h"
 
-bool isolate_movable_page(struct page *page, isolate_mode_t mode)
+/**
+ * folio_isolate_movable() - Try to isolate a non-lru movable folio.
+ * @folio: Folio to isolate.
+ *
+ * Must be called with an elevated refcount on the folio.
+ *
+ * Return: true if the folio was isolated, false otherwise
+ */
+bool folio_isolate_movable(struct folio *folio, isolate_mode_t mode)
 {
-	struct folio *folio = folio_get_nontail_page(page);
 	const struct movable_operations *mops;
 
-	/*
-	 * Avoid burning cycles with pages that are yet under __free_pages(),
-	 * or just got freed under us.
-	 *
-	 * In case we 'win' a race for a movable page being freed under us and
-	 * raise its refcount preventing __free_pages() from doing its job
-	 * the put_page() at the end of this block will take care of
-	 * release this page, thus avoiding a nasty leakage.
-	 */
-	if (!folio)
-		goto out;
+	VM_BUG_ON_FOLIO(!folio_ref_count(folio), folio);
+
+	folio_get(folio);
 
 	if (unlikely(folio_test_slab(folio)))
 		goto out_putfolio;
 	/* Pairs with smp_wmb() in slab freeing, e.g. SLUB's __free_slab() */
 	smp_rmb();
 	/*
-	 * Check movable flag before taking the page lock because
-	 * we use non-atomic bitops on newly allocated page flags so
-	 * unconditionally grabbing the lock ruins page's owner side.
+	 * Check movable flag before taking the folio lock because
+	 * we use non-atomic bitops on newly allocated folio flags so
+	 * unconditionally grabbing the lock ruins folio's owner side.
 	 */
 	if (unlikely(!__folio_test_movable(folio)))
 		goto out_putfolio;
@@ -92,15 +91,15 @@ bool isolate_movable_page(struct page *page, isolate_mode_t mode)
 		goto out_putfolio;
 
 	/*
-	 * As movable pages are not isolated from LRU lists, concurrent
-	 * compaction threads can race against page migration functions
-	 * as well as race against the releasing a page.
+	 * As movable folios are not isolated from LRU lists, concurrent
+	 * compaction threads can race against folio migration functions
+	 * as well as race against the releasing a folio.
 	 *
-	 * In order to avoid having an already isolated movable page
+	 * In order to avoid having an already isolated movable folio
 	 * being (wrongly) re-isolated while it is under migration,
-	 * or to avoid attempting to isolate pages being released,
-	 * lets be sure we have the page lock
-	 * before proceeding with the movable page isolation steps.
+	 * or to avoid attempting to isolate folios being released,
+	 * lets be sure we have the folio lock
+	 * before proceeding with the movable folio isolation steps.
 	 */
 	if (unlikely(!folio_trylock(folio)))
 		goto out_putfolio;
@@ -125,10 +124,17 @@ bool isolate_movable_page(struct page *page, isolate_mode_t mode)
 	folio_unlock(folio);
 out_putfolio:
 	folio_put(folio);
-out:
 	return false;
 }
 
+bool isolate_movable_page(struct page *page, isolate_mode_t mode)
+{
+	if (PageTail(page))
+		return false;
+
+	return folio_isolate_movable((struct folio *)page, mode);
+}
+
 static void putback_movable_folio(struct folio *folio)
 {
 	const struct movable_operations *mops = folio_movable_ops(folio);
-- 
2.27.0

Re: [PATCH v2 2/5] mm: migrate: add folio_isolate_movable()

[Matthew Wilcox]

On Thu, Aug 29, 2024 at 10:54:53PM +0800, Kefeng Wang wrote:
> Like isolate_lru_page(), make isolate_movable_page() as a wrapper
> around folio_isolate_movable(), since isolate_movable_page() always

But not everything that is movable is a folio.  Look at who sets movable
ops to see counterexamples.

Re: [PATCH v2 2/5] mm: migrate: add folio_isolate_movable()

[Kefeng Wang]

On 2024/9/5 3:05, Matthew Wilcox wrote:
> On Thu, Aug 29, 2024 at 10:54:53PM +0800, Kefeng Wang wrote:
>> Like isolate_lru_page(), make isolate_movable_page() as a wrapper
>> around folio_isolate_movable(), since isolate_movable_page() always
> 
> But not everything that is movable is a folio.  Look at who sets movable
> ops to see counterexamples.

Confused, balloon, zsmalloc and z3fold(may deprecate) will set movable,
but for core migration, most functions are converted to folio, and all
callers of movable_operations are from folio to page too except
isolate_movable_page(),

   mops->migrate_page(&dst->page, &src->page, mode) // move_to_new_folio()
   mops->isolate_page(&folio->page, mode) // isolate_movable_page()
   mops->putback_page(&folio->page) // putback_movable_folio()

This try to unify them, so I move folio_get_nontail_page() out to 
compaction code, then isolate_movable_page-> folio_migrate_movable.
could you explain more about your comments?  thanks.

[PATCH v2 3/5] mm: migrate: convert to folio_isolate_movable()

[Kefeng Wang]

Directly use folio_isolate_movable() in isolate_folio_to_list().

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/migrate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 704102cc3951..4cac7f749620 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -196,8 +196,7 @@ bool isolate_folio_to_list(struct folio *folio, struct list_head *list)
 	if (lru)
 		isolated = folio_isolate_lru(folio);
 	else
-		isolated = isolate_movable_page(&folio->page,
-						ISOLATE_UNEVICTABLE);
+		isolated = folio_isolate_movable(folio, ISOLATE_UNEVICTABLE);
 
 	if (!isolated)
 		return false;
-- 
2.27.0

[PATCH v2 4/5] mm: compaction: convert to folio_isolate_movable()

[Kefeng Wang]

Directly use folio_isolate_movable() in isolate_migratepages_block().

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/compaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 8998574da065..f2af4493a878 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1100,7 +1100,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 					locked = NULL;
 				}
 
-				if (isolate_movable_page(&folio->page, mode)) {
+				if (folio_isolate_movable(folio, mode)) {
 					folio_put(folio);
 					goto isolate_success;
 				}
-- 
2.27.0

[PATCH v2 5/5] mm: migrate: remove isolate_movable_page()

[Kefeng Wang]

There are no more callers of isolate_movable_page(), remove it.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/linux/migrate.h | 3 ---
 mm/migrate.c            | 8 --------
 2 files changed, 11 deletions(-)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 0a33f751596c..dca6712b8563 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -69,7 +69,6 @@ int migrate_pages(struct list_head *l, new_folio_t new, free_folio_t free,
 		  unsigned long private, enum migrate_mode mode, int reason,
 		  unsigned int *ret_succeeded);
 struct folio *alloc_migration_target(struct folio *src, unsigned long private);
-bool isolate_movable_page(struct page *page, isolate_mode_t mode);
 bool folio_isolate_movable(struct folio *folio, isolate_mode_t mode);
 bool isolate_folio_to_list(struct folio *folio, struct list_head *list);
 
@@ -91,8 +90,6 @@ static inline int migrate_pages(struct list_head *l, new_folio_t new,
 static inline struct folio *alloc_migration_target(struct folio *src,
 		unsigned long private)
 	{ return NULL; }
-static inline bool isolate_movable_page(struct page *page, isolate_mode_t mode)
-	{ return false; }
 static inline bool folio_isolate_movable(struct folio *folio,
 					 isolate_mode_t mode)
 	{ return false; }
diff --git a/mm/migrate.c b/mm/migrate.c
index 4cac7f749620..a29f3f8f61d0 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -127,14 +127,6 @@ bool folio_isolate_movable(struct folio *folio, isolate_mode_t mode)
 	return false;
 }
 
-bool isolate_movable_page(struct page *page, isolate_mode_t mode)
-{
-	if (PageTail(page))
-		return false;
-
-	return folio_isolate_movable((struct folio *)page, mode);
-}
-
 static void putback_movable_folio(struct folio *folio)
 {
 	const struct movable_operations *mops = folio_movable_ops(folio);
-- 
2.27.0

Re: [PATCH v2 0/5] mm: convert to folio_isolate_movable()

[Vishal Moola]

On Thu, Aug 29, 2024 at 10:54:51PM +0800, Kefeng Wang wrote:
> Convert to the new folio_isolate_movable() to get rid of
> isolate_movable_page().
> 
> Based on next-20240829.

For the whole series:

Reviewed-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>