Re: [PATCH -mm -V2] mm, swap: Fix race between swapoff and some swap operations

From: Paul E. McKenney
Date: Fri Dec 15 2017 - 00:49:42 EST


On Fri, Dec 15, 2017 at 09:33:03AM +0800, Huang, Ying wrote:
> Michal Hocko <mhocko@xxxxxxxxxx> writes:
>
> > On Thu 14-12-17 21:38:32, Huang, Ying wrote:
> >> From: Huang Ying <ying.huang@xxxxxxxxx>
> >>
> >> When the swapin is performed, after getting the swap entry information
> >> from the page table, system will swap in the swap entry, without any
> >> lock held to prevent the swap device from being swapoff. This may
> >> cause the race like below,
> >>
> >> CPU 1 CPU 2
> >> ----- -----
> >> do_swap_page
> >> swapin_readahead
> >> __read_swap_cache_async
> >> swapoff swapcache_prepare
> >> p->swap_map = NULL __swap_duplicate
> >> p->swap_map[?] /* !!! NULL pointer access */
> >>
> >> Because swap off is usually done when system shutdown only, the race
> >> may not hit many people in practice. But it is still a race need to
> >> be fixed.
> >>
> >> To fix the race, get_swap_device() is added to prevent swap device
> >> from being swapoff until put_swap_device() is called. When
> >> get_swap_device() is called, the caller should have some locks (like
> >> PTL, page lock, or swap_info_struct->lock) held to guarantee the swap
> >> entry is valid, or check the origin of swap entry again to make sure
> >> the swap device hasn't been swapoff already.
> >>
> >> Because swapoff() is very race code path, to make the normal path runs
> >
> > s@race@rare@ I suppose
>
> Oops, thanks for pointing this out!
>
> >> as fast as possible, SRCU instead of reference count is used to
> >> implement get/put_swap_device(). From get_swap_device() to
> >> put_swap_device(), the reader side of SRCU is locked, so
> >> synchronize_srcu() in swapoff() will wait until put_swap_device() is
> >> called.
> >
> > It is quite unfortunate to pull SRCU as a dependency to the core kernel.
> > Different attempts to do this have failed in the past. This one is
> > slightly different though because I would suspect that those tiny
> > systems do not configure swap. But who knows, maybe they do.
>
> I remember Paul said there is a tiny implementation of SRCU which can
> fit this requirement.
>
> Hi, Paul, whether my memory is correct?

Yes, if you build with CONFIG_SMP=n, then you will get Tiny SRCU, which
is quite compact.

Thanx, Paul

> > Anyway, if you are worried about performance then I would expect some
> > numbers to back that worry. So why don't simply start with simpler
> > ref count based and then optimize it later based on some actual numbers.
>
> My -V1 is based on ref count. I think the performance difference should
> be not measurable. The idea is that swapoff() is so rare, so we should
> accelerate normal path as much as possible, even if this will cause slow
> down in swapoff. If we cannot use SRCU in the end, we may try RCU,
> preempt off (for stop_machine()), etc.
>
> > Btw. have you considered pcp refcount framework. I would suspect that
> > this would give you close to SRCU performance.
>
> No. I think pcp refcount doesn't fit here. You should hold a initial
> refcount for pcp refcount, it isn't the case here.
>
> Best Regards,
> Huang, Ying
>