On Wed, Jan 15, 2025 at 03:37:15PM +0530, Shivank Garg wrote:
> Hi Byungchul,
>
> On 1/15/2025 3:23 PM, Byungchul Park wrote:
> > Hi,
> >
> > This is a part of luf(lazy unmap flush) patchset and also referred
> > several times by e.g. Shivank Garg and Zi Yan who are currently working
> > on page migration optimization. Why don't we take this first so that
> > such a migration optimization tries can use it.
> >
> > Byungchul
> >
> > --->8---
> > From a65a6e4975962707bf87171e317f005c6276887e Mon Sep 17 00:00:00 2001
> > From: Byungchul Park <byungchul@xxxxxx>
> > Date: Thu, 8 Aug 2024 15:53:58 +0900
> > Subject: [PATCH] mm: separate move/undo parts from migrate_pages_batch()
> >
> > Functionally, no change. This is a preparation for migration
> > optimization tries that require to use separated folio lists for its own
> > handling during migration. Refactored migrate_pages_batch() so as to
> > separate move/undo parts from migrate_pages_batch().
> >
> > Signed-off-by: Byungchul Park <byungchul@xxxxxx>
> > ---
> > mm/migrate.c | 134 +++++++++++++++++++++++++++++++--------------------
> > 1 file changed, 83 insertions(+), 51 deletions(-)
>
> [...]
>
> > + */
> > + switch(rc) {
> ^^^
> minor nit here - space after 'switch' before the parentheses.
> This code style issue was present in the original code and was carried
> over during refactoring.
I should've taken more care of it. Thank you.
Byungchul
> With this fix, please add-
> Reviewed-by: Shivank Garg <shivankg@xxxxxxx>
>
> > + case -EAGAIN:
> > + *retry += 1;
> > + *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;
> > + default:
> > + *nr_failed += 1;
> > + stats->nr_thp_failed += is_thp;
> > + stats->nr_failed_pages += nr_pages;
> > + break;
> > + }
> > + dst = dst2;
> > + dst2 = list_next_entry(dst, lru);
> > + }
> > +}
> > +
> > +static void migrate_folios_undo(struct list_head *src_folios,
> > + struct list_head *dst_folios,
> > + free_folio_t put_new_folio, unsigned long private,
> > + struct list_head *ret_folios)
> > +{
> > + struct folio *folio, *folio2, *dst, *dst2;
> > +
> > + dst = list_first_entry(dst_folios, struct folio, lru);
> > + dst2 = list_next_entry(dst, lru);
> > + list_for_each_entry_safe(folio, folio2, src_folios, lru) {
> > + int old_page_state = 0;
> > + struct anon_vma *anon_vma = NULL;
> > +
> > + __migrate_folio_extract(dst, &old_page_state, &anon_vma);
> > + migrate_folio_undo_src(folio, old_page_state & PAGE_WAS_MAPPED,
> > + anon_vma, true, ret_folios);
> > + list_del(&dst->lru);
> > + migrate_folio_undo_dst(dst, true, put_new_folio, private);
> > + dst = dst2;
> > + dst2 = list_next_entry(dst, lru);
> > + }
> > +}
> > +
> > /*
> > * migrate_pages_batch() first unmaps folios in the from list as many as
> > * possible, then move the unmapped folios.
> > @@ -1717,7 +1792,7 @@ static int migrate_pages_batch(struct list_head *from,
> > int pass = 0;
> > bool is_thp = false;
> > bool is_large = false;
> > - struct folio *folio, *folio2, *dst = NULL, *dst2;
> > + struct folio *folio, *folio2, *dst = NULL;
> > int rc, rc_saved = 0, nr_pages;
> > LIST_HEAD(unmap_folios);
> > LIST_HEAD(dst_folios);
> > @@ -1888,42 +1963,11 @@ static int migrate_pages_batch(struct list_head *from,
> > 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_thp = folio_test_large(folio) && folio_test_pmd_mappable(folio);
> > - nr_pages = folio_nr_pages(folio);
> > -
> > - cond_resched();
> > -
> > - rc = migrate_folio_move(put_new_folio, 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:
> > - retry++;
> > - 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;
> > - default:
> > - nr_failed++;
> > - stats->nr_thp_failed += is_thp;
> > - stats->nr_failed_pages += nr_pages;
> > - break;
> > - }
> > - dst = dst2;
> > - dst2 = list_next_entry(dst, lru);
> > - }
> > + /* Move the unmapped folios */
> > + migrate_folios_move(&unmap_folios, &dst_folios,
> > + put_new_folio, private, mode, reason,
> > + ret_folios, stats, &retry, &thp_retry,
> > + &nr_failed, &nr_retry_pages);
> > }
> > nr_failed += retry;
> > stats->nr_thp_failed += thp_retry;
> > @@ -1932,20 +1976,8 @@ static int migrate_pages_batch(struct list_head *from,
> > rc = rc_saved ? : nr_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 old_page_state = 0;
> > - struct anon_vma *anon_vma = NULL;
> > -
> > - __migrate_folio_extract(dst, &old_page_state, &anon_vma);
> > - migrate_folio_undo_src(folio, old_page_state & PAGE_WAS_MAPPED,
> > - anon_vma, true, ret_folios);
> > - list_del(&dst->lru);
> > - migrate_folio_undo_dst(dst, true, put_new_folio, private);
> > - dst = dst2;
> > - dst2 = list_next_entry(dst, lru);
> > - }
> > + migrate_folios_undo(&unmap_folios, &dst_folios,
> > + put_new_folio, private, ret_folios);
> >
> > return rc;
> > }
