Re: [PATCH 6/9] drm/i915: Wire up shrinkctl->nr_scanned

Joonas Lahtinen
2017-10-16T12:57:12Z

On Mon, 2017-10-16 at 11:51 +0100, Chris Wilson wrote:
> Quoting Joonas Lahtinen (2017-10-16 11:46:09)
> > On Fri, 2017-10-13 at 21:26 +0100, Chris Wilson wrote:
> > > shrink_slab() allows us to report back the number of objects we
> > > successfully scanned (out of the target shrinkctl->nr_to_scan). As
> > > report the number of pages owned by each GEM object as a separate item
> > > to the shrinker, we cannot precisely control the number of shrinker
> > > objects we scan on each pass; and indeed may free more than requested.
> > > If we fail to tell the shrinker about the number of objects we process,
> > > it will continue to hold a grudge against us as any objects left
> > > unscanned are added to the next reclaim -- and so we will keep on
> > > "unfairly" shrinking our own slab in comparison to other slabs.
> > >
> > > v2: fixup the misplaced addition, we want to count everything we scan
> > > (to match the number we reported earlier) not just the objects we
> > > successfully validated and freed.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > Umm, full explanation and "v2" is bit misleading. Should not this be
> > one Fixes: if significant enough, or just a clear reference to
> > 912d572d63b8 ("drm/i915: wire up shrinkctl->nr_scanned")?
>
> It's the patch I had sent, and thought I acked for mm; I didn't check
> carefully enough. The current code is more or less a no-op, the counter
> is identical to free, so we don't change the reported value to
> shrinkctl. This patch is required to make the code do as the commitlog
> describes.
>
> So the code isn't broken and we don't need a Fixes tag, maybe just a
> References?

"References:" should be good.

Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation