Re: [PATCH v2 02/28] vmscan: take at least one pass with shrinkers

[Joonsoo Kim <iamjoonsoo.kim-Hm3cg6mZ9cc@public.gmane.org>]

Hello, Glauber.

On Tue, Apr 09, 2013 at 11:43:33AM +0400, Glauber Costa wrote:
> On 04/09/2013 06:05 AM, Joonsoo Kim wrote:
> > I don't think so.
> > Yes, lowmem_shrink() return number of (in)active lru pages
> > when nr_to_scan is 0. And in shrink_slab(), we divide it by lru_pages.
> > lru_pages can vary where shrink_slab() is called, anyway, perhaps this
> > logic makes total_scan below 128.
> > 
> You may benefit from looking at the lowmemory patches in this patchset
> itself. We modified the shrinker API to separate the count and scan
> phases. With this, the whole nr_to_scan == 0 disappears and the code
> gets easier to follow.
> 
> >> > 
> >> > And, interestingly enough, when the file cache has been pruned down
> >> > to it's smallest possible size, that's when the shrinker *won't run*
> >> > because the that's when the total_scan will be smaller than the
> >> > batch size and hence shrinker won't get called.
> >> > 
> >> > The shrinker is hacky, abuses the shrinker API, and doesn't appear
> >> > to do what it is intended to do.  You need to fix the shrinker, not
> >> > use it's brokenness as an excuse to hold up a long overdue shrinker
> >> > rework.
> > Agreed. I also think shrinker rework is valuable and I don't want
> > to become a stopper for this change. But, IMHO, at least, we should
> > notify users of shrinker API to know how shrinker API behavior changed,
> 
> Except that the behavior didn't change.
> 
> > because this is unexpected behavior change when they used this API.
> > When they used this API, they can assume that it is possible to control
> > logic with seeks and return value(when nr_to_scan=0), but with this patch,
> > this assumption is broken.
> > 
> 
> Jonsoo, you are still missing the point. nr_to_scan=0 has nothing to do
> with this, or with this patch. nr_to_scan will reach 0 ANYWAY if you
> shrink all objects you have to shrink, which is a *very* common thing to
> happen.
> 
> The only case changed here is where this happen when attempting to
> shrink a small number of objects that is smaller than the batch size.
> 
> Also, again, the nr_to_scan=0 checks in the shrinker calls have nothing
> to do with that. They reflect the situation *BEFORE* the shrinker was
> called. So how many objects we shrunk afterwards have zero to do with
> it. This is just the shrinker API using the magic value of 0 to mean :
> "don't shrink, just tell me how much do you have", vs a positive number
> meaning "try to shrink as much as nr_to_scan objects".

Yes, I know that :)
It seems that I mislead you and you misunderstand what I want to say.
Sorry for my poor English.

I mean to say, changing when we attempt to shrink a small number of
objects(below batch size) can affect some users of API and their system.
Maybe they assume that if they have a little objects, shrinker will not
call do_shrinker_shrink(). But, with this patch, although they have a
little objects, shrinker call do_shrinker_shrink() at least once.

Thanks.

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