| Message ID | 1700569840-17327-1-git-send-email-quic_charante@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] mm: migrate: rcu stalls because of invalid swap cache entries | expand |
On Tue, Nov 21, 2023 at 06:00:40PM +0530, Charan Teja Kalla wrote: > The below race on a folio between reclaim and migration exposed a bug > of not populating the swap cache with proper folio resulting into the > rcu stalls: Thank you for figuring out this race and describing it so well. It explains a few things I've seen, at least potentially. What would you think to this? I think a better fix would be to fix the swap cache to user multi-order entries, but I would like to see this backportable! diff --git a/mm/migrate.c b/mm/migrate.c index d9d2b9432e81..2d67ca47d2e2 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -405,6 +405,7 @@ int folio_migrate_mapping(struct address_space *mapping, int dirty; int expected_count = folio_expected_refs(mapping, folio) + extra_count; long nr = folio_nr_pages(folio); + long entries, i; if (!mapping) { /* Anonymous page without mapping */ @@ -442,8 +443,10 @@ int folio_migrate_mapping(struct address_space *mapping, folio_set_swapcache(newfolio); newfolio->private = folio_get_private(folio); } + entries = nr; } else { VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio); + entries = 1; } /* Move dirty while page refs frozen and newpage not yet exposed */ @@ -453,7 +456,11 @@ int folio_migrate_mapping(struct address_space *mapping, folio_set_dirty(newfolio); } - xas_store(&xas, newfolio); + /* Swap cache still stores N entries instead of a high-order entry */ + for (i = 0; i < entries; i++) { + xas_store(&xas, newfolio); + xas_next(&xas); + } /* * Drop cache reference from old page by unfreezing
Thanks Matthew! On 11/21/2023 9:43 PM, Matthew Wilcox wrote: > What would you think to this? I think a better fix would be to > fix the swap cache to user multi-order entries, but I would like to > see this backportable! > > diff --git a/mm/migrate.c b/mm/migrate.c > index d9d2b9432e81..2d67ca47d2e2 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -405,6 +405,7 @@ int folio_migrate_mapping(struct address_space *mapping, > int dirty; > int expected_count = folio_expected_refs(mapping, folio) + extra_count; > long nr = folio_nr_pages(folio); > + long entries, i; > > if (!mapping) { > /* Anonymous page without mapping */ > @@ -442,8 +443,10 @@ int folio_migrate_mapping(struct address_space *mapping, > folio_set_swapcache(newfolio); > newfolio->private = folio_get_private(folio); > } > + entries = nr; > } else { > VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio); > + entries = 1; > } > > /* Move dirty while page refs frozen and newpage not yet exposed */ > @@ -453,7 +456,11 @@ int folio_migrate_mapping(struct address_space *mapping, > folio_set_dirty(newfolio); > } > > - xas_store(&xas, newfolio); > + /* Swap cache still stores N entries instead of a high-order entry */ > + for (i = 0; i < entries; i++) { > + xas_store(&xas, newfolio); > + xas_next(&xas); > + } > > /* > * Drop cache reference from old page by unfreezing Seems a cleaner one to store N entries. Supporting swap cache for multi order entries might be time consuming. Till then, can we use this patch as the solution, with the proper commit log conveying revert of this patch when swap cache supported with Multi-order indices? Please Lmk, If I can raise this patch with suggested-by:you . Thanks Charan
HI Matthew, Just a ping to have your valuable opinion here. Thanks, Charan On 11/23/2023 7:55 PM, Charan Teja Kalla wrote: >> What would you think to this? I think a better fix would be to >> fix the swap cache to user multi-order entries, but I would like to >> see this backportable! >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index d9d2b9432e81..2d67ca47d2e2 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -405,6 +405,7 @@ int folio_migrate_mapping(struct address_space *mapping, >> int dirty; >> int expected_count = folio_expected_refs(mapping, folio) + extra_count; >> long nr = folio_nr_pages(folio); >> + long entries, i; >> >> if (!mapping) { >> /* Anonymous page without mapping */ >> @@ -442,8 +443,10 @@ int folio_migrate_mapping(struct address_space *mapping, >> folio_set_swapcache(newfolio); >> newfolio->private = folio_get_private(folio); >> } >> + entries = nr; >> } else { >> VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio); >> + entries = 1; >> } >> >> /* Move dirty while page refs frozen and newpage not yet exposed */ >> @@ -453,7 +456,11 @@ int folio_migrate_mapping(struct address_space *mapping, >> folio_set_dirty(newfolio); >> } >> >> - xas_store(&xas, newfolio); >> + /* Swap cache still stores N entries instead of a high-order entry */ >> + for (i = 0; i < entries; i++) { >> + xas_store(&xas, newfolio); >> + xas_next(&xas); >> + } >> >> /* >> * Drop cache reference from old page by unfreezing > Seems a cleaner one to store N entries. Supporting swap cache for multi > order entries might be time consuming. Till then, can we use this patch > as the solution, with the proper commit log conveying revert of this > patch when swap cache supported with Multi-order indices? > > Please Lmk, If I can raise this patch with suggested-by:you
diff --git a/mm/migrate.c b/mm/migrate.c index 35a8833..05cb4a9b 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -403,6 +403,7 @@ int folio_migrate_mapping(struct address_space *mapping, XA_STATE(xas, &mapping->i_pages, folio_index(folio)); struct zone *oldzone, *newzone; int dirty; + void *entry; int expected_count = folio_expected_refs(mapping, folio) + extra_count; long nr = folio_nr_pages(folio); @@ -454,6 +455,16 @@ int folio_migrate_mapping(struct address_space *mapping, } xas_store(&xas, newfolio); + entry = xas_next(&xas); + + if (nr > 1 && !xa_is_sibling(entry)) { + int i; + + for (i = 1; i < nr; ++i) { + xas_store(&xas, newfolio); + xas_next(&xas); + } + } /* * Drop cache reference from old page by unfreezing
The below race on a folio between reclaim and migration exposed a bug of not populating the swap cache with proper folio resulting into the rcu stalls: Reclaim migration (from mem offline) -------- ------------------ 1) folio_trylock(); 2) add_to_swap(): a) get swap_entry to store the thp folio through folio_alloc_swap(). b) do add_to_swap_cache() on a folio, which fills the xarray with folio corresponding to indexes of swap entries. Also dirty this folio. c) try_to_unmap(folio, TTU_SPLIT_HUGE_PMD) which splits the pmd, unmaps the folio and replace the mapped entries with swap entries. d) as the folio is dirty do, pageout()::mapping->a_ops->writepage(). This calls swap_writepage() which unlock the page from 1) and do submit_bio(). 3) Since the page can still be under writeback, add the folio added back to the LRU. 4) As the folio now on LRU, it is visible to migration thus will endup in offline_pages()->migrate_pages(): a) isolate the folio. b) do __unmap_and_move(): 1) lock the folio and wait till writeback is done. 2) Replace the eligible pte entries with migrate and then issue the move_to_new_folio(), which calls migrate_folio()-> folio_migrate_mapping(), for the pages on the swap cache which just replace a single swap cache entry source folio with destination folio and can endup in freeing the source folio. Now A process in parallel can endup in do_swap_page() which will try read the stale entry(of source folio) after step4 above and thus will endup in the below loop with rcu lock held. mapping_get_entry(): rcu_read_lock(); repeat: xas_reset(&xas); folio = xas_load(&xas); if (!folio || xa_is_value(folio)) goto out; if (!folio_try_get_rcu(folio)) goto repeat; folio_try_get_rcu(): if (unlikely(!folio_ref_add_unless(folio, count, 0))) { /* Either the folio has been freed, or will be freed. */ return false; Because of the source folio is freed in 4.b.2) The above loop can continue till the destination folio too is reclaimed where it is removed from the swap cache and then set the swap cache entry to zero where the xas_load() return 0 thus exit. And this destination folio can be either removed immediately as part of the reclaim or can stay longer in the swap cache because of parallel swapin happen between 3) and 4.b.1)(whose valid pte mappings, pointing to the source folio, is replaced with the destination folio). It is the latter case which is resulted into the rcu stalls. The similar sort of issue also reported sometime back and is fixed in [1]. This issue seems to be introduced from the commit 6b24ca4a1a8d ("mm: Use multi-index entries in the page cache"), in the function folio_migrate_mapping()[2]. Since a large folio to be migrated and present in the swap cache can't use the multi-index entries, and migrate code uses the same folio_migrate_mapping() for migrating this folio, any inputs you can provide to fix this issue, please? What I have thought is, if the adjacent entry in the xarray is not a sibling, then assume that it is not a multi-index entry thus store as 2^N consecutive entries. [1] https://lore.kernel.org/all/20180406030706.GA2434@hori1.linux.bs1.fc.nec.co.jp/T/#u [2] https://lore.kernel.org/linux-mm/20210715033704.692967-128-willy@infradead.org/#Z31mm:migrate.c Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com> --- mm/migrate.c | 11 +++++++++++ 1 file changed, 11 insertions(+)