Re: [PATCH 12/28] shrinker: defer work only to kswapd

[Brian Foster <bfoster@redhat.com>]

On Fri, Nov 15, 2019 at 08:11:50AM +1100, Dave Chinner wrote:
> On Mon, Nov 04, 2019 at 10:29:54AM -0500, Brian Foster wrote:
> > On Fri, Nov 01, 2019 at 10:46:02AM +1100, Dave Chinner wrote:
> > > @@ -601,10 +605,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > >  	 * scanning at high prio and therefore should try to reclaim as much as
> > >  	 * possible.
> > >  	 */
> > > -	while (total_scan >= batch_size ||
> > > -	       total_scan >= freeable_objects) {
> > > +	while (scan_count >= batch_size ||
> > > +	       scan_count >= freeable_objects) {
> > >  		unsigned long ret;
> > > -		unsigned long nr_to_scan = min(batch_size, total_scan);
> > > +		unsigned long nr_to_scan = min_t(long, batch_size, scan_count);
> > >  
> > >  		shrinkctl->nr_to_scan = nr_to_scan;
> > >  		shrinkctl->nr_scanned = nr_to_scan;
> > > @@ -614,29 +618,29 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > >  		freed += ret;
> > >  
> > >  		count_vm_events(SLABS_SCANNED, shrinkctl->nr_scanned);
> > > -		total_scan -= shrinkctl->nr_scanned;
> > > -		scanned += shrinkctl->nr_scanned;
> > > +		scan_count -= shrinkctl->nr_scanned;
> > > +		scanned_objects += shrinkctl->nr_scanned;
> > >  
> > >  		cond_resched();
> > >  	}
> > > -
> > >  done:
> > > -	if (next_deferred >= scanned)
> > > -		next_deferred -= scanned;
> > > +	if (deferred_count)
> > > +		next_deferred = deferred_count - scanned_objects;
> > >  	else
> > > -		next_deferred = 0;
> > > +		next_deferred = scan_count;
> > 
> > Hmm.. so if there was no deferred count on this cycle, we set
> > next_deferred to whatever is left from scan_count and add that back into
> > the shrinker struct below. If there was a pending deferred count on this
> > cycle, we subtract what we scanned from that and add that value back.
> > But what happens to the remaining scan_count in the latter case? Is it
> > lost, or am I missing something?
> 
> if deferred_count is not zero, then it is kswapd that is running. It
> does the deferred work, and if it doesn't make progress then adding
> it's scan count to the deferred work doesn't matter. That's because
> it will come back with an increased priority in a short while and
> try to scan more of the deferred count plus it's larger scan count.
> 

Ok, so perhaps there is no functional reason to defer remaining scan
count from a context (i.e. kswapd) that attempts to process deferred
work...

> IOWs, if we defer kswapd unused scan count, we effectively increase
> the pressure as the priority goes up, potentially making the
> deferred count increase out of control. i.e. kswapd can make
> progress and free items, but the result is that it increased the
> deferred scan count rather than reducing it. This leads to excessive
> reclaim of the slab caches and kswapd can trash the caches long
> after the memory pressure has gone away...
> 

... yet if kswapd runs without pre-existing deferred work, that's
precisely what it does. next_deferred is set to remaining scan_count and
that is added back to the shrinker struct. So should kswapd generally
defer work or not? If the answer is sometimes, then please add a comment
to the next_deferred assignment to explain when/why.

> > For example, suppose we start this cycle with a large scan_count and
> > ->scan_objects() returned SHRINK_STOP before doing much work. In that
> > scenario, it looks like whether ->nr_deferred is 0 or not is the only
> > thing that determines whether we defer the entire remaining scan_count
> > or just what is left from the previous ->nr_deferred. The existing code
> > appears to consistently factor in what is left from the current scan
> > with the previous deferred count. Hm?
> 
> If kswapd doesn't have any deferred work, then it's largely no
> different in behaviour to direct reclaim. If it has no deferred
> work, then the shrinker is not getting stopped early in direct
> reclaim, so it's unlikely that kswapd is going to get stopped early,
> either....
> 

Then perhaps the logic could be simplified to explicitly not defer from
kswapd..?

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>