by Zi Yan

[permalink] [raw]

Subject: Re: [PATCH -v4 5/9] migrate_pages: batch _unmap and _move

On 6 Feb 2023, at 1:33, Huang Ying wrote:

> In this patch the _unmap and _move stage of the folio migration is
> batched. That for, previously, it is,
>
> for each folio
> _unmap()
> _move()
>
> Now, it is,
>
> for each folio
> _unmap()
> for each folio
> _move()
>
> Based on this, we can batch the TLB flushing and use some hardware
> accelerator to copy folios between batched _unmap and batched _move
> stages.
>
> Signed-off-by: "Huang, Ying" <[email protected]>
> Tested-by: Hyeonggon Yoo <[email protected]>
> Cc: Zi Yan <[email protected]>
> Cc: Yang Shi <[email protected]>
> Cc: Baolin Wang <[email protected]>
> Cc: Oscar Salvador <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Bharata B Rao <[email protected]>
> Cc: Alistair Popple <[email protected]>
> Cc: haoxin <[email protected]>
> Cc: Minchan Kim <[email protected]>
> Cc: Mike Kravetz <[email protected]>
> ---
> mm/migrate.c | 208 +++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 184 insertions(+), 24 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 0428449149f4..fa7212330cb6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1033,6 +1033,33 @@ static void __migrate_folio_extract(struct folio *dst,
> dst->private = NULL;
> }
>
> +/* Restore the source folio to the original state upon failure */
> +static void migrate_folio_undo_src(struct folio *src,
> + int page_was_mapped,
> + struct anon_vma *anon_vma,
> + struct list_head *ret)
> +{
> + if (page_was_mapped)
> + remove_migration_ptes(src, src, false);
> + /* Drop an anon_vma reference if we took one */
> + if (anon_vma)
> + put_anon_vma(anon_vma);
> + folio_unlock(src);
> + list_move_tail(&src->lru, ret);
> +}
> +
> +/* Restore the destination folio to the original state upon failure */
> +static void migrate_folio_undo_dst(struct folio *dst,
> + free_page_t put_new_page,
> + unsigned long private)
> +{
> + folio_unlock(dst);
> + if (put_new_page)
> + put_new_page(&dst->page, private);
> + else
> + folio_put(dst);
> +}
> +
> /* Cleanup src folio upon migration success */
> static void migrate_folio_done(struct folio *src,
> enum migrate_reason reason)
> @@ -1052,7 +1079,7 @@ static void migrate_folio_done(struct folio *src,
> }
>
> static int __migrate_folio_unmap(struct folio *src, struct folio *dst,
> - int force, enum migrate_mode mode)
> + int force, bool force_lock, enum migrate_mode mode)
> {
> int rc = -EAGAIN;
> int page_was_mapped = 0;
> @@ -1079,6 +1106,17 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst,
> if (current->flags & PF_MEMALLOC)
> goto out;
>
> + /*
> + * We have locked some folios, to avoid deadlock, we cannot
> + * lock the folio synchronously. Go out to process (and
> + * unlock) all the locked folios. Then we can lock the folio
> + * synchronously.
> + */
The comment alone is quite confusing and the variable might be better
renamed to avoid_force_lock, since there is a force variable to force
lock folio already. And the variable intends to discourage force lock
on a folio to avoid potential deadlock.

How about? Since "lock synchronously" might not be as straightforward
as wait to lock.

/*
* We have locked some folios and are going to wait to lock this folio.
* To avoid a potential deadlock, let's bail out and not do that. The
* locked folios will be moved and unlocked, then we can wait to lock
* this folio
*/

> + if (!force_lock) {
> + rc = -EDEADLOCK;
> + goto out;
> + }
> +
> folio_lock(src);
> }
>
> @@ -1187,10 +1225,20 @@ static int __migrate_folio_move(struct folio *src, struct folio *dst,
> int page_was_mapped = 0;
> struct anon_vma *anon_vma = NULL;
> bool is_lru = !__PageMovable(&src->page);
> + struct list_head *prev;
>
> __migrate_folio_extract(dst, &page_was_mapped, &anon_vma);
> + prev = dst->lru.prev;
> + list_del(&dst->lru);
>
> rc = move_to_new_folio(dst, src, mode);
> +
> + if (rc == -EAGAIN) {
> + list_add(&dst->lru, prev);
> + __migrate_folio_record(dst, page_was_mapped, anon_vma);
> + return rc;
> + }
> +
> if (unlikely(!is_lru))
> goto out_unlock_both;
>
> @@ -1233,7 +1281,7 @@ static int __migrate_folio_move(struct folio *src, struct folio *dst,
> /* Obtain the lock on page, remove all ptes. */
> static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page,
> unsigned long private, struct folio *src,
> - struct folio **dstp, int force,
> + struct folio **dstp, int force, bool force_lock,
> enum migrate_mode mode, enum migrate_reason reason,
> struct list_head *ret)
> {
> @@ -1261,7 +1309,7 @@ static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page
> *dstp = dst;
>
> dst->private = NULL;
> - rc = __migrate_folio_unmap(src, dst, force, mode);
> + rc = __migrate_folio_unmap(src, dst, force, force_lock, mode);
> if (rc == MIGRATEPAGE_UNMAP)
> return rc;
>
> @@ -1270,7 +1318,7 @@ static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page
> * references and be restored.
> */
> /* restore the folio to right list. */
> - if (rc != -EAGAIN)
> + if (rc != -EAGAIN && rc != -EDEADLOCK)
> list_move_tail(&src->lru, ret);
>
> if (put_new_page)
> @@ -1309,9 +1357,8 @@ static int migrate_folio_move(free_page_t put_new_page, unsigned long private,
> */
> if (rc == MIGRATEPAGE_SUCCESS) {
> migrate_folio_done(src, reason);
> - } else {
> - if (rc != -EAGAIN)
> - list_add_tail(&src->lru, ret);
> + } else if (rc != -EAGAIN) {
> + list_add_tail(&src->lru, ret);
>
> if (put_new_page)
> put_new_page(&dst->page, private);
> @@ -1591,7 +1638,7 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
> enum migrate_mode mode, int reason, struct list_head *ret_folios,
> struct migrate_pages_stats *stats)

Like I said in my last comment to this patch, migrate_pages_batch() function
deserves a detailed comment about its working flow including the error handling.
Now you only put some in the git log, which is hard to access after several code
changes later.

How about?

/*
* migrate_pages_batch() first unmaps pages in the from as many as possible,
* then migrates the unmapped pages. During unmap process, different situations
* are handled differently:
* 1. ENOSYS, unsupported large folio migration: move to ret_folios list
* 2. ENOMEM, lower memory at the destination: migrate existing unmapped folios
* and stop, since existing unmapped folios have new pages allocated and can
* be migrated
* 3. EDEADLOCK, to be unmapped page is locked by someone else, to avoid deadlock,
* we migrate existing unmapped pages and try to lock again
* 4. MIGRATEPAGE_SUCCESS, the folios was freed under us: no action
* 5. MIGRATEPAGE_UNMAP, unmap succeeded: set avoid_force_lock to true to avoid
* wait to lock a folio in the future to avoid deadlock.
*
* For folios unmapped but cannot be migrated, we will restore their original
* states during cleanup stage at the end.
*/

> {
> - int retry = 1;
> + int retry;
> int large_retry = 1;
> int thp_retry = 1;
> int nr_failed = 0;
> @@ -1600,13 +1647,19 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
> int pass = 0;
> bool is_large = false;
> bool is_thp = false;
> - struct folio *folio, *folio2, *dst = NULL;
> - int rc, nr_pages;
> + struct folio *folio, *folio2, *dst = NULL, *dst2;
> + int rc, rc_saved, nr_pages;
> LIST_HEAD(split_folios);
> + LIST_HEAD(unmap_folios);
> + LIST_HEAD(dst_folios);
> bool nosplit = (reason == MR_NUMA_MISPLACED);
> bool no_split_folio_counting = false;
> + bool force_lock;
>
> -split_folio_migration:
> +retry:
> + rc_saved = 0;
> + force_lock = true;
> + retry = 1;
> for (pass = 0;
> pass < NR_MAX_MIGRATE_PAGES_RETRY && (retry || large_retry);
> pass++) {
> @@ -1628,16 +1681,15 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
> cond_resched();
>
> rc = migrate_folio_unmap(get_new_page, put_new_page, private,
> - folio, &dst, pass > 2, mode,
> - reason, ret_folios);
> - if (rc == MIGRATEPAGE_UNMAP)
> - rc = migrate_folio_move(put_new_page, private,
> - folio, dst, mode,
> - reason, ret_folios);
> + folio, &dst, pass > 2, force_lock,
> + 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
> * -EAGAIN: stay on the from list
> + * -EDEADLOCK: stay on the from list
> * -ENOMEM: stay on the from list
> * -ENOSYS: stay on the from list
> * Other errno: put on ret_folios list
> @@ -1672,7 +1724,7 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
> case -ENOMEM:
> /*
> * When memory is low, don't bother to try to migrate
> - * other folios, just exit.
> + * other folios, move unmapped folios, then exit.
> */
> if (is_large) {
> nr_large_failed++;
> @@ -1711,7 +1763,19 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
> /* nr_failed isn't updated for not used */
> nr_large_failed += large_retry;
> stats->nr_thp_failed += thp_retry;
> - goto out;
> + rc_saved = rc;
> + if (list_empty(&unmap_folios))
> + goto out;
> + else
> + goto move;
> + case -EDEADLOCK:
> + /*
> + * The folio cannot be locked for potential deadlock.
> + * Go move (and unlock) all locked folios. Then we can
> + * try again.
> + */
> + rc_saved = rc;
> + goto move;
> case -EAGAIN:
> if (is_large) {
> large_retry++;
> @@ -1725,6 +1789,15 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
> stats->nr_succeeded += nr_pages;
> stats->nr_thp_succeeded += is_thp;
> break;
> + case MIGRATEPAGE_UNMAP:
> + /*
> + * We have locked some folios, don't force lock
> + * to avoid deadlock.
> + */
> + force_lock = false;
> + list_move_tail(&folio->lru, &unmap_folios);
> + list_add_tail(&dst->lru, &dst_folios);
> + break;
> default:
> /*
> * Permanent failure (-EBUSY, etc.):
> @@ -1748,12 +1821,95 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
> nr_large_failed += large_retry;
> stats->nr_thp_failed += thp_retry;
> stats->nr_failed_pages += nr_retry_pages;
> +move:
> + retry = 1;
> + for (pass = 0;
> + pass < NR_MAX_MIGRATE_PAGES_RETRY && (retry || large_retry);
> + pass++) {
> + retry = 0;
> + large_retry = 0;
> + thp_retry = 0;
> + nr_retry_pages = 0;
> +
> + dst = list_first_entry(&dst_folios, struct folio, lru);
> + dst2 = list_next_entry(dst, lru);
> + list_for_each_entry_safe(folio, folio2, &unmap_folios, lru) {
> + is_large = folio_test_large(folio);
> + is_thp = is_large && folio_test_pmd_mappable(folio);
> + nr_pages = folio_nr_pages(folio);
> +
> + cond_resched();
> +
> + rc = migrate_folio_move(put_new_page, private,
> + folio, dst, mode,
> + reason, ret_folios);
> + /*
> + * The rules are:
> + * Success: folio will be freed
> + * -EAGAIN: stay on the unmap_folios list
> + * Other errno: put on ret_folios list
> + */
> + switch(rc) {
> + case -EAGAIN:
> + if (is_large) {
> + large_retry++;
> + thp_retry += is_thp;
> + } else if (!no_split_folio_counting) {
> + retry++;
> + }
> + nr_retry_pages += nr_pages;
> + break;
> + case MIGRATEPAGE_SUCCESS:
> + stats->nr_succeeded += nr_pages;
> + stats->nr_thp_succeeded += is_thp;
> + break;
> + default:
> + if (is_large) {
> + nr_large_failed++;
> + stats->nr_thp_failed += is_thp;
> + } else if (!no_split_folio_counting) {
> + nr_failed++;
> + }
> +
> + stats->nr_failed_pages += nr_pages;
> + break;
> + }
> + dst = dst2;
> + dst2 = list_next_entry(dst, lru);
> + }
> + }
> + nr_failed += retry;
> + nr_large_failed += large_retry;
> + stats->nr_thp_failed += thp_retry;
> + stats->nr_failed_pages += nr_retry_pages;
> +
> + if (rc_saved)
> + rc = rc_saved;
> + else
> + rc = nr_failed + nr_large_failed;
> +out:
> + /* Cleanup remaining folios */
> + dst = list_first_entry(&dst_folios, struct folio, lru);
> + dst2 = list_next_entry(dst, lru);
> + list_for_each_entry_safe(folio, folio2, &unmap_folios, lru) {
> + int page_was_mapped = 0;
> + struct anon_vma *anon_vma = NULL;
> +
> + __migrate_folio_extract(dst, &page_was_mapped, &anon_vma);
> + migrate_folio_undo_src(folio, page_was_mapped, anon_vma,
> + ret_folios);
> + list_del(&dst->lru);
> + migrate_folio_undo_dst(dst, put_new_page, private);
> + dst = dst2;
> + dst2 = list_next_entry(dst, lru);
> + }
> +
> /*
> * Try to migrate split folios of fail-to-migrate large folios, no
> * nr_failed counting in this round, since all split folios of a
> * large folio is counted as 1 failure in the first round.
> */
> - if (!list_empty(&split_folios)) {
> + if (rc >= 0 && !list_empty(&split_folios)) {
> /*
> * Move non-migrated folios (after NR_MAX_MIGRATE_PAGES_RETRY
> * retries) to ret_folios to avoid migrating them again.
> @@ -1761,12 +1917,16 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
> list_splice_init(from, ret_folios);
> list_splice_init(&split_folios, from);
> no_split_folio_counting = true;
> - retry = 1;
> - goto split_folio_migration;
> + goto retry;
> }
>
> - rc = nr_failed + nr_large_failed;
> -out:
> + /*
> + * We have unlocked all locked folios, so we can force lock now, let's
> + * try again.
> + */
> + if (rc == -EDEADLOCK)
> + goto retry;
> +
> return rc;
> }
>
> --
> 2.35.1

After rename the variable (or give it a better name) and add the comments,
you can add Reviewed-by: Zi Yan <[email protected]>

Thanks.

--
Best Regards,
Yan, Zi

Attachments:

signature.asc (854.00 B)
OpenPGP digital signature

2023-02-07 06:00:01

by Huang, Ying

On 8 Feb 2023, at 7:02, Huang, Ying wrote:

> Zi Yan <[email protected]> writes:
>
>> On 6 Feb 2023, at 1:33, Huang Ying wrote:
>>
>>> This is a code cleanup patch to reduce the duplicated code between the
>>> _unmap and _move stages of migrate_pages(). No functionality change
>>> is expected.
>>>
>>> Signed-off-by: "Huang, Ying" <[email protected]>
>>> Cc: Zi Yan <[email protected]>
>>> Cc: Yang Shi <[email protected]>
>>> Cc: Baolin Wang <[email protected]>
>>> Cc: Oscar Salvador <[email protected]>
>>> Cc: Matthew Wilcox <[email protected]>
>>> Cc: Bharata B Rao <[email protected]>
>>> Cc: Alistair Popple <[email protected]>
>>> Cc: haoxin <[email protected]>
>>> Cc: Minchan Kim <[email protected]>
>>> Cc: Mike Kravetz <[email protected]>
>>> Cc: Hyeonggon Yoo <[email protected]>
>>> ---
>>> mm/migrate.c | 203 ++++++++++++++++++++-------------------------------
>>> 1 file changed, 81 insertions(+), 122 deletions(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index 23eb01cfae4c..9378fa2ad4a5 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1037,6 +1037,7 @@ static void __migrate_folio_extract(struct folio *dst,
>>> static void migrate_folio_undo_src(struct folio *src,
>>> int page_was_mapped,
>>> struct anon_vma *anon_vma,
>>> + bool locked,
>>> struct list_head *ret)
>>> {
>>> if (page_was_mapped)
>>> @@ -1044,16 +1045,20 @@ static void migrate_folio_undo_src(struct folio *src,
>>> /* Drop an anon_vma reference if we took one */
>>> if (anon_vma)
>>> put_anon_vma(anon_vma);
>>> - folio_unlock(src);
>>> - list_move_tail(&src->lru, ret);
>>> + if (locked)
>>> + folio_unlock(src);
>>
>> Having a comment would be better.
>> /* A page that has not been migrated, move it to a list for later restoration */
>
> Emm... the page state has been restored in the previous operations of
> the function. This is the last step and the page will be moved to
> "return" list, then the caller of migrate_pages() will call
> putback_movable_pages().

But if (rc == -EAGAIN || rc == -EDEADLOCK) then ret will be NULL, thus the page
will not be put back, right? And for both cases, the src page state is not
changed at all. So probably only call migrate_folio_undo_src() when
(rc != -EAGAIN && rc != -EDEADLOCK)? And still require ret to be non NULL.

>
> We have some comments for the function (migrate_folio_undo_src()) as
> follows,
>
> /* Restore the source folio to the original state upon failure */
>
>>> + if (ret)
>>> + list_move_tail(&src->lru, ret);
>>> }
>>>
>>> /* Restore the destination folio to the original state upon failure */
>>> static void migrate_folio_undo_dst(struct folio *dst,
>>> + bool locked,
>>> free_page_t put_new_page,
>>> unsigned long private)
>>> {
>>> - folio_unlock(dst);
>>> + if (locked)
>>> + folio_unlock(dst);
>>> if (put_new_page)
>>> put_new_page(&dst->page, private);
>>> else
>>> @@ -1078,13 +1083,42 @@ static void migrate_folio_done(struct folio *src,
>>> folio_put(src);
>>> }
>>>
>>> -static int __migrate_folio_unmap(struct folio *src, struct folio *dst,
>>> - int force, bool force_lock, enum migrate_mode mode)
>>> +/* Obtain the lock on page, remove all ptes. */
>>> +static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page,
>>> + unsigned long private, struct folio *src,
>>> + struct folio **dstp, int force, bool force_lock,
>>> + enum migrate_mode mode, enum migrate_reason reason,
>>> + struct list_head *ret)
>>> {
>>> + struct folio *dst;
>>> int rc = -EAGAIN;
>>> + struct page *newpage = NULL;
>>> int page_was_mapped = 0;
>>> struct anon_vma *anon_vma = NULL;
>>> bool is_lru = !__PageMovable(&src->page);
>>> + bool locked = false;
>>> + bool dst_locked = false;
>>> +
>>> + if (!thp_migration_supported() && folio_test_transhuge(src))
>>> + return -ENOSYS;
>>> +
>>> + 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;
>>> + }
>>> +
>>> + newpage = get_new_page(&src->page, private);
>>> + if (!newpage)
>>> + return -ENOMEM;
>>> + dst = page_folio(newpage);
>>> + *dstp = dst;
>>> +
>>> + dst->private = NULL;
>>>
>>> if (!folio_trylock(src)) {
>>> if (!force || mode == MIGRATE_ASYNC)
>>> @@ -1119,6 +1153,7 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst,
>>>
>>> folio_lock(src);
>>> }
>>> + locked = true;
>>>
>>> if (folio_test_writeback(src)) {
>>> /*
>>> @@ -1133,10 +1168,10 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst,
>>> break;
>>> default:
>>> rc = -EBUSY;
>>> - goto out_unlock;
>>> + goto out;
>>> }
>>> if (!force)
>>> - goto out_unlock;
>>> + goto out;
>>> folio_wait_writeback(src);
>>> }
>>>
>>> @@ -1166,7 +1201,8 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst,
>>> * This is much like races on refcount of oldpage: just don't BUG().
>>> */
>>> if (unlikely(!folio_trylock(dst)))
>>> - goto out_unlock;
>>> + goto out;
>>> + dst_locked = true;
>>>
>>> if (unlikely(!is_lru)) {
>>> __migrate_folio_record(dst, page_was_mapped, anon_vma);
>>> @@ -1188,7 +1224,7 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst,
>>> if (!src->mapping) {
>>> if (folio_test_private(src)) {
>>> try_to_free_buffers(src);
>>> - goto out_unlock_both;
>>> + goto out;
>>> }
>>> } else if (folio_mapped(src)) {
>>> /* Establish migration ptes */
>>> @@ -1203,74 +1239,26 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst,
>>> return MIGRATEPAGE_UNMAP;
>>> }
>>>
>>> - if (page_was_mapped)
>>> - remove_migration_ptes(src, src, false);
>>> -
>>> -out_unlock_both:
>>> - folio_unlock(dst);
>>> -out_unlock:
>>> - /* Drop an anon_vma reference if we took one */
>>> - if (anon_vma)
>>> - put_anon_vma(anon_vma);
>>> - folio_unlock(src);
>>> out:
>>> -
>>> - return rc;
>>> -}
>>> -
>>> -/* Obtain the lock on page, remove all ptes. */
>>> -static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page,
>>> - unsigned long private, struct folio *src,
>>> - struct folio **dstp, int force, bool force_lock,
>>> - enum migrate_mode mode, enum migrate_reason reason,
>>> - struct list_head *ret)
>>> -{
>>> - struct folio *dst;
>>> - int rc = MIGRATEPAGE_UNMAP;
>>> - struct page *newpage = NULL;
>>> -
>>> - if (!thp_migration_supported() && folio_test_transhuge(src))
>>> - return -ENOSYS;
>>> -
>>> - 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;
>>> - }
>>> -
>>> - newpage = get_new_page(&src->page, private);
>>> - if (!newpage)
>>> - return -ENOMEM;
>>> - dst = page_folio(newpage);
>>> - *dstp = dst;
>>> -
>>> - dst->private = NULL;
>>> - rc = __migrate_folio_unmap(src, dst, force, force_lock, mode);
>>> - if (rc == MIGRATEPAGE_UNMAP)
>>> - return rc;
>>> -
>>> /*
>>> * A page that has not been migrated will have kept its
>>> * references and be restored.
>>> */
>>> /* restore the folio to right list. */
>>
>> This comment is stale. Probably should be
>> /* Keep the folio and we will try it again later */
>
> Good catch! Will revise this in the next version.
>
> Best Regards,
> Huang, Ying
>
>>> - if (rc != -EAGAIN && rc != -EDEADLOCK)
>>> - list_move_tail(&src->lru, ret);
>>> + if (rc == -EAGAIN || rc == -EDEADLOCK)
>>> + ret = NULL;
>>>
>>> - if (put_new_page)
>>> - put_new_page(&dst->page, private);
>>> - else
>>> - folio_put(dst);
>>> + migrate_folio_undo_src(src, page_was_mapped, anon_vma, locked, ret);
>>> + migrate_folio_undo_dst(dst, dst_locked, put_new_page, private);
>>>
>>> return rc;
>>> }
>>>
>>> -static int __migrate_folio_move(struct folio *src, struct folio *dst,
>>> - enum migrate_mode mode)
>>> +/* Migrate the folio to the newly allocated folio in dst. */
>>> +static int migrate_folio_move(free_page_t put_new_page, unsigned long private,
>>> + struct folio *src, struct folio *dst,
>>> + enum migrate_mode mode, enum migrate_reason reason,
>>> + struct list_head *ret)
>>> {
>>> int rc;
>>> int page_was_mapped = 0;
>>> @@ -1283,12 +1271,8 @@ static int __migrate_folio_move(struct folio *src, struct folio *dst,
>>> list_del(&dst->lru);
>>>
>>> rc = move_to_new_folio(dst, src, mode);
>>> -
>>> - if (rc == -EAGAIN) {
>>> - list_add(&dst->lru, prev);
>>> - __migrate_folio_record(dst, page_was_mapped, anon_vma);
>>> - return rc;
>>> - }
>>> + if (rc)
>>> + goto out;
>>>
>>> if (unlikely(!is_lru))
>>> goto out_unlock_both;
>>> @@ -1302,70 +1286,45 @@ static int __migrate_folio_move(struct folio *src, struct folio *dst,
>>> * unsuccessful, and other cases when a page has been temporarily
>>> * isolated from the unevictable LRU: but this case is the easiest.
>>> */
>>> - if (rc == MIGRATEPAGE_SUCCESS) {
>>> - folio_add_lru(dst);
>>> - if (page_was_mapped)
>>> - lru_add_drain();
>>> - }
>>> + folio_add_lru(dst);
>>> + if (page_was_mapped)
>>> + lru_add_drain();
>>>
>>> if (page_was_mapped)
>>> - remove_migration_ptes(src,
>>> - rc == MIGRATEPAGE_SUCCESS ? dst : src, false);
>>> + remove_migration_ptes(src, dst, false);
>>>
>>> out_unlock_both:
>>> folio_unlock(dst);
>>> - /* Drop an anon_vma reference if we took one */
>>> - if (anon_vma)
>>> - put_anon_vma(anon_vma);
>>> - folio_unlock(src);
>>> + set_page_owner_migrate_reason(&dst->page, reason);
>>> /*
>>> * If migration is successful, decrease refcount of dst,
>>> * which will not free the page because new page owner increased
>>> * refcounter.
>>> */
>>> - if (rc == MIGRATEPAGE_SUCCESS)
>>> - folio_put(dst);
>>> -
>>> - return rc;
>>> -}
>>> -
>>> -/* Migrate the folio to the newly allocated folio in dst. */
>>> -static int migrate_folio_move(free_page_t put_new_page, unsigned long private,
>>> - struct folio *src, struct folio *dst,
>>> - enum migrate_mode mode, enum migrate_reason reason,
>>> - struct list_head *ret)
>>> -{
>>> - int rc;
>>> -
>>> - rc = __migrate_folio_move(src, dst, mode);
>>> - if (rc == MIGRATEPAGE_SUCCESS)
>>> - set_page_owner_migrate_reason(&dst->page, reason);
>>> -
>>> - if (rc != -EAGAIN) {
>>> - /*
>>> - * A folio that has been migrated has all references
>>> - * removed and will be freed. A folio that has not been
>>> - * migrated will have kept its references and be restored.
>>> - */
>>> - list_del(&src->lru);
>>> - }
>>> + folio_put(dst);
>>>
>>> /*
>>> - * If migration is successful, releases reference grabbed during
>>> - * isolation. Otherwise, restore the folio to right list unless
>>> - * we want to retry.
>>> + * A page that has been migrated has all references removed
>>> + * and will be freed.
>>> */
>>> - if (rc == MIGRATEPAGE_SUCCESS) {
>>> - migrate_folio_done(src, reason);
>>> - } else if (rc != -EAGAIN) {
>>> - list_add_tail(&src->lru, ret);
>>> + list_del(&src->lru);
>>> + /* Drop an anon_vma reference if we took one */
>>> + if (anon_vma)
>>> + put_anon_vma(anon_vma);
>>> + folio_unlock(src);
>>> + migrate_folio_done(src, reason);
>>>
>>> - if (put_new_page)
>>> - put_new_page(&dst->page, private);
>>> - else
>>> - folio_put(dst);
>>> + return rc;
>>> +out:
>>> + if (rc == -EAGAIN) {
>>> + list_add(&dst->lru, prev);
>>> + __migrate_folio_record(dst, page_was_mapped, anon_vma);
>>> + return rc;
>>> }
>>>
>>> + migrate_folio_undo_src(src, page_was_mapped, anon_vma, true, ret);
>>> + migrate_folio_undo_dst(dst, true, put_new_page, private);
>>> +
>>> return rc;
>>> }
>>>
>>> @@ -1897,9 +1856,9 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>>>
>>> __migrate_folio_extract(dst, &page_was_mapped, &anon_vma);
>>> migrate_folio_undo_src(folio, page_was_mapped, anon_vma,
>>> - ret_folios);
>>> + true, ret_folios);
>>> list_del(&dst->lru);
>>> - migrate_folio_undo_dst(dst, put_new_page, private);
>>> + migrate_folio_undo_dst(dst, true, put_new_page, private);
>>> dst = dst2;
>>> dst2 = list_next_entry(dst, lru);
>>> }
>>> --
>>> 2.35.1
>>
>> Everything else looks good to me, just need to fix the two comments above.
>> Reviewed-by: Zi Yan <[email protected]>
>>
>> --
>> Best Regards,
>> Yan, Zi

--
Best Regards,
Yan, Zi

Attachments:

signature.asc (854.00 B)
OpenPGP digital signature

2023-02-10 07:10:20

by Huang, Ying

[permalink] [raw]

Subject: Re: [PATCH -v4 7/9] migrate_pages: share more code between _unmap and _move

Zi Yan <[email protected]> writes:

> On 8 Feb 2023, at 7:02, Huang, Ying wrote:
>
>> Zi Yan <[email protected]> writes:
>>
>>> On 6 Feb 2023, at 1:33, Huang Ying wrote:
>>>
>>>> This is a code cleanup patch to reduce the duplicated code between the
>>>> _unmap and _move stages of migrate_pages(). No functionality change
>>>> is expected.
>>>>
>>>> Signed-off-by: "Huang, Ying" <[email protected]>
>>>> Cc: Zi Yan <[email protected]>
>>>> Cc: Yang Shi <[email protected]>
>>>> Cc: Baolin Wang <[email protected]>
>>>> Cc: Oscar Salvador <[email protected]>
>>>> Cc: Matthew Wilcox <[email protected]>
>>>> Cc: Bharata B Rao <[email protected]>
>>>> Cc: Alistair Popple <[email protected]>
>>>> Cc: haoxin <[email protected]>
>>>> Cc: Minchan Kim <[email protected]>
>>>> Cc: Mike Kravetz <[email protected]>
>>>> Cc: Hyeonggon Yoo <[email protected]>
>>>> ---
>>>> mm/migrate.c | 203 ++++++++++++++++++++-------------------------------
>>>> 1 file changed, 81 insertions(+), 122 deletions(-)
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index 23eb01cfae4c..9378fa2ad4a5 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -1037,6 +1037,7 @@ static void __migrate_folio_extract(struct folio *dst,
>>>> static void migrate_folio_undo_src(struct folio *src,
>>>> int page_was_mapped,
>>>> struct anon_vma *anon_vma,
>>>> + bool locked,
>>>> struct list_head *ret)
>>>> {
>>>> if (page_was_mapped)
>>>> @@ -1044,16 +1045,20 @@ static void migrate_folio_undo_src(struct folio *src,
>>>> /* Drop an anon_vma reference if we took one */
>>>> if (anon_vma)
>>>> put_anon_vma(anon_vma);
>>>> - folio_unlock(src);
>>>> - list_move_tail(&src->lru, ret);
>>>> + if (locked)
>>>> + folio_unlock(src);
>>>
>>> Having a comment would be better.
>>> /* A page that has not been migrated, move it to a list for later restoration */
>>
>> Emm... the page state has been restored in the previous operations of
>> the function. This is the last step and the page will be moved to
>> "return" list, then the caller of migrate_pages() will call
>> putback_movable_pages().
>
> But if (rc == -EAGAIN || rc == -EDEADLOCK) then ret will be NULL, thus the page
> will not be put back, right?

Yes. That is a special case.

> And for both cases, the src page state is not
> changed at all.

Their state should be restored to the original state too for being
processed again. That is done in the previous operations too. For
example, if the folio has been locked, before return with -EAGAIN, we
need to unlock the folio, otherwise, we will run into double lock.

> So probably only call migrate_folio_undo_src() when
> (rc != -EAGAIN && rc != -EDEADLOCK)? And still require ret to be non NULL.
>
>>
>> We have some comments for the function (migrate_folio_undo_src()) as
>> follows,
>>
>> /* Restore the source folio to the original state upon failure */
>>
>>>> + if (ret)
>>>> + list_move_tail(&src->lru, ret);
>>>> }
>>>>

[snip]

Best Regards,
Huang, Ying

2023-02-13 06:56:42

by Huang, Ying

[permalink] [raw]

Subject: Re: [PATCH -v4 5/9] migrate_pages: batch _unmap and _move

Zi Yan <[email protected]> writes:

> On 6 Feb 2023, at 1:33, Huang Ying wrote:
>
>> In this patch the _unmap and _move stage of the folio migration is
>> batched. That for, previously, it is,
>>
>> for each folio
>> _unmap()
>> _move()
>>
>> Now, it is,
>>
>> for each folio
>> _unmap()
>> for each folio
>> _move()
>>
>> Based on this, we can batch the TLB flushing and use some hardware
>> accelerator to copy folios between batched _unmap and batched _move
>> stages.
>>
>> Signed-off-by: "Huang, Ying" <[email protected]>
>> Tested-by: Hyeonggon Yoo <[email protected]>
>> Cc: Zi Yan <[email protected]>
>> Cc: Yang Shi <[email protected]>
>> Cc: Baolin Wang <[email protected]>
>> Cc: Oscar Salvador <[email protected]>
>> Cc: Matthew Wilcox <[email protected]>
>> Cc: Bharata B Rao <[email protected]>
>> Cc: Alistair Popple <[email protected]>
>> Cc: haoxin <[email protected]>
>> Cc: Minchan Kim <[email protected]>
>> Cc: Mike Kravetz <[email protected]>
>> ---
>> mm/migrate.c | 208 +++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 184 insertions(+), 24 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 0428449149f4..fa7212330cb6 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1033,6 +1033,33 @@ static void __migrate_folio_extract(struct folio *dst,
>> dst->private = NULL;
>> }
>>
>> +/* Restore the source folio to the original state upon failure */
>> +static void migrate_folio_undo_src(struct folio *src,
>> + int page_was_mapped,
>> + struct anon_vma *anon_vma,
>> + struct list_head *ret)
>> +{
>> + if (page_was_mapped)
>> + remove_migration_ptes(src, src, false);
>> + /* Drop an anon_vma reference if we took one */
>> + if (anon_vma)
>> + put_anon_vma(anon_vma);
>> + folio_unlock(src);
>> + list_move_tail(&src->lru, ret);
>> +}
>> +
>> +/* Restore the destination folio to the original state upon failure */
>> +static void migrate_folio_undo_dst(struct folio *dst,
>> + free_page_t put_new_page,
>> + unsigned long private)
>> +{
>> + folio_unlock(dst);
>> + if (put_new_page)
>> + put_new_page(&dst->page, private);
>> + else
>> + folio_put(dst);
>> +}
>> +
>> /* Cleanup src folio upon migration success */
>> static void migrate_folio_done(struct folio *src,
>> enum migrate_reason reason)
>> @@ -1052,7 +1079,7 @@ static void migrate_folio_done(struct folio *src,
>> }
>>
>> static int __migrate_folio_unmap(struct folio *src, struct folio *dst,
>> - int force, enum migrate_mode mode)
>> + int force, bool force_lock, enum migrate_mode mode)
>> {
>> int rc = -EAGAIN;
>> int page_was_mapped = 0;
>> @@ -1079,6 +1106,17 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst,
>> if (current->flags & PF_MEMALLOC)
>> goto out;
>>
>> + /*
>> + * We have locked some folios, to avoid deadlock, we cannot
>> + * lock the folio synchronously. Go out to process (and
>> + * unlock) all the locked folios. Then we can lock the folio
>> + * synchronously.
>> + */
> The comment alone is quite confusing and the variable might be better
> renamed to avoid_force_lock, since there is a force variable to force
> lock folio already. And the variable intends to discourage force lock
> on a folio to avoid potential deadlock.
>
> How about? Since "lock synchronously" might not be as straightforward
> as wait to lock.
>
> /*
> * We have locked some folios and are going to wait to lock this folio.
> * To avoid a potential deadlock, let's bail out and not do that. The
> * locked folios will be moved and unlocked, then we can wait to lock
> * this folio
> */
>
>> + if (!force_lock) {
>> + rc = -EDEADLOCK;
>> + goto out;
>> + }
>> +
>> folio_lock(src);
>> }
>>
>> @@ -1187,10 +1225,20 @@ static int __migrate_folio_move(struct folio *src, struct folio *dst,
>> int page_was_mapped = 0;
>> struct anon_vma *anon_vma = NULL;
>> bool is_lru = !__PageMovable(&src->page);
>> + struct list_head *prev;
>>
>> __migrate_folio_extract(dst, &page_was_mapped, &anon_vma);
>> + prev = dst->lru.prev;
>> + list_del(&dst->lru);
>>
>> rc = move_to_new_folio(dst, src, mode);
>> +
>> + if (rc == -EAGAIN) {
>> + list_add(&dst->lru, prev);
>> + __migrate_folio_record(dst, page_was_mapped, anon_vma);
>> + return rc;
>> + }
>> +
>> if (unlikely(!is_lru))
>> goto out_unlock_both;
>>
>> @@ -1233,7 +1281,7 @@ static int __migrate_folio_move(struct folio *src, struct folio *dst,
>> /* Obtain the lock on page, remove all ptes. */
>> static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page,
>> unsigned long private, struct folio *src,
>> - struct folio **dstp, int force,
>> + struct folio **dstp, int force, bool force_lock,
>> enum migrate_mode mode, enum migrate_reason reason,
>> struct list_head *ret)
>> {
>> @@ -1261,7 +1309,7 @@ static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page
>> *dstp = dst;
>>
>> dst->private = NULL;
>> - rc = __migrate_folio_unmap(src, dst, force, mode);
>> + rc = __migrate_folio_unmap(src, dst, force, force_lock, mode);
>> if (rc == MIGRATEPAGE_UNMAP)
>> return rc;
>>
>> @@ -1270,7 +1318,7 @@ static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page
>> * references and be restored.
>> */
>> /* restore the folio to right list. */
>> - if (rc != -EAGAIN)
>> + if (rc != -EAGAIN && rc != -EDEADLOCK)
>> list_move_tail(&src->lru, ret);
>>
>> if (put_new_page)
>> @@ -1309,9 +1357,8 @@ static int migrate_folio_move(free_page_t put_new_page, unsigned long private,
>> */
>> if (rc == MIGRATEPAGE_SUCCESS) {
>> migrate_folio_done(src, reason);
>> - } else {
>> - if (rc != -EAGAIN)
>> - list_add_tail(&src->lru, ret);
>> + } else if (rc != -EAGAIN) {
>> + list_add_tail(&src->lru, ret);
>>
>> if (put_new_page)
>> put_new_page(&dst->page, private);
>> @@ -1591,7 +1638,7 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>> enum migrate_mode mode, int reason, struct list_head *ret_folios,
>> struct migrate_pages_stats *stats)
>
> Like I said in my last comment to this patch, migrate_pages_batch() function
> deserves a detailed comment about its working flow including the error handling.
> Now you only put some in the git log, which is hard to access after several code
> changes later.
>
> How about?
>
> /*
> * migrate_pages_batch() first unmaps pages in the from as many as possible,
> * then migrates the unmapped pages. During unmap process, different situations
> * are handled differently:
> * 1. ENOSYS, unsupported large folio migration: move to ret_folios list
> * 2. ENOMEM, lower memory at the destination: migrate existing unmapped folios
> * and stop, since existing unmapped folios have new pages allocated and can
> * be migrated
> * 3. EDEADLOCK, to be unmapped page is locked by someone else, to avoid deadlock,
> * we migrate existing unmapped pages and try to lock again
> * 4. MIGRATEPAGE_SUCCESS, the folios was freed under us: no action
> * 5. MIGRATEPAGE_UNMAP, unmap succeeded: set avoid_force_lock to true to avoid
> * wait to lock a folio in the future to avoid deadlock.
> *
> * For folios unmapped but cannot be migrated, we will restore their original
> * states during cleanup stage at the end.
> */

Sorry, I didn't notice the above comments in the previous reply.

The comments appear to too detailed for me. I think that it's better
for people to get the details from the code itself. So, I want to use
the much simplified version as below.

/*
* migrate_pages_batch() first unmaps folios in the from list as many as
* possible, then move the unmapped folios.
*/

Best Regards,
Huang, Ying

>> {
>> - int retry = 1;
>> + int retry;
>> int large_retry = 1;
>> int thp_retry = 1;
>> int nr_failed = 0;
>> @@ -1600,13 +1647,19 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>> int pass = 0;
>> bool is_large = false;
>> bool is_thp = false;
>> - struct folio *folio, *folio2, *dst = NULL;
>> - int rc, nr_pages;
>> + struct folio *folio, *folio2, *dst = NULL, *dst2;
>> + int rc, rc_saved, nr_pages;
>> LIST_HEAD(split_folios);
>> + LIST_HEAD(unmap_folios);
>> + LIST_HEAD(dst_folios);
>> bool nosplit = (reason == MR_NUMA_MISPLACED);
>> bool no_split_folio_counting = false;
>> + bool force_lock;
>>
>> -split_folio_migration:
>> +retry:
>> + rc_saved = 0;
>> + force_lock = true;
>> + retry = 1;
>> for (pass = 0;
>> pass < NR_MAX_MIGRATE_PAGES_RETRY && (retry || large_retry);
>> pass++) {
>> @@ -1628,16 +1681,15 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>> cond_resched();
>>
>> rc = migrate_folio_unmap(get_new_page, put_new_page, private,
>> - folio, &dst, pass > 2, mode,
>> - reason, ret_folios);
>> - if (rc == MIGRATEPAGE_UNMAP)
>> - rc = migrate_folio_move(put_new_page, private,
>> - folio, dst, mode,
>> - reason, ret_folios);
>> + folio, &dst, pass > 2, force_lock,
>> + 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
>> * -EAGAIN: stay on the from list
>> + * -EDEADLOCK: stay on the from list
>> * -ENOMEM: stay on the from list
>> * -ENOSYS: stay on the from list
>> * Other errno: put on ret_folios list
>> @@ -1672,7 +1724,7 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>> case -ENOMEM:
>> /*
>> * When memory is low, don't bother to try to migrate
>> - * other folios, just exit.
>> + * other folios, move unmapped folios, then exit.
>> */
>> if (is_large) {
>> nr_large_failed++;
>> @@ -1711,7 +1763,19 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>> /* nr_failed isn't updated for not used */
>> nr_large_failed += large_retry;
>> stats->nr_thp_failed += thp_retry;
>> - goto out;
>> + rc_saved = rc;
>> + if (list_empty(&unmap_folios))
>> + goto out;
>> + else
>> + goto move;
>> + case -EDEADLOCK:
>> + /*
>> + * The folio cannot be locked for potential deadlock.
>> + * Go move (and unlock) all locked folios. Then we can
>> + * try again.
>> + */
>> + rc_saved = rc;
>> + goto move;
>> case -EAGAIN:
>> if (is_large) {
>> large_retry++;
>> @@ -1725,6 +1789,15 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>> stats->nr_succeeded += nr_pages;
>> stats->nr_thp_succeeded += is_thp;
>> break;
>> + case MIGRATEPAGE_UNMAP:
>> + /*
>> + * We have locked some folios, don't force lock
>> + * to avoid deadlock.
>> + */
>> + force_lock = false;
>> + list_move_tail(&folio->lru, &unmap_folios);
>> + list_add_tail(&dst->lru, &dst_folios);
>> + break;
>> default:
>> /*
>> * Permanent failure (-EBUSY, etc.):
>> @@ -1748,12 +1821,95 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>> nr_large_failed += large_retry;
>> stats->nr_thp_failed += thp_retry;
>> stats->nr_failed_pages += nr_retry_pages;
>> +move:
>> + retry = 1;
>> + for (pass = 0;
>> + pass < NR_MAX_MIGRATE_PAGES_RETRY && (retry || large_retry);
>> + pass++) {
>> + retry = 0;
>> + large_retry = 0;
>> + thp_retry = 0;
>> + nr_retry_pages = 0;
>> +
>> + dst = list_first_entry(&dst_folios, struct folio, lru);
>> + dst2 = list_next_entry(dst, lru);
>> + list_for_each_entry_safe(folio, folio2, &unmap_folios, lru) {
>> + is_large = folio_test_large(folio);
>> + is_thp = is_large && folio_test_pmd_mappable(folio);
>> + nr_pages = folio_nr_pages(folio);
>> +
>> + cond_resched();
>> +
>> + rc = migrate_folio_move(put_new_page, private,
>> + folio, dst, mode,
>> + reason, ret_folios);
>> + /*
>> + * The rules are:
>> + * Success: folio will be freed
>> + * -EAGAIN: stay on the unmap_folios list
>> + * Other errno: put on ret_folios list
>> + */
>> + switch(rc) {
>> + case -EAGAIN:
>> + if (is_large) {
>> + large_retry++;
>> + thp_retry += is_thp;
>> + } else if (!no_split_folio_counting) {
>> + retry++;
>> + }
>> + nr_retry_pages += nr_pages;
>> + break;
>> + case MIGRATEPAGE_SUCCESS:
>> + stats->nr_succeeded += nr_pages;
>> + stats->nr_thp_succeeded += is_thp;
>> + break;
>> + default:
>> + if (is_large) {
>> + nr_large_failed++;
>> + stats->nr_thp_failed += is_thp;
>> + } else if (!no_split_folio_counting) {
>> + nr_failed++;
>> + }
>> +
>> + stats->nr_failed_pages += nr_pages;
>> + break;
>> + }
>> + dst = dst2;
>> + dst2 = list_next_entry(dst, lru);
>> + }
>> + }
>> + nr_failed += retry;
>> + nr_large_failed += large_retry;
>> + stats->nr_thp_failed += thp_retry;
>> + stats->nr_failed_pages += nr_retry_pages;
>> +
>> + if (rc_saved)
>> + rc = rc_saved;
>> + else
>> + rc = nr_failed + nr_large_failed;
>> +out:
>> + /* Cleanup remaining folios */
>> + dst = list_first_entry(&dst_folios, struct folio, lru);
>> + dst2 = list_next_entry(dst, lru);
>> + list_for_each_entry_safe(folio, folio2, &unmap_folios, lru) {
>> + int page_was_mapped = 0;
>> + struct anon_vma *anon_vma = NULL;
>> +
>> + __migrate_folio_extract(dst, &page_was_mapped, &anon_vma);
>> + migrate_folio_undo_src(folio, page_was_mapped, anon_vma,
>> + ret_folios);
>> + list_del(&dst->lru);
>> + migrate_folio_undo_dst(dst, put_new_page, private);
>> + dst = dst2;
>> + dst2 = list_next_entry(dst, lru);
>> + }
>> +
>> /*
>> * Try to migrate split folios of fail-to-migrate large folios, no
>> * nr_failed counting in this round, since all split folios of a
>> * large folio is counted as 1 failure in the first round.
>> */
>> - if (!list_empty(&split_folios)) {
>> + if (rc >= 0 && !list_empty(&split_folios)) {
>> /*
>> * Move non-migrated folios (after NR_MAX_MIGRATE_PAGES_RETRY
>> * retries) to ret_folios to avoid migrating them again.
>> @@ -1761,12 +1917,16 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>> list_splice_init(from, ret_folios);
>> list_splice_init(&split_folios, from);
>> no_split_folio_counting = true;
>> - retry = 1;
>> - goto split_folio_migration;
>> + goto retry;
>> }
>>
>> - rc = nr_failed + nr_large_failed;
>> -out:
>> + /*
>> + * We have unlocked all locked folios, so we can force lock now, let's
>> + * try again.
>> + */
>> + if (rc == -EDEADLOCK)
>> + goto retry;
>> +
>> return rc;
>> }
>>
>> --
>> 2.35.1
>
> After rename the variable (or give it a better name) and add the comments,
> you can add Reviewed-by: Zi Yan <[email protected]>
>
> Thanks.
>
> --
> Best Regards,
> Yan, Zi