On Thu, Aug 24, 2017 at 06:45:46PM -0400, Josef Bacik wrote:
> 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@xxxxxxxxxxxxxx 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,
Cool, thanks for verifying it works as intended now, Josef. :P
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
