Re: [PATCH v2 4/5] mm: migrate: add isolate_folio_to_list()

[Miaohe Lin <linmiaohe@huawei.com>]

On 2024/8/17 16:49, Kefeng Wang wrote:
> Add isolate_folio_to_list() helper to try to isolate HugeTLB,
> no-LRU movable and LRU folios to a list, which will be reused by
> do_migrate_range() from memory hotplug soon, also drop the
> mf_isolate_folio() since we could directly use new helper in
> the soft_offline_in_use_page().
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>

Thanks for your patch.

> ---
>  include/linux/migrate.h |  3 +++
>  mm/memory-failure.c     | 46 ++++++++++-------------------------------
>  mm/migrate.c            | 27 ++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index 644be30b69c8..002e49b2ebd9 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 isolate_folio_to_list(struct folio *folio, struct list_head *list);
>  
>  int migrate_huge_page_move_mapping(struct address_space *mapping,
>  		struct folio *dst, struct folio *src);
> @@ -91,6 +92,8 @@ 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 isolate_folio_to_list(struct folio *folio, struct list_head *list)
> +	{ return false; }
>  
>  static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
>  				  struct folio *dst, struct folio *src)
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 93848330de1f..d8298017bd99 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2659,40 +2659,6 @@ EXPORT_SYMBOL(unpoison_memory);
>  #undef pr_fmt
>  #define pr_fmt(fmt) "Soft offline: " fmt
>  
> -static bool mf_isolate_folio(struct folio *folio, struct list_head *pagelist)
> -{
> -	bool isolated = false;
> -
> -	if (folio_test_hugetlb(folio)) {
> -		isolated = isolate_hugetlb(folio, pagelist);
> -	} else {
> -		bool lru = !__folio_test_movable(folio);
> -
> -		if (lru)
> -			isolated = folio_isolate_lru(folio);
> -		else
> -			isolated = isolate_movable_page(&folio->page,
> -							ISOLATE_UNEVICTABLE);
> -
> -		if (isolated) {
> -			list_add(&folio->lru, pagelist);
> -			if (lru)
> -				node_stat_add_folio(folio, NR_ISOLATED_ANON +
> -						    folio_is_file_lru(folio));
> -		}
> -	}
> -
> -	/*
> -	 * If we succeed to isolate the folio, we grabbed another refcount on
> -	 * the folio, so we can safely drop the one we got from get_any_page().
> -	 * If we failed to isolate the folio, it means that we cannot go further
> -	 * and we will return an error, so drop the reference we got from
> -	 * get_any_page() as well.
> -	 */
> -	folio_put(folio);
> -	return isolated;
> -}
> -
>  /*
>   * soft_offline_in_use_page handles hugetlb-pages and non-hugetlb pages.
>   * If the page is a non-dirty unmapped page-cache page, it simply invalidates.
> @@ -2744,7 +2710,7 @@ static int soft_offline_in_use_page(struct page *page)
>  		return 0;
>  	}
>  
> -	if (mf_isolate_folio(folio, &pagelist)) {
> +	if (isolate_folio_to_list(folio, &pagelist)) {
>  		ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
>  			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, NULL);
>  		if (!ret) {
> @@ -2766,6 +2732,16 @@ static int soft_offline_in_use_page(struct page *page)
>  			pfn, msg_page[huge], page_count(page), &page->flags);
>  		ret = -EBUSY;
>  	}
> +
> +	/*
> +	 * If we succeed to isolate the folio, we grabbed another refcount on
> +	 * the folio, so we can safely drop the one we got from get_any_page().
> +	 * If we failed to isolate the folio, it means that we cannot go further
> +	 * and we will return an error, so drop the reference we got from
> +	 * get_any_page() as well.
> +	 */
> +	folio_put(folio);

Why folio_put() is deferred here? With this change, folio will have extra two refcnt when
calling migrate_pages() above. One is from get_any_page() and another one from folio_isolate_lru().
This would lead to migrate_pages() never success. And my many testcases failed due to this
change.

Thanks.
.