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

Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> · Tue, 20 Aug 2024 17:46:41 +0800

On 2024/8/20 17:32, Miaohe Lin wrote:
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@xxxxxxxxxx>
Signed-off-by: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>

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 for your review, missing this, only test memory-hotplug, could 
you try this,

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index d8298017bd99..64a145a0e29f 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2671,6 +2671,7 @@ static int soft_offline_in_use_page(struct page *page)
        struct folio *folio = page_folio(page);
        char const *msg_page[] = {"page", "hugepage"};
        bool huge = folio_test_hugetlb(folio);
+       bool isolated;
        LIST_HEAD(pagelist);
        struct migration_target_control mtc = {
                .nid = NUMA_NO_NODE,
@@ -2710,7 +2711,18 @@ static int soft_offline_in_use_page(struct page 
*page)
                return 0;
        }

-       if (isolate_folio_to_list(folio, &pagelist)) {
+       isolated = isolate_folio_to_list(folio, &pagelist);
+
+       /*
+        * 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);
+
+       if (isolated) {
                ret = migrate_pages(&pagelist, alloc_migration_target, 
NULL,
                        (unsigned long)&mtc, MIGRATE_SYNC, 
MR_MEMORY_FAILURE, NULL);
                if (!ret) {
@@ -2733,15 +2745,6 @@ static int soft_offline_in_use_page(struct page 
*page)
                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);
-
        return ret;
 }



Thanks.
.







