Re: [PATCH] vmscan: set correct defer count for shrinker

[Shaohua Li <shli@fb.com>]

On Sat, Oct 15, 2016 at 11:48:13PM +0300, Vladimir Davydov wrote:
> On Thu, Oct 13, 2016 at 08:53:28AM +0200, Michal Hocko wrote:
> > On Wed 12-10-16 09:09:49, Shaohua Li wrote:
> > > Our system uses significantly more slab memory with memcg enabled with
> > > latest kernel. With 3.10 kernel, slab uses 2G memory, while with 4.6
> > > kernel, 6G memory is used. Looks the shrinker has problem. Let's see we
> > > have two memcg for one shrinker. In do_shrink_slab:
> > > 
> > > 1. Check cg1. nr_deferred = 0, assume total_scan = 700. batch size is 1024,
> > > then no memory is freed. nr_deferred = 700
> > > 2. Check cg2. nr_deferred = 700. Assume freeable = 20, then total_scan = 10
> > > or 40. Let's assume it's 10. No memory is freed. nr_deferred = 10.
> > > 
> > > The deferred share of cg1 is lost in this case. kswapd will free no
> > > memory even run above steps again and again.
> 
> I agree this is possible. IMO the ideal way to fix this problem would be
> making deferred counters per memory cgroup. That would also resolve
> possible fairness issues when objects deferred by one cgroup are
> reclaimed from another. However, it's unclear to me how to implement it
> w/o bringing in a lot of awkward code. So I guess your patch is
> reasonable for now. Apart from a couple nitpicks (below), it looks good
> to me:
> 
> Acked-by: Vladimir Davydov <vdavydov.dev@gmail.com>
> 
> > > @@ -312,7 +313,9 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > >  		pr_err("shrink_slab: %pF negative objects to delete nr=%ld\n",
> > >  		       shrinker->scan_objects, total_scan);
> > >  		total_scan = freeable;
> > > -	}
> > > +		next_deferred = nr;
> > > +	} else
> > > +		next_deferred = total_scan;
> 
> nitpick: Why do we want to handle this what-the-heck-is-going-on case in
> a special way? Why not just always assign total_scan to next_deferred
> here? I don't see how it could make things worse when total_scan gets
> screwed up.

I have no idea when this special case will hapen. I'd like to make it
conservative. Somebody knowing this code probably could help clean up.
 
> > >  
> > >  	/*
> > >  	 * We need to avoid excessive windup on filesystem shrinkers
> > > @@ -369,17 +372,22 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > >  
> > >  		count_vm_events(SLABS_SCANNED, nr_to_scan);
> > >  		total_scan -= nr_to_scan;
> > > +		scanned += nr_to_scan;
> 
> nitpick: We could get along w/o 'scanned' here:
> 
>                 next_deferred -= nr_to_scan;

In that special case the next_deferred could be smaller than total_scan. That
said, if we don't have the special case, your suggestion is good. I'm totally
open here.

> > >  
> > >  		cond_resched();
> > >  	}
> > >  
> > > +	if (next_deferred >= scanned)
> > > +		next_deferred -= scanned;
> > > +	else
> > > +		next_deferred = 0;
> 
> ... and this chunk wouldn't be needed then.

Ditto.

Thanks,
Shaohua

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>