Re: [PATCH v4 17/31] drivers: convert shrinkers to new count/scan API

[Glauber Costa <glommer@parallels.com>]

[-- Attachment #1: Type: text/plain, Size: 2864 bytes --]

Sorry for the following crappy message. I came travelling without my laptop.

Please note that one of my patches implement one shot shrinkers onto of vmpressure mechanism. It can still be called frequently, because right now it is called every time userspace would get an event. But at least it won't iterate.

You can try investigating if that interface suits your i915 needs better


Sent by Samsung Mobile



-------- Original message --------
From: Mel Gorman <mgorman@suse.de>
Date:
To: Kent Overstreet <koverstreet@google.com>
Cc: Glauber Costa <glommer@openvz.org>,linux-mm@kvack.org,cgroups@vger.kernel.org,Andrew Morton <akpm@linux-foundation.org>,Greg Thelen <gthelen@google.com>,kamezawa.hiroyu@jp.fujitsu.com,Michal Hocko <mhocko@suse.cz>,Johannes Weiner <hannes@cmpxchg.org>,Dave Chinner <dchinner@redhat.com>,intel-gfx@lists.freedesktop.org,dri-devel@lists.freedesktop.org,devel@driverdev.osuosl.org,Dan Magenheimer <dan.magenheimer@oracle.com>
Subject: Re: [PATCH v4 17/31] drivers: convert shrinkers to new count/scan API


On Tue, Apr 30, 2013 at 03:00:50PM -0700, Kent Overstreet wrote:
> On Tue, Apr 30, 2013 at 10:53:55PM +0100, Mel Gorman wrote:
> > On Sat, Apr 27, 2013 at 03:19:13AM +0400, Glauber Costa wrote:
> > > diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> > > index 03e44c1..8b9c1a6 100644
> > > --- a/drivers/md/bcache/btree.c
> > > +++ b/drivers/md/bcache/btree.c
> > > @@ -599,11 +599,12 @@ static int mca_reap(struct btree *b, struct closure *cl, unsigned min_order)
> > >    return 0;
> > >  }
> > >
> > > -static int bch_mca_shrink(struct shrinker *shrink, struct shrink_control *sc)
> > > +static long bch_mca_scan(struct shrinker *shrink, struct shrink_control *sc)
> > >  {
> > >    struct cache_set *c = container_of(shrink, struct cache_set, shrink);
> > >    struct btree *b, *t;
> > >    unsigned long i, nr = sc->nr_to_scan;
> > > + long freed = 0;
> > >
> > >    if (c->shrinker_disabled)
> > >            return 0;
> >
> > -1 if shrinker disabled?
> >
> > Otherwise if the shrinker is disabled we ultimately hit this loop in
> > shrink_slab_one()
>
> My memory is very hazy on this stuff, but I recall there being another
> loop that'd just spin if we always returned -1.
>
> (It might've been /proc/sys/vm/drop_caches, or maybe that was another
> bug..)
>

It might be worth chasing down what that bug was and fixing it.

> But 0 should certainly be safe - if we're always returning 0, then we're
> claiming we don't have anything to shrink.
>

It won't crash, but in Glauber's current code, it'll call you a few more
times uselessly and the scanned statistics become misleading. I think
Glauber/Dave's series is a big improvement over what we currently have
and it would be nice to get it ironed out.

--
Mel Gorman
SUSE Labs

[-- Attachment #2: Type: text/html, Size: 4190 bytes --]