Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone

[Mel Gorman]

On Fri, Jan 20, 2017 at 02:42:24PM +0800, Hillf Danton wrote:
> > @@ -1603,16 +1603,16 @@ int isolate_lru_page(struct page *page)
> >   * the LRU list will go small and be scanned faster than necessary, leading to
> >   * unnecessary swapping, thrashing and OOM.
> >   */
> > -static int too_many_isolated(struct pglist_data *pgdat, int file,
> > +static bool safe_to_isolate(struct pglist_data *pgdat, int file,
> >  		struct scan_control *sc)
> 
> I prefer the current function name.
> 

The restructure is to work with the workqueue api.

> >  {
> >  	unsigned long inactive, isolated;
> > 
> >  	if (current_is_kswapd())
> > -		return 0;
> > +		return true;
> > 
> > -	if (!sane_reclaim(sc))
> > -		return 0;
> > +	if (sane_reclaim(sc))
> > +		return true;
> 
> We only need a one-line change.

It's bool so the conversion is made to bool while it's being changed
anyway.

> > 
> >  	if (file) {
> >  		inactive = node_page_state(pgdat, NR_INACTIVE_FILE);
> > @@ -1630,7 +1630,7 @@ static int too_many_isolated(struct pglist_data *pgdat, int file,
> >  	if ((sc->gfp_mask & (__GFP_IO | __GFP_FS)) == (__GFP_IO | __GFP_FS))
> >  		inactive >>= 3;
> > 
> > -	return isolated > inactive;
> > +	return isolated < inactive;
> >  }
> > 
> >  static noinline_for_stack void
> > @@ -1719,12 +1719,28 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
> >  	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
> >  	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
> > 
> > -	while (unlikely(too_many_isolated(pgdat, file, sc))) {
> > -		congestion_wait(BLK_RW_ASYNC, HZ/10);
> > +	while (!safe_to_isolate(pgdat, file, sc)) {
> > +		long ret;
> > +
> > +		ret = wait_event_interruptible_timeout(pgdat->isolated_wait,
> > +			safe_to_isolate(pgdat, file, sc), HZ/10);
> > 
> >  		/* We are about to die and free our memory. Return now. */
> > -		if (fatal_signal_pending(current))
> > -			return SWAP_CLUSTER_MAX;
> > +		if (fatal_signal_pending(current)) {
> > +			nr_reclaimed = SWAP_CLUSTER_MAX;
> > +			goto out;
> > +		}
> > +
> > +		/*
> > +		 * If we reached the timeout, this is direct reclaim, and
> > +		 * pages cannot be isolated then return. If the situation
> 
> Please add something that we would rather shrink slab than go
> another round of nap.
> 

That's not necessarily true or even a good idea. It could result in
excessive slab shrinking that is no longer in proportion to LRU scanning
and increased contention within shrinkers.

> > +		 * persists for a long time then it'll eventually reach
> > +		 * the no_progress limit in should_reclaim_retry and consider
> > +		 * going OOM. In this case, do not wake the isolated_wait
> > +		 * queue as the wakee will still not be able to make progress.
> > +		 */
> > +		if (!ret && !current_is_kswapd() && !safe_to_isolate(pgdat, file, sc))
> > +			return 0;
> >  	}
> > 
> >  	lru_add_drain();
> > @@ -1839,6 +1855,10 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
> >  			stat.nr_activate, stat.nr_ref_keep,
> >  			stat.nr_unmap_fail,
> >  			sc->priority, file);
> > +
> > +out:
> > +	if (waitqueue_active(&pgdat->isolated_wait))
> > +		wake_up(&pgdat->isolated_wait);
> >  	return nr_reclaimed;
> >  }
> > 
> Is it also needed to check isolated_wait active before kswapd 
> takes nap?
> 

No because this is where pages were isolated and there is no putback
event that would justify waking the queue. There is a race between
waitqueue_active() and going to sleep that we rely on the timeout to
recover from.

-- 
Mel Gorman
SUSE Labs