Re: [PATCH 1/2] mm: use sc->priority for slab shrink targets

From: Josef Bacik <josef@toxicpanda.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Josef Bacik <josef@toxicpanda.com>,
	Andrey Ryabinin <aryabinin@virtuozzo.com>,
	minchan@kernel.org, linux-mm@kvack.org, hannes@cmpxchg.org,
	riel@redhat.com, akpm@linux-foundation.org, kernel-team@fb.com,
	Josef Bacik <jbacik@fb.com>
Subject: Re: [PATCH 1/2] mm: use sc->priority for slab shrink targets
Date: Thu, 24 Aug 2017 18:45:46 -0400	[thread overview]
Message-ID: <20170824224544.3yrsl36u536fgkhc@destiny> (raw)
In-Reply-To: <20170824221559.GF21024@dastard>

On Fri, Aug 25, 2017 at 08:15:59AM +1000, Dave Chinner wrote:
> On Thu, Aug 24, 2017 at 10:49:25AM -0400, Josef Bacik wrote:
> > On Thu, Aug 24, 2017 at 05:29:59PM +0300, Andrey Ryabinin wrote:
> > > 
> > > 
> > > On 08/22/2017 10:35 PM, josef@toxicpanda.com wrote:
> > > > --- a/mm/vmscan.c
> > > > +++ b/mm/vmscan.c
> > > > @@ -306,9 +306,7 @@ EXPORT_SYMBOL(unregister_shrinker);
> > > >  #define SHRINK_BATCH 128
> > > >  
> > > >  static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > > > -				    struct shrinker *shrinker,
> > > > -				    unsigned long nr_scanned,
> > > > -				    unsigned long nr_eligible)
> > > > +				    struct shrinker *shrinker, int priority)
> > > >  {
> > > >  	unsigned long freed = 0;
> > > >  	unsigned long long delta;
> > > > @@ -333,9 +331,8 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > > >  	nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
> > > >  
> > > >  	total_scan = nr;
> > > > -	delta = (4 * nr_scanned) / shrinker->seeks;
> > > > -	delta *= freeable;
> > > > -	do_div(delta, nr_eligible + 1);
> > > > +	delta = freeable >> priority;
> > > > +	delta = (4 * freeable) / shrinker->seeks;
> > > 
> > > Something is wrong. The first line does nothing.
> > > 
> > 
> > Lol jesus, nice catch, I'll fix this up.  Thanks,
> 
> Josef, this bug has been in every patch you've sent. What does
> fixing it do to the behaviour of the algorithm now? It's going to
> change it, for sure, so can you run all your behavioural
> characterisation tests and let us know what the difference between
> the broken and fixed patches are?

The bug made it so we were way more agressive with slab reclaim than we should
be.  My second patch masked this with the inactive/slab diff stuff, but I've
dropped that patch since its controversial and I don't really care to argue
about it anymore.  This patch still fixes the issue of us not reclaiming enough
in slab mostly workloads, I ran my fs_mark test before I sent out the new
version to verify there still isn't the huge drop in performance once reclaim
kicks in.  Without any changes we reclaimed basically no slab, with the bug in
place (without my second patch) we reclaimed all of the slab in one go, and with
the fixed patch we reclaim a proportional amount each time we enter the
shrinker.  Thanks,

Josef

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