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@xxxxxxxxxx> > > --- > > Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx> > > > 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
