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

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

[Kefeng Wang]

Convert to the new folio_isolate_movable() helper.

Based on next-20240823.

Kefeng Wang (4):
  mm: migrate: add folio_isolate_movable()
  mm: compaction: convert to folio_isolate_movable()
  mm: migrate: convert to folio_isolate_movable()
  mm: migrate: remove isolate_movable_page()

 include/linux/migrate.h |  5 +++--
 mm/compaction.c         | 32 +++++++++++++++++---------------
 mm/migrate.c            | 37 ++++++++++++++++++-------------------
 3 files changed, 38 insertions(+), 36 deletions(-)

-- 
2.27.0

[PATCH 1/4] 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().

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/linux/migrate.h |  4 ++++
 mm/migrate.c            | 41 ++++++++++++++++++++++++-----------------
 2 files changed, 28 insertions(+), 17 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 4f55f4930fe8..cc1c268c3822 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -58,21 +58,20 @@
 
 #include "internal.h"
 
-bool isolate_movable_page(struct page *page, isolate_mode_t mode)
+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(),
+	 * Avoid burning cycles with folios 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
+	 * In case we 'win' a race for a movable folio 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.
+	 * the folio_put() at the end of this block will take care of
+	 * release this folio, thus avoiding a nasty leakage.
 	 */
-	if (!folio)
+	if (!folio_try_get(folio))
 		goto out;
 
 	if (unlikely(folio_test_slab(folio)))
@@ -80,9 +79,9 @@ bool isolate_movable_page(struct page *page, isolate_mode_t mode)
 	/* 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;
@@ -129,6 +128,14 @@ bool isolate_movable_page(struct page *page, 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

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

[Kefeng Wang]

The tail page will always fail to isolate for non-lru-movable and
LRU page in isolate_migratepages_block(), so move PageTail check
ahead, then convert to use folio_isolate_movable() helper and more
folios.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/compaction.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index a2b16b08cbbf..aa2e8bb9fa58 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 			}
 		}
 
+		if (PageTail(page))
+			goto isolate_fail;
+
+		folio = page_folio(page);
+
 		/*
 		 * 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 (folio_isolate_movable(folio, mode))
 					goto isolate_success;
-				}
 			}
 
 			goto isolate_fail;
 		}
 
 		/*
-		 * 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.
+		 * 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))
+		if (unlikely(folio_try_get(folio)))
 			goto isolate_fail;
 
 		/*
-- 
2.27.0

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

[Kefeng Wang]

Directly use folio_isolate_movable() helper in isolate_folio_to_list().

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

diff --git a/mm/migrate.c b/mm/migrate.c
index cc1c268c3822..3d8a97b74356 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -198,8 +198,8 @@ 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) {
 			list_add(&folio->lru, list);
-- 
2.27.0

[PATCH 4/4] 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 3d8a97b74356..5bed761b1be9 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -128,14 +128,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 1/4] mm: migrate: add folio_isolate_movable()

[Andrew Morton]

On Mon, 26 Aug 2024 12:01:29 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> 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().
> 
> ...
>
> +bool isolate_movable_page(struct page *page, isolate_mode_t mode)
> +{
> +	if (PageTail(page))
> +		return false;
> +
> +	return folio_isolate_movable((struct folio *)page, mode);
> +}

Use page_folio() here?

Re: [PATCH 1/4] mm: migrate: add folio_isolate_movable()

[Kefeng Wang]

On 2024/8/28 9:14, Andrew Morton wrote:
> On Mon, 26 Aug 2024 12:01:29 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
>> 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().
>>
>> ...
>>
>> +bool isolate_movable_page(struct page *page, isolate_mode_t mode)
>> +{
>> +	if (PageTail(page))
>> +		return false;
>> +
>> +	return folio_isolate_movable((struct folio *)page, mode);
>> +}
> 
> Use page_folio() here?

Page must head after above check, and this function will be killed in 
patch4 , so no need to use page_folio().

Re: [PATCH 2/4] mm: compaction: convert to folio_isolate_movable()

[Baolin Wang]

Hi Kefeng,

On 2024/8/26 12:01, Kefeng Wang wrote:
> The tail page will always fail to isolate for non-lru-movable and
> LRU page in isolate_migratepages_block(), so move PageTail check
> ahead, then convert to use folio_isolate_movable() helper and more
> folios.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   mm/compaction.c | 32 +++++++++++++++++---------------
>   1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index a2b16b08cbbf..aa2e8bb9fa58 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>   			}
>   		}
>   
> +		if (PageTail(page))
> +			goto isolate_fail;
> +
> +		folio = page_folio(page);

I wonder if this is stable. Before page_folio(), it does not hold a 
reference on the page, so seems we should re-check the folio still 
contains this page after gaining a reference on the folio?

>   		/*
>   		 * 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 (folio_isolate_movable(folio, mode))
>   					goto isolate_success;
> -				}
>   			}
>   
>   			goto isolate_fail;
>   		}
>   
>   		/*
> -		 * 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.
> +		 * 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))
> +		if (unlikely(folio_try_get(folio)))
>   			goto isolate_fail;
>   
>   		/*

Re: [PATCH 2/4] mm: compaction: convert to folio_isolate_movable()

[Kefeng Wang]

On 2024/8/28 18:04, Baolin Wang wrote:
> Hi Kefeng,
> 
> On 2024/8/26 12:01, Kefeng Wang wrote:
>> The tail page will always fail to isolate for non-lru-movable and
>> LRU page in isolate_migratepages_block(), so move PageTail check
>> ahead, then convert to use folio_isolate_movable() helper and more
>> folios.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/compaction.c | 32 +++++++++++++++++---------------
>>   1 file changed, 17 insertions(+), 15 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index a2b16b08cbbf..aa2e8bb9fa58 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct 
>> compact_control *cc, unsigned long low_pfn,
>>               }
>>           }
>> +        if (PageTail(page))
>> +            goto isolate_fail;
>> +
>> +        folio = page_folio(page);
> 
> I wonder if this is stable. Before page_folio(), it does not hold a 
> reference on the page, so seems we should re-check the folio still 
> contains this page after gaining a reference on the folio?

Oh, you are right, so two way to avoid this,

1) re-check 'page_folio(page)  == folio', but this need change a little 
more.

2)  directly use folio_get_nontail_page(page) here, and folio_put in the 
following path, this will try to get for any pages, but it should be 
accepted ?

I'd prefer 2) but any other suggestion?

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
>> +         * 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 (folio_isolate_movable(folio, mode))
>>                       goto isolate_success;
>> -                }
>>               }
>>               goto isolate_fail;
>>           }
>>           /*
>> -         * 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.
>> +         * 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))
>> +        if (unlikely(folio_try_get(folio)))
>>               goto isolate_fail;
>>           /*

Re: [PATCH 2/4] mm: compaction: convert to folio_isolate_movable()

[Andrew Morton]

On Wed, 28 Aug 2024 20:36:03 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> > I wonder if this is stable. Before page_folio(), it does not hold a 
> > reference on the page, so seems we should re-check the folio still 
> > contains this page after gaining a reference on the folio?
> 
> Oh, you are right, so two way to avoid this,
> 
> 1) re-check 'page_folio(page)  == folio', but this need change a little 
> more.
> 
> 2)  directly use folio_get_nontail_page(page) here, and folio_put in the 
> following path, this will try to get for any pages, but it should be 
> accepted ?
> 
> I'd prefer 2) but any other suggestion?

I dropped the v1 series.

Re: [PATCH 2/4] mm: compaction: convert to folio_isolate_movable()

[Vishal Moola]

On Wed, Aug 28, 2024 at 08:36:03PM +0800, Kefeng Wang wrote:
> 
> 
> On 2024/8/28 18:04, Baolin Wang wrote:
> > Hi Kefeng,
> > 
> > On 2024/8/26 12:01, Kefeng Wang wrote:
> > > The tail page will always fail to isolate for non-lru-movable and
> > > LRU page in isolate_migratepages_block(), so move PageTail check
> > > ahead, then convert to use folio_isolate_movable() helper and more
> > > folios.
> > > 
> > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> > > ---
> > >   mm/compaction.c | 32 +++++++++++++++++---------------
> > >   1 file changed, 17 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/mm/compaction.c b/mm/compaction.c
> > > index a2b16b08cbbf..aa2e8bb9fa58 100644
> > > --- a/mm/compaction.c
> > > +++ b/mm/compaction.c
> > > @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct
> > > compact_control *cc, unsigned long low_pfn,
> > >               }
> > >           }
> > > +        if (PageTail(page))
> > > +            goto isolate_fail;
> > > +
> > > +        folio = page_folio(page);
> > 
> > I wonder if this is stable. Before page_folio(), it does not hold a
> > reference on the page, so seems we should re-check the folio still
> > contains this page after gaining a reference on the folio?
> 
> Oh, you are right, so two way to avoid this,
> 
> 1) re-check 'page_folio(page)  == folio', but this need change a little
> more.
> 
> 2)  directly use folio_get_nontail_page(page) here, and folio_put in the
> following path, this will try to get for any pages, but it should be
> accepted ?
> 
> I'd prefer 2) but any other suggestion?

I think option 2 makes sense, and simply use folio_put() on success and
goto isolate_fail_put on failure instead of isolate_fail.

With option 2, it might make sense to have folio_isolate_movable()
expect to be passed a folio with elevated refcount. Then it could be
treated similarly to its sister function folio_isolate_lru() and simply
use folio_get() for its reference.

Re: [PATCH 2/4] mm: compaction: convert to folio_isolate_movable()

[Baolin Wang]

On 2024/8/29 07:37, Vishal Moola wrote:
> On Wed, Aug 28, 2024 at 08:36:03PM +0800, Kefeng Wang wrote:
>>
>>
>> On 2024/8/28 18:04, Baolin Wang wrote:
>>> Hi Kefeng,
>>>
>>> On 2024/8/26 12:01, Kefeng Wang wrote:
>>>> The tail page will always fail to isolate for non-lru-movable and
>>>> LRU page in isolate_migratepages_block(), so move PageTail check
>>>> ahead, then convert to use folio_isolate_movable() helper and more
>>>> folios.
>>>>
>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>> ---
>>>>    mm/compaction.c | 32 +++++++++++++++++---------------
>>>>    1 file changed, 17 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>> index a2b16b08cbbf..aa2e8bb9fa58 100644
>>>> --- a/mm/compaction.c
>>>> +++ b/mm/compaction.c
>>>> @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct
>>>> compact_control *cc, unsigned long low_pfn,
>>>>                }
>>>>            }
>>>> +        if (PageTail(page))
>>>> +            goto isolate_fail;
>>>> +
>>>> +        folio = page_folio(page);
>>>
>>> I wonder if this is stable. Before page_folio(), it does not hold a
>>> reference on the page, so seems we should re-check the folio still
>>> contains this page after gaining a reference on the folio?
>>
>> Oh, you are right, so two way to avoid this,
>>
>> 1) re-check 'page_folio(page)  == folio', but this need change a little
>> more.
>>
>> 2)  directly use folio_get_nontail_page(page) here, and folio_put in the
>> following path, this will try to get for any pages, but it should be
>> accepted ?
>>
>> I'd prefer 2) but any other suggestion?
> 
> I think option 2 makes sense, and simply use folio_put() on success and
> goto isolate_fail_put on failure instead of isolate_fail.
> 
> With option 2, it might make sense to have folio_isolate_movable()
> expect to be passed a folio with elevated refcount. Then it could be
> treated similarly to its sister function folio_isolate_lru() and simply
> use folio_get() for its reference.

Yes, that makes sense to me too.

Re: [PATCH 2/4] mm: compaction: convert to folio_isolate_movable()

[Kefeng Wang]

On 2024/8/29 9:11, Baolin Wang wrote:
> 
> 
> On 2024/8/29 07:37, Vishal Moola wrote:
>> On Wed, Aug 28, 2024 at 08:36:03PM +0800, Kefeng Wang wrote:
>>>
>>>
>>> On 2024/8/28 18:04, Baolin Wang wrote:
>>>> Hi Kefeng,
>>>>
>>>> On 2024/8/26 12:01, Kefeng Wang wrote:
>>>>> The tail page will always fail to isolate for non-lru-movable and
>>>>> LRU page in isolate_migratepages_block(), so move PageTail check
>>>>> ahead, then convert to use folio_isolate_movable() helper and more
>>>>> folios.
>>>>>
>>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>>> ---
>>>>>    mm/compaction.c | 32 +++++++++++++++++---------------
>>>>>    1 file changed, 17 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>>> index a2b16b08cbbf..aa2e8bb9fa58 100644
>>>>> --- a/mm/compaction.c
>>>>> +++ b/mm/compaction.c
>>>>> @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct
>>>>> compact_control *cc, unsigned long low_pfn,
>>>>>                }
>>>>>            }
>>>>> +        if (PageTail(page))
>>>>> +            goto isolate_fail;
>>>>> +
>>>>> +        folio = page_folio(page);
>>>>
>>>> I wonder if this is stable. Before page_folio(), it does not hold a
>>>> reference on the page, so seems we should re-check the folio still
>>>> contains this page after gaining a reference on the folio?
>>>
>>> Oh, you are right, so two way to avoid this,
>>>
>>> 1) re-check 'page_folio(page)  == folio', but this need change a little
>>> more.
>>>
>>> 2)  directly use folio_get_nontail_page(page) here, and folio_put in the
>>> following path, this will try to get for any pages, but it should be
>>> accepted ?
>>>
>>> I'd prefer 2) but any other suggestion?
>>
>> I think option 2 makes sense, and simply use folio_put() on success and
>> goto isolate_fail_put on failure instead of isolate_fail.
>>
>> With option 2, it might make sense to have folio_isolate_movable()
>> expect to be passed a folio with elevated refcount. Then it could be
>> treated similarly to its sister function folio_isolate_lru() and simply
>> use folio_get() for its reference.
> 
> Yes, that makes sense to me too.

Thanks Vishal/Baolin, will refresh and send v2.