migrate_folio_unmap() is the only user of MIGRATEPAGE_UNMAP. We want to
remove MIGRATEPAGE_* completely.
It's rather weird to have a generic MIGRATEPAGE_UNMAP, documented to be
returned from address-space callbacks, when it's only used for an
internal helper.
Let's start by having only a single "success" return value for
migrate_folio_unmap() -- 0 -- by moving the "folio was already freed"
check into the single caller.
There is a remaining comment for PG_isolated, which we renamed to
PG_movable_ops_isolated recently and forgot to update.
While we might still run into that case with zsmalloc, it's something we
want to get rid of soon. So let's just focus that optimization on real
folios only for now by excluding movable_ops pages. Note that concurrent
freeing can happen at any time and this "already freed" check is not
relevant for correctness.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/linux/migrate.h | 1 -
mm/migrate.c | 40 ++++++++++++++++++++--------------------
2 files changed, 20 insertions(+), 21 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index acadd41e0b5cf..40f2b5a37efbb 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -18,7 +18,6 @@ struct migration_target_control;
* - zero on page migration success;
*/
#define MIGRATEPAGE_SUCCESS 0
-#define MIGRATEPAGE_UNMAP 1
/**
* struct movable_operations - Driver page migration
diff --git a/mm/migrate.c b/mm/migrate.c
index 425401b2d4e14..e9dacf1028dc7 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1176,16 +1176,6 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
bool locked = false;
bool dst_locked = false;
- if (folio_ref_count(src) == 1) {
- /* Folio was freed from under us. So we are done. */
- folio_clear_active(src);
- folio_clear_unevictable(src);
- /* free_pages_prepare() will clear PG_isolated. */
- list_del(&src->lru);
- migrate_folio_done(src, reason);
- return MIGRATEPAGE_SUCCESS;
- }
-
dst = get_new_folio(src, private);
if (!dst)
return -ENOMEM;
@@ -1275,7 +1265,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
if (unlikely(page_has_movable_ops(&src->page))) {
__migrate_folio_record(dst, old_page_state, anon_vma);
- return MIGRATEPAGE_UNMAP;
+ return 0;
}
/*
@@ -1305,7 +1295,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
if (!folio_mapped(src)) {
__migrate_folio_record(dst, old_page_state, anon_vma);
- return MIGRATEPAGE_UNMAP;
+ return 0;
}
out:
@@ -1848,14 +1838,28 @@ static int migrate_pages_batch(struct list_head *from,
continue;
}
+ /*
+ * If we are holding the last folio reference, the folio
+ * was freed from under us, so just drop our reference.
+ */
+ if (likely(!page_has_movable_ops(&folio->page)) &&
+ folio_ref_count(folio) == 1) {
+ folio_clear_active(folio);
+ folio_clear_unevictable(folio);
+ list_del(&folio->lru);
+ migrate_folio_done(folio, reason);
+ stats->nr_succeeded += nr_pages;
+ stats->nr_thp_succeeded += is_thp;
+ continue;
+ }
+
rc = migrate_folio_unmap(get_new_folio, put_new_folio,
private, folio, &dst, mode, reason,
ret_folios);
/*
* The rules are:
- * Success: folio will be freed
- * Unmap: folio will be put on unmap_folios list,
- * dst folio put on dst_folios list
+ * 0: folio will be put on unmap_folios list,
+ * dst folio put on dst_folios list
* -EAGAIN: stay on the from list
* -ENOMEM: stay on the from list
* Other errno: put on ret_folios list
@@ -1905,11 +1909,7 @@ static int migrate_pages_batch(struct list_head *from,
thp_retry += is_thp;
nr_retry_pages += nr_pages;
break;
- case MIGRATEPAGE_SUCCESS:
- stats->nr_succeeded += nr_pages;
- stats->nr_thp_succeeded += is_thp;
- break;
- case MIGRATEPAGE_UNMAP:
+ case 0:
list_move_tail(&folio->lru, &unmap_folios);
list_add_tail(&dst->lru, &dst_folios);
break;
--
2.50.1On Mon, Aug 11, 2025 at 10:47 PM David Hildenbrand <david@redhat.com> wrote:
>
[...]
> +++ b/mm/migrate.c
> @@ -1176,16 +1176,6 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
> bool locked = false;
> bool dst_locked = false;
>
> - if (folio_ref_count(src) == 1) {
> - /* Folio was freed from under us. So we are done. */
> - folio_clear_active(src);
> - folio_clear_unevictable(src);
> - /* free_pages_prepare() will clear PG_isolated. */
> - list_del(&src->lru);
> - migrate_folio_done(src, reason);
> - return MIGRATEPAGE_SUCCESS;
> - }
> -
> dst = get_new_folio(src, private);
> if (!dst)
> return -ENOMEM;
> @@ -1275,7 +1265,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>
> if (unlikely(page_has_movable_ops(&src->page))) {
> __migrate_folio_record(dst, old_page_state, anon_vma);
> - return MIGRATEPAGE_UNMAP;
> + return 0;
> }
>
> /*
> @@ -1305,7 +1295,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>
> if (!folio_mapped(src)) {
> __migrate_folio_record(dst, old_page_state, anon_vma);
> - return MIGRATEPAGE_UNMAP;
> + return 0;
> }
>
> out:
> @@ -1848,14 +1838,28 @@ static int migrate_pages_batch(struct list_head *from,
> continue;
> }
>
> + /*
> + * If we are holding the last folio reference, the folio
> + * was freed from under us, so just drop our reference.
> + */
> + if (likely(!page_has_movable_ops(&folio->page)) &&
> + folio_ref_count(folio) == 1) {
> + folio_clear_active(folio);
> + folio_clear_unevictable(folio);
> + list_del(&folio->lru);
> + migrate_folio_done(folio, reason);
> + stats->nr_succeeded += nr_pages;
> + stats->nr_thp_succeeded += is_thp;
> + continue;
> + }
> +
It seems the reason parameter is no longer used within migrate_folio_unmap()
after this patch.
Perhaps it could be removed from the function's signature ;)
> rc = migrate_folio_unmap(get_new_folio, put_new_folio,
> private, folio, &dst, mode, reason,
> ret_folios);
Anyway, just a small thought. Feel free to add:
Reviewed-by: Lance Yang <lance.yang@linux.dev>
Thanks,
Lance
On 13.08.25 07:05, Lance Yang wrote:
> On Mon, Aug 11, 2025 at 10:47 PM David Hildenbrand <david@redhat.com> wrote:
>>
> [...]
>> +++ b/mm/migrate.c
>> @@ -1176,16 +1176,6 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>> bool locked = false;
>> bool dst_locked = false;
>>
>> - if (folio_ref_count(src) == 1) {
>> - /* Folio was freed from under us. So we are done. */
>> - folio_clear_active(src);
>> - folio_clear_unevictable(src);
>> - /* free_pages_prepare() will clear PG_isolated. */
>> - list_del(&src->lru);
>> - migrate_folio_done(src, reason);
>> - return MIGRATEPAGE_SUCCESS;
>> - }
>> -
>> dst = get_new_folio(src, private);
>> if (!dst)
>> return -ENOMEM;
>> @@ -1275,7 +1265,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>
>> if (unlikely(page_has_movable_ops(&src->page))) {
>> __migrate_folio_record(dst, old_page_state, anon_vma);
>> - return MIGRATEPAGE_UNMAP;
>> + return 0;
>> }
>>
>> /*
>> @@ -1305,7 +1295,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>
>> if (!folio_mapped(src)) {
>> __migrate_folio_record(dst, old_page_state, anon_vma);
>> - return MIGRATEPAGE_UNMAP;
>> + return 0;
>> }
>>
>> out:
>> @@ -1848,14 +1838,28 @@ static int migrate_pages_batch(struct list_head *from,
>> continue;
>> }
>>
>> + /*
>> + * If we are holding the last folio reference, the folio
>> + * was freed from under us, so just drop our reference.
>> + */
>> + if (likely(!page_has_movable_ops(&folio->page)) &&
>> + folio_ref_count(folio) == 1) {
>> + folio_clear_active(folio);
>> + folio_clear_unevictable(folio);
>> + list_del(&folio->lru);
>> + migrate_folio_done(folio, reason);
>> + stats->nr_succeeded += nr_pages;
>> + stats->nr_thp_succeeded += is_thp;
>> + continue;
>> + }
>> +
>
> It seems the reason parameter is no longer used within migrate_folio_unmap()
> after this patch.
>
> Perhaps it could be removed from the function's signature ;)
Thanks, well spotted, @Andrew can you squash the following?
From 40938bb0de20e03250c813d5abc7286aea69d835 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Mon, 18 Aug 2025 13:26:05 +0200
Subject: [PATCH] fixup: mm/migrate: remove MIGRATEPAGE_UNMAP
No need to pass "reason" to migrate_folio_unmap().
Signed-off-by: David Hildenbrand <david@redhat.com>
---
mm/migrate.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 2db4974178e6a..aabc736eec022 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1166,7 +1166,7 @@ static void migrate_folio_done(struct folio *src,
static int migrate_folio_unmap(new_folio_t get_new_folio,
free_folio_t put_new_folio, unsigned long private,
struct folio *src, struct folio **dstp, enum migrate_mode mode,
- enum migrate_reason reason, struct list_head *ret)
+ struct list_head *ret)
{
struct folio *dst;
int rc = -EAGAIN;
@@ -1852,8 +1852,7 @@ static int migrate_pages_batch(struct list_head *from,
}
rc = migrate_folio_unmap(get_new_folio, put_new_folio,
- private, folio, &dst, mode, reason,
- ret_folios);
+ private, folio, &dst, mode, ret_folios);
/*
* The rules are:
* 0: folio will be put on unmap_folios list,
--
2.50.1
--
Cheers
David / dhildenb
On 11 Aug 2025, at 10:39, David Hildenbrand wrote: > migrate_folio_unmap() is the only user of MIGRATEPAGE_UNMAP. We want to > remove MIGRATEPAGE_* completely. > > It's rather weird to have a generic MIGRATEPAGE_UNMAP, documented to be > returned from address-space callbacks, when it's only used for an > internal helper. > > Let's start by having only a single "success" return value for > migrate_folio_unmap() -- 0 -- by moving the "folio was already freed" > check into the single caller. > > There is a remaining comment for PG_isolated, which we renamed to > PG_movable_ops_isolated recently and forgot to update. > > While we might still run into that case with zsmalloc, it's something we > want to get rid of soon. So let's just focus that optimization on real > folios only for now by excluding movable_ops pages. Note that concurrent > freeing can happen at any time and this "already freed" check is not > relevant for correctness. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > include/linux/migrate.h | 1 - > mm/migrate.c | 40 ++++++++++++++++++++-------------------- > 2 files changed, 20 insertions(+), 21 deletions(-) > LGTM. Reviewed-by: Zi Yan <ziy@nvidia.com> Best Regards, Yan, Zi
© 2016 - 2025 Red Hat, Inc.