| Message ID | 20180115212455.24046-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 15/01/2018 21:24, Chris Wilson wrote: > Since commit 4e773c3a8a69 ("drm/i915: Wire up shrinkctl->nr_scanned"), Sounds like it deserves this in the Fixes: tag as well? > we track the number of objects we scan and do not wish to exceed that as > it will overly penalise our own slabs under mempressure. Given that we > now know the target number of objects to scan, use that as our guide for > deciding to shrink as opposed to the number of objects we manage to > shrink (which doesn't correspond to the numbers we report to shrinkctl). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c > index 9029ed04879c..0e158f9287c4 100644 > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c > @@ -363,13 +363,13 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) > I915_SHRINK_BOUND | > I915_SHRINK_UNBOUND | > I915_SHRINK_PURGEABLE); > - if (freed < sc->nr_to_scan) > + if (sc->nr_scanned < sc->nr_to_scan) > freed += i915_gem_shrink(i915, > sc->nr_to_scan - sc->nr_scanned, > &sc->nr_scanned, > I915_SHRINK_BOUND | > I915_SHRINK_UNBOUND); > - if (freed < sc->nr_to_scan && current_is_kswapd()) { > + if (sc->nr_scanned < sc->nr_to_scan && current_is_kswapd()) { > intel_runtime_pm_get(i915); > freed += i915_gem_shrink(i915, > sc->nr_to_scan - sc->nr_scanned, > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-01-17 10:29:51) > > On 15/01/2018 21:24, Chris Wilson wrote: > > Since commit 4e773c3a8a69 ("drm/i915: Wire up shrinkctl->nr_scanned"), > > Sounds like it deserves this in the Fixes: tag as well? Not really, the commit maintained the old behaviour just missing an opportunity to take advantage of the new interface. Not strictly a fix, I'd rather let it percolate upstream slowly. > > we track the number of objects we scan and do not wish to exceed that as > > it will overly penalise our own slabs under mempressure. Given that we > > now know the target number of objects to scan, use that as our guide for > > deciding to shrink as opposed to the number of objects we manage to > > shrink (which doesn't correspond to the numbers we report to shrinkctl). > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c > > index 9029ed04879c..0e158f9287c4 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c > > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c > > @@ -363,13 +363,13 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) > > I915_SHRINK_BOUND | > > I915_SHRINK_UNBOUND | > > I915_SHRINK_PURGEABLE); > > - if (freed < sc->nr_to_scan) > > + if (sc->nr_scanned < sc->nr_to_scan) > > freed += i915_gem_shrink(i915, > > sc->nr_to_scan - sc->nr_scanned, > > &sc->nr_scanned, > > I915_SHRINK_BOUND | > > I915_SHRINK_UNBOUND); > > - if (freed < sc->nr_to_scan && current_is_kswapd()) { > > + if (sc->nr_scanned < sc->nr_to_scan && current_is_kswapd()) { > > intel_runtime_pm_get(i915); > > freed += i915_gem_shrink(i915, > > sc->nr_to_scan - sc->nr_scanned, > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Thanks, pushed this separately. -Chris
Quoting Tvrtko Ursulin (2018-01-17 10:29:51) > > On 15/01/2018 21:24, Chris Wilson wrote: > > Since commit 4e773c3a8a69 ("drm/i915: Wire up shrinkctl->nr_scanned"), > > Sounds like it deserves this in the Fixes: tag as well? On second thoughts, that we ask to free sc->nr_to_scan - sc->nr_scanned, so we shouldn't let that underflow! Fixes is then deserved. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c index 9029ed04879c..0e158f9287c4 100644 --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c @@ -363,13 +363,13 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) I915_SHRINK_BOUND | I915_SHRINK_UNBOUND | I915_SHRINK_PURGEABLE); - if (freed < sc->nr_to_scan) + if (sc->nr_scanned < sc->nr_to_scan) freed += i915_gem_shrink(i915, sc->nr_to_scan - sc->nr_scanned, &sc->nr_scanned, I915_SHRINK_BOUND | I915_SHRINK_UNBOUND); - if (freed < sc->nr_to_scan && current_is_kswapd()) { + if (sc->nr_scanned < sc->nr_to_scan && current_is_kswapd()) { intel_runtime_pm_get(i915); freed += i915_gem_shrink(i915, sc->nr_to_scan - sc->nr_scanned,
Since commit 4e773c3a8a69 ("drm/i915: Wire up shrinkctl->nr_scanned"), we track the number of objects we scan and do not wish to exceed that as it will overly penalise our own slabs under mempressure. Given that we now know the target number of objects to scan, use that as our guide for deciding to shrink as opposed to the number of objects we manage to shrink (which doesn't correspond to the numbers we report to shrinkctl). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)