On Fri, Feb 16, 2024 at 11:58 AM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, 15 Feb 2024 12:55:11 -0800 Minchan Kim <minchan@xxxxxxxxxx> wrote: > > > Only concern of the approach is that it would be harder to have the fix > > in the stable tree. If there isn't strong objection, I prefer the > > Kairui's orginal solution(with some tweak of scheduler if it's > > necessary) first and then pursue your idea on latest tree. > > Do we agree that this fix is needed in -stable? For some reason I > don't have a cc:stable in the changelog. > > I'd like to move this patch into mm-hotfixes-stable in order to get it > upstream very soon. Are we all agreeable with that? I don't have an > Acked-by: David? > > I have a note (which dates back to an earlier version) that Barry would > be performing runtime testing. Has that occurred? yep. i wanted to check if Kairui's patch can also take a positive affection on my problem, a race between large folios swap-in and madvise pageout i reported here[1] but unfortunately I failed as Kairui's patch can only handle normal folio with one page as if (swapcache_prepare(entry)) supports only one entry. so please consider my test irrelevant for this moment. if Kairui's approach is accepted, I think I need to somehow make a batched API like swapcache_prepare_nr(entries, nr) afterwards. [1] https://lore.kernel.org/linux-mm/20240205095155.7151-1-v-songbaohua@xxxxxxxx/ > > Thanks. > > > Here's what is presently in mm-hotfixes-unstable: > > > > From: Kairui Song <kasong@xxxxxxxxxxx> > Subject: mm/swap: fix race when skipping swapcache > Date: Wed, 7 Feb 2024 02:25:59 +0800 > > When skipping swapcache for SWP_SYNCHRONOUS_IO, if two or more threads > swapin the same entry at the same time, they get different pages (A, B). > Before one thread (T0) finishes the swapin and installs page (A) to the > PTE, another thread (T1) could finish swapin of page (B), swap_free the > entry, then swap out the possibly modified page reusing the same entry. > It breaks the pte_same check in (T0) because PTE value is unchanged, > causing ABA problem. Thread (T0) will install a stalled page (A) into the > PTE and cause data corruption. > > One possible callstack is like this: > > CPU0 CPU1 > ---- ---- > do_swap_page() do_swap_page() with same entry > <direct swapin path> <direct swapin path> > <alloc page A> <alloc page B> > swap_read_folio() <- read to page A swap_read_folio() <- read to page B > <slow on later locks or interrupt> <finished swapin first> > .. set_pte_at() > swap_free() <- entry is free > <write to page B, now page A stalled> > <swap out page B to same swap entry> > pte_same() <- Check pass, PTE seems > unchanged, but page A > is stalled! > swap_free() <- page B content lost! > set_pte_at() <- staled page A installed! > > And besides, for ZRAM, swap_free() allows the swap device to discard the > entry content, so even if page (B) is not modified, if swap_read_folio() > on CPU0 happens later than swap_free() on CPU1, it may also cause data > loss. > > To fix this, reuse swapcache_prepare which will pin the swap entry using > the cache flag, and allow only one thread to pin it. Release the pin > after PT unlocked. Racers will simply busy wait since it's a rare and > very short event. > > Other methods like increasing the swap count don't seem to be a good idea > after some tests, that will cause racers to fall back to use the swap > cache again. Parallel swapin using different methods leads to a much more > complex scenario. > > Reproducer: > > This race issue can be triggered easily using a well constructed > reproducer and patched brd (with a delay in read path) [1]: > > With latest 6.8 mainline, race caused data loss can be observed easily: > $ gcc -g -lpthread test-thread-swap-race.c && ./a.out > Polulating 32MB of memory region... > Keep swapping out... > Starting round 0... > Spawning 65536 workers... > 32746 workers spawned, wait for done... > Round 0: Error on 0x5aa00, expected 32746, got 32743, 3 data loss! > Round 0: Error on 0x395200, expected 32746, got 32743, 3 data loss! > Round 0: Error on 0x3fd000, expected 32746, got 32737, 9 data loss! > Round 0 Failed, 15 data loss! > > This reproducer spawns multiple threads sharing the same memory region > using a small swap device. Every two threads updates mapped pages one by > one in opposite direction trying to create a race, with one dedicated > thread keep swapping out the data out using madvise. > > The reproducer created a reproduce rate of about once every 5 minutes, > so the race should be totally possible in production. > > After this patch, I ran the reproducer for over a few hundred rounds > and no data loss observed. > > Performance overhead is minimal, microbenchmark swapin 10G from 32G > zram: > > Before: 10934698 us > After: 11157121 us > Non-direct: 13155355 us (Dropping SWP_SYNCHRONOUS_IO flag) > > Link: https://lkml.kernel.org/r/20240206182559.32264-1-ryncsn@xxxxxxxxx > Fixes: 0bcac06f27d7 ("mm, swap: skip swapcache for swapin of synchronous device") > Reported-by: "Huang, Ying" <ying.huang@xxxxxxxxx> > Closes: https://lore.kernel.org/lkml/87bk92gqpx.fsf_-_@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > Link: https://github.com/ryncsn/emm-test-project/tree/master/swap-stress-race [1] > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx> > Reviewed-by: "Huang, Ying" <ying.huang@xxxxxxxxx> > Acked-by: Yu Zhao <yuzhao@xxxxxxxxxx> > Cc: Chris Li <chrisl@xxxxxxxxxx> > Cc: David Hildenbrand <david@xxxxxxxxxx> > Cc: Hugh Dickins <hughd@xxxxxxxxxx> > Cc: Johannes Weiner <hannes@xxxxxxxxxxx> > Cc: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx> > Cc: Michal Hocko <mhocko@xxxxxxxx> > Cc: Minchan Kim <minchan@xxxxxxxxxx> > Cc: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > Cc: Yu Zhao <yuzhao@xxxxxxxxxx> > Cc: Barry Song <21cnbao@xxxxxxxxx> > Cc: SeongJae Park <sj@xxxxxxxxxx> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > --- > > include/linux/swap.h | 5 +++++ > mm/memory.c | 15 +++++++++++++++ > mm/swap.h | 5 +++++ > mm/swapfile.c | 13 +++++++++++++ > 4 files changed, 38 insertions(+) > > --- a/include/linux/swap.h~mm-swap-fix-race-when-skipping-swapcache > +++ a/include/linux/swap.h > @@ -549,6 +549,11 @@ static inline int swap_duplicate(swp_ent > return 0; > } > > +static inline int swapcache_prepare(swp_entry_t swp) > +{ > + return 0; > +} > + > static inline void swap_free(swp_entry_t swp) > { > } > --- a/mm/memory.c~mm-swap-fix-race-when-skipping-swapcache > +++ a/mm/memory.c > @@ -3867,6 +3867,16 @@ vm_fault_t do_swap_page(struct vm_fault > if (!folio) { > if (data_race(si->flags & SWP_SYNCHRONOUS_IO) && > __swap_count(entry) == 1) { > + /* > + * Prevent parallel swapin from proceeding with > + * the cache flag. Otherwise, another thread may > + * finish swapin first, free the entry, and swapout > + * reusing the same entry. It's undetectable as > + * pte_same() returns true due to entry reuse. > + */ > + if (swapcache_prepare(entry)) > + goto out; > + > /* skip swapcache */ > folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, > vma, vmf->address, false); > @@ -4116,6 +4126,9 @@ vm_fault_t do_swap_page(struct vm_fault > unlock: > if (vmf->pte) > pte_unmap_unlock(vmf->pte, vmf->ptl); > + /* Clear the swap cache pin for direct swapin after PTL unlock */ > + if (folio && !swapcache) > + swapcache_clear(si, entry); > out: > if (si) > put_swap_device(si); > @@ -4124,6 +4137,8 @@ out_nomap: > if (vmf->pte) > pte_unmap_unlock(vmf->pte, vmf->ptl); > out_page: > + if (!swapcache) > + swapcache_clear(si, entry); > folio_unlock(folio); > out_release: > folio_put(folio); > --- a/mm/swapfile.c~mm-swap-fix-race-when-skipping-swapcache > +++ a/mm/swapfile.c > @@ -3365,6 +3365,19 @@ int swapcache_prepare(swp_entry_t entry) > return __swap_duplicate(entry, SWAP_HAS_CACHE); > } > > +void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry) > +{ > + struct swap_cluster_info *ci; > + unsigned long offset = swp_offset(entry); > + unsigned char usage; > + > + ci = lock_cluster_or_swap_info(si, offset); > + usage = __swap_entry_free_locked(si, offset, SWAP_HAS_CACHE); > + unlock_cluster_or_swap_info(si, ci); > + if (!usage) > + free_swap_slot(entry); > +} > + > struct swap_info_struct *swp_swap_info(swp_entry_t entry) > { > return swap_type_to_swap_info(swp_type(entry)); > --- a/mm/swap.h~mm-swap-fix-race-when-skipping-swapcache > +++ a/mm/swap.h > @@ -41,6 +41,7 @@ void __delete_from_swap_cache(struct fol > void delete_from_swap_cache(struct folio *folio); > void clear_shadow_from_swap_cache(int type, unsigned long begin, > unsigned long end); > +void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry); > struct folio *swap_cache_get_folio(swp_entry_t entry, > struct vm_area_struct *vma, unsigned long addr); > struct folio *filemap_get_incore_folio(struct address_space *mapping, > @@ -97,6 +98,10 @@ static inline int swap_writepage(struct > return 0; > } > > +static inline void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry) > +{ > +} > + > static inline struct folio *swap_cache_get_folio(swp_entry_t entry, > struct vm_area_struct *vma, unsigned long addr) > { > _ > > Thanks Barry
