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

[Vishal Moola <vishal.moola@gmail.com>]

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
is. You could then also consider making isolate_movable_folio() more similar
to folio_isolate_lru() if you really wanted to.

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