Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 May 2020 14:44:31 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Johannes Weiner <hannes@cmpxchg.org>
cc:     Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Alex Shi <alex.shi@linux.alibaba.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH mmotm] mm/swap: fix livelock in
 __read_swap_cache_async()
In-Reply-To: <20200526154528.GA850116@cmpxchg.org>
Message-ID: <alpine.LSU.2.11.2005271428560.6459@eggly.anvils>
References: <alpine.LSU.2.11.2005212246080.8458@eggly.anvils> <20200526154528.GA850116@cmpxchg.org>
User-Agent: Alpine 2.11 (LSU 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 26 May 2020, Johannes Weiner wrote:
> On Thu, May 21, 2020 at 10:56:20PM -0700, Hugh Dickins wrote:
> > I've only seen this livelock on one machine (repeatably, but not to
> > order), and not fully analyzed it - two processes seen looping around
> > getting -EEXIST from swapcache_prepare(), I guess a third (at lower
> > priority? but wanting the same cpu as one of the loopers? preemption
> > or cond_resched() not enough to let it back in?) set SWAP_HAS_CACHE,
> > then went off into direct reclaim, scheduled away, and somehow could
> > not get back to add the page to swap cache and let them all complete.
> > 
> > Restore the page allocation in __read_swap_cache_async() to before
> > the swapcache_prepare() call: "mm: memcontrol: charge swapin pages
> > on instantiation" moved it outside the loop, which indeed looks much
> > nicer, but exposed this weakness.  We used to allocate new_page once
> > and then keep it across all iterations of the loop: but I think that
> > just optimizes for a rare case, and complicates the flow, so go with
> > the new simpler structure, with allocate+free each time around (which
> > is more considerate use of the memory too).
> > 
> > Fix the comment on the looping case, which has long been inaccurate:
> > it's not a racing get_swap_page() that's the problem here.
> > 
> > Fix the add_to_swap_cache() and mem_cgroup_charge() error recovery:
> > not swap_free(), but put_swap_page() to undo SWAP_HAS_CACHE, as was
> > done before; but delete_from_swap_cache() already includes it.
> > 
> > And one more nit: I don't think it makes any difference in practice,
> > but remove the "& GFP_KERNEL" mask from the mem_cgroup_charge() call:
> > add_to_swap_cache() needs that, to convert gfp_mask from user and page
> > cache allocation (e.g. highmem) to radix node allocation (lowmem), but
> > we don't need or usually apply that mask when charging mem_cgroup.
> > 
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > ---
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> > Mostly fixing mm-memcontrol-charge-swapin-pages-on-instantiation.patch
> > but now I see that mm-memcontrol-delete-unused-lrucare-handling.patch
> > made a further change here (took an arg off the mem_cgroup_charge call):
> > as is, this patch is diffed to go on top of both of them, and better
> > that I get it out now for Johannes look at; but could be rediffed for
> > folding into blah-instantiation.patch later.
> 
> IMO it's worth having as a separate change. Joonsoo was concerned
> about the ordering but I didn't see it. Having this sequence of
> changes on record would be good for later reference.

Yes, there would be some value in that: whichever way Andrew prefers.

Now that the Acks are safely in (thanks guys), I will concede that
that SWAP_HAS_CACHE occasional busywait loop is not ideal - but with
this patch, no worse than it was before.

Later on I hope to come back and do something better there: it's not
immediately clear why swapcache_prepare() is important, and it would
be nicer if add_to_swap_cache() were the thing to fail with -EEXIST
(because then there's a page in the cache that others can lock to
wait on if required); but there's memories I need to dredge up
before going that way, and it may turn out to be a delusion.

Hugh