Re: [PATCH v2] mm/swap: fix race when skipping swapcache
From: Minchan Kim
Date: Wed Feb 14 2024 - 19:44:27 EST
Hi Kairui,
On Tue, Feb 13, 2024 at 03:53:09AM +0800, Kairui Song wrote:
> On Fri, Feb 9, 2024 at 1:30 PM Kairui Song <ryncsn@xxxxxxxxx> wrote:
> >
> > On Fri, Feb 9, 2024 at 3:42 AM Chris Li <chrisl@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Feb 8, 2024 at 11:01 AM Kairui Song <ryncsn@xxxxxxxxx> wrote:
> > > >
> > > > On Thu, Feb 8, 2024 at 2:36 PM Huang, Ying <ying.huang@xxxxxxxxx> wrote:
> > > > >
> > > > > Kairui Song <ryncsn@xxxxxxxxx> writes:
> > > > >
> > > > > > On Thu, Feb 8, 2024 at 2:31 AM Minchan Kim <minchan@xxxxxxxxxx> wrote:
> > > > > >>
> > > > > >> On Wed, Feb 07, 2024 at 12:06:15PM +0800, Kairui Song wrote:
> > > > >
> > > > > [snip]
> > > > >
> > > > > >> >
> > > > > >> > So I think the thing is, it's getting complex because this patch
> > > > > >> > wanted to make it simple and just reuse the swap cache flags.
> > > > > >>
> > > > > >> I agree that a simple fix would be the important at this point.
> > > > > >>
> > > > > >> Considering your description, here's my understanding of the other idea:
> > > > > >> Other method, such as increasing the swap count, haven't proven effective
> > > > > >> in your tests. The approach risk forcing racers to rely on the swap cache
> > > > > >> again and the potential performance loss in race scenario.
> > > > > >>
> > > > > >> While I understand that simplicity is important, and performance loss
> > > > > >> in this case may be infrequent, I believe swap_count approach could be a
> > > > > >> suitable solution. What do you think?
> > > > > >
> > > > > > Hi Minchan
> > > > > >
> > > > > > Yes, my main concern was about simplicity and performance.
> > > > > >
> > > > > > Increasing swap_count here will also race with another process from
> > > > > > releasing swap_count to 0 (swapcache was able to sync callers in other
> > > > > > call paths but we skipped swapcache here).
> > > > >
> > > > > What is the consequence of the race condition?
> > > >
> > > > Hi Ying,
> > > >
> > > > It will increase the swap count of an already freed entry, this race
> > > > with multiple swap free/alloc logic that checks if count ==
> > > > SWAP_HAS_CACHE or sets count to zero, or repeated free of an entry,
> > > > all result in random corruption of the swap map. This happens a lot
> > > > during stress testing.
> > >
> > > In theory, the original code before your patch can get into a
> > > situation similar to what you try to avoid.
> > > CPU1 enters the do_swap_page() with entry swap count == 1 and
> > > continues handling the swap fault without swap cache. Then some
> > > operation bumps up the swap entry count and CPU2 enters the
> > > do_swap_page() racing with CPU1 with swap count == 2. CPU2 will need
> > > to go through the swap cache case. We still need to handle this
> > > situation correctly.
> >
> > Hi Chris,
> >
> > This won't happen, nothing can bump the swap entry count unless it's
> > swapped in and freed. There are only two places that call
> > swap_duplicate: unmap or fork, unmap need page mapped and entry alloc,
> > so it won't happen unless we hit the entry reuse issue. Fork needs the
> > VMA lock which we hold it during page fault.
> >
> > > So the complexity is already there.
> > >
> > > If we can make sure the above case works correctly, then one way to
> > > avoid the problem is just send the CPU2 to use the swap cache (without
> > > the swap cache by-passing).
> >
> > Yes, more auditing of existing code and explanation is needed to
> > ensure things won't go wrong, that's the reason I tried to avoid
> > things from going too complex...
> >
> > > > > > So the right step is: 1. Lock the cluster/swap lock; 2. Check if still
> > > > > > have swap_count == 1, bail out if not; 3. Set it to 2;
> > > > > > __swap_duplicate can be modified to support this, it's similar to
> > > > > > existing logics for SWAP_HAS_CACHE.
> > > > > >
> > > > > > And swap freeing path will do more things, swapcache clean up needs to
> > > > > > be handled even in the bypassing path since the racer may add it to
> > > > > > swapcache.
> > > > > >
> > > > > > Reusing SWAP_HAS_CACHE seems to make it much simpler and avoided many
> > > > > > overhead, so I used that way in this patch, the only issue is
> > > > > > potentially repeated page faults now.
> > > > > >
> > > > > > I'm currently trying to add a SWAP_MAP_LOCK (or SWAP_MAP_SYNC, I'm bad
> > > > > > at naming it) special value, so any racer can just spin on it to avoid
> > > > > > all the problems, how do you think about this?
> > > > >
> > > > > Let's try some simpler method firstly.
> > > >
> > > > Another simpler idea is, add a schedule() or
> > > > schedule_timeout_uninterruptible(1) in the swapcache_prepare failure
> > > > path before goto out (just like __read_swap_cache_async). I think this
> > > > should ensure in almost all cases, PTE is ready after it returns, also
> > > > yields more CPU.
> > >
> > > I mentioned in my earlier email and Ying points out that as well.
> > > Looping waiting inside do_swap_page() is bad because it is holding
> > > other locks.
> >
> > It's not looping here though, just a tiny delay, since
> > SWP_SYNCHRONOUS_IO is supposed to be super fast devices so a tiny
> > delay should be enough.
> >
> > > Sorry I started this idea but it seems no good.
> >
> > Not at all, more reviewing helps to find a better solution :)
> >
> > > If we can have CPU2 make forward progress without retrying
> > > the page fault would be the best, if possible.
> >
> > Yes, making CPU2 fall back to cached swapin path is doable after
> > careful auditing. Just CPU2 is usually slower than CPU1 due to cache
> > and timing, so what it does will most likely be in vain and need to be
> > reverted, causing more work for both code logic and CPU. The case of
> > the race (CPU2 went faster) is very rare.
> >
> > I'm not against the idea of bump count, it's better if things that can
> > be done without introducing too much noise. Will come back after more
> > tests and work on this.
>
> Hi,
>
> After some more testing, I still think bump swap count is a bad idea
> here. Besides the issues I mentioned above, adding folio into cache is
> fundamentally in conflict with the problem we are trying to solve:
> folio in swapcache can be swapped out without allocating a new entry,
> so the entry reuse issue will be much more serious.
>
> If we want to avoid this we have to release the cache unconditionally
> before folio unlock because it can't tell if there is a parallel cache
> bypassing swapin going on or not. This is unacceptable, it breaks
> normal usage of swap cache. There are also other potential issues like
> read ahead, race causing stalled folio remain in swap cache, so I dont
> think this is a good approach.
I also played the swap refcount stuff ideas and encoutered a lot
problems(e.g., kernel crash and/or data missing).
Now I realize your original solution would be best to fix under such a
small change.
Folks, please chime in if you have another idea.
>
> Instead if we add a schedule or schedule_timeout_uninterruptible(1),
How much your workload is going If we use schedule instead of
schedule_timeout_uninterruptible(1)? If that doesn't increase the
statistics a lot, I prefer the schedule.
(TBH, I didn't care much about the stat since it would be
very rare).
> it seems quite enough to avoid repeated page faults. I did following
> test with the reproducer I provided in the commit message:
>
> Using ZRAM instead of brd for more realistic workload:
> $ perf stat -e minor-faults,major-faults -I 30000 ./a.out
>
> Unpatched kernel:
> # time counts unit events
> 30.000096504 33,089 minor-faults:u
> 30.000096504 958,745 major-faults:u
> 60.000375308 22,150 minor-faults:u
> 60.000375308 1,221,665 major-faults:u
> 90.001062176 24,840 minor-faults:u
> 90.001062176 1,005,427 major-faults:u
> 120.004233268 23,634 minor-faults:u
> 120.004233268 1,045,596 major-faults:u
> 150.004530091 22,358 minor-faults:u
> 150.004530091 1,097,871 major-faults:u
>
> Patched kernel:
> # time counts unit events
> 30.000069753 280,094 minor-faults:u
> 30.000069753 1,198,871 major-faults:u
> 60.000415915 436,862 minor-faults:u
> 60.000415915 919,860 major-faults:u
> 90.000651670 359,176 minor-faults:u
> 90.000651670 955,340 major-faults:u
> 120.001088135 289,137 minor-faults:u
> 120.001088135 992,317 major-faults:u
>
> Indeed there is a huge increase of minor page faults, which indicate
> the raced path returned without handling the fault. This reproducer
> tries to maximize the race chance so the reading is more terrifying
> than usual.
>
> But after adding a schedule/schedule_timeout_uninterruptible(1) after
> swapcache_prepare failed, still using the same test:
>
> Patched kernel (adding a schedule() before goto out):
> # time counts unit events
> 30.000335901 43,066 minor-faults:u
> 30.000335901 1,163,232 major-faults:u
> 60.034629687 35,652 minor-faults:u
> 60.034629687 844,188 major-faults:u
> 90.034941472 34,957 minor-faults:u
> 90.034941472 1,218,174 major-faults:u
> 120.035255386 36,241 minor-faults:u
> 120.035255386 1,073,395 major-faults:u
> 150.035535786 33,057 minor-faults:u
> 150.035535786 1,171,684 major-faults:u
>
> Patched kernel (adding a schedule_timeout_uninterruptible(1) before goto out):
> # time counts unit events
> 30.000044452 26,748 minor-faults:u
> 30.000044452 1,202,064 major-faults:u
> 60.000317379 19,883 minor-faults:u
> 60.000317379 1,186,152 major-faults:u
> 90.000568779 18,333 minor-faults:u
> 90.000568779 1,151,015 major-faults:u
> 120.000787253 22,844 minor-faults:u
> 120.000787253 887,531 major-faults:u
> 150.001309065 18,900 minor-faults:u
> 150.001309065 1,211,894 major-faults:u
>
> The minor faults are basically the same as before, or even lower since
> other races are also reduced, with no observable performance
> regression so far.
> If the vague margin of this schedule call is still concerning, I think
> another approach is just a new swap map value for parallel cache
> bypassed swapping to loop on.
Long term, the swap map vaule would be good but for now, I prefer
your original solution with some delay with schedule.
Thanks, Kairui!