Re: [PATCH] mm: vmscan: remove redundant querying to shrinker

[Andrew Morton]

On Sat, 15 Jun 2013 03:13:26 +0900 HeeSub Shin <heesub@xxxxxxxxx> wrote:

> Hello,
> 
> On Fri, Jun 14, 2013 at 8:10 PM, Minchan Kim <minchan@xxxxxxxxxx> wrote:
> 
> >
> > Hello,
> >
> > On Fri, Jun 14, 2013 at 07:07:51PM +0900, Heesub Shin wrote:
> > > shrink_slab() queries each slab cache to get the number of
> > > elements in it. In most cases such queries are cheap but,
> > > on some caches. For example, Android low-memory-killer,
> > > which is operates as a slab shrinker, does relatively
> > > long calculation once invoked and it is quite expensive.
> >
> > LMK as shrinker is really bad, which everybody didn't want
> > when we reviewed it a few years ago so that's a one of reason
> > LMK couldn't be promoted to mainline yet. So your motivation is
> > already not atrractive. ;-)
> >
> > >
> > > This patch removes redundant queries to shrinker function
> > > in the loop of shrink batch.
> >
> > I didn't review the patch and others don't want it, I guess.
> > Because slab shrink is under construction and many patches were
> > already merged into mmtom. Please look at latest mmotm tree.
> >
> >         git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git
> 
> 
> >
> > If you concern is still in there and it's really big concern of MM
> > we should take care, NOT LMK, plese, resend it.
> >
> >
> I've noticed that there are huge changes there in the recent mmotm and you
> guys already settled the issue of my concern. I usually keep track changes
> in recent mm-tree, but this time I didn't. My bad :-)
> 

I'm not averse to merging an improvement like this even if it gets
rubbed out by forthcoming changes.  The big changes may never get
merged or may be reverted.  And by merging this patch, others are more
likely to grab it, backport it into earlier kernels and benefit from
it.

Also, the problem which this simple patch fixes might be present in a
different form after the large patchset has been merged.  That does not
appear to be the case this time.

So I'd actually like to merge Heesub's patch.  Problem is, I don't have
a way to redistribute it for testing - I'd need to effectively revert
the whole thing when integrating Glauber's stuff on top, so nobody who
is using linux-next would test Heesub's change.  Drat.




However I'm a bit sceptical about the description here.  The shrinker
is supposed to special-case the "nr_to_scan == 0" case and AFAICT
drivers/staging/android/lowmemorykiller.c:lowmem_shrink() does do this,
and it looks like the function will be pretty quick in this case.

In other words, the behaviour of lowmem_shrink(nr_to_scan == 0) does
not match Heesub's description.  What's up with that?



Also, there is an obvious optimisation which we could make to
lowmem_shrink().  All this stuff:

	if (lowmem_adj_size < array_size)
		array_size = lowmem_adj_size;
	if (lowmem_minfree_size < array_size)
		array_size = lowmem_minfree_size;
	for (i = 0; i < array_size; i++) {
		if (other_free < lowmem_minfree[i] &&
		    other_file < lowmem_minfree[i]) {
			min_score_adj = lowmem_adj[i];
			break;
		}
	}

does nothing useful in the nr_to_scan==0 case and should be omitted for
this special case.  But this problem was fixed in the large shrinker
rework in -mm.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/