Re: [PATCH v2] mm/swap: fix race when skipping swapcache

[Minchan Kim <minchan@kernel.org>]

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@gmail.com> wrote:
> >
> > On Fri, Feb 9, 2024 at 3:42 AM Chris Li <chrisl@kernel.org> wrote:
> > >
> > > On Thu, Feb 8, 2024 at 11:01 AM Kairui Song <ryncsn@gmail.com> wrote:
> > > >
> > > > On Thu, Feb 8, 2024 at 2:36 PM Huang, Ying <ying.huang@intel.com> wrote:
> > > > >
> > > > > Kairui Song <ryncsn@gmail.com> writes:
> > > > >
> > > > > > On Thu, Feb 8, 2024 at 2:31 AM Minchan Kim <minchan@kernel.org> 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!