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

[Minchan Kim]

Hello Andrew,

On Fri, Jun 14, 2013 at 04:04:25PM -0700, Andrew Morton wrote:
> 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.

Fair enough.

> 
> 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.

True but if you suggest a good reason to review the patch, I will do.
I will reply on his patch mail.

> 
> 
> 
> 
> 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.

-- 
Kind regards,
Minchan Kim
--
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/