| Message ID | 1461150294-14123-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ke, 2016-04-20 at 12:04 +0100, Chris Wilson wrote: > Inside the shrinker we call can_release_pages() to indicate whether or > not we can make forward progress in freeing up memory by unbinding that > object. When adding our report to oom, we should be using the same > logic. > > Whilst here, change the reporting from bytes to pages so that it looks > smaller to the user!, is consistent with the neighbouring oom report > itself which displays counts in pages, and makes the unsigned long > overflow less likely. Makes sense as that is the smallest pinning unit, too. > > 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 | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c > index 4e4fcfa76b4c..cb225e039d48 100644 > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c > @@ -336,7 +336,7 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr) > container_of(nb, struct drm_i915_private, mm.oom_notifier); > struct shrinker_lock_uninterruptible slu; > struct drm_i915_gem_object *obj; > - unsigned long pinned, bound, unbound, freed_pages; > + unsigned long unevictable, bound, unbound, freed_pages; > > if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000)) > return NOTIFY_DONE; > @@ -347,33 +347,33 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr) > * assert that there are no objects with pinned pages that are not > * being pointed to by hardware. > */ > - unbound = bound = pinned = 0; > + unbound = bound = unevictable = 0; > list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list) { > if (!obj->base.filp) /* not backed by a freeable object */ > continue; > > - if (obj->pages_pin_count) > - pinned += obj->base.size; > + if (!can_release_pages(obj)) > + unevictable += obj->base.size >> PAGE_SHIFT; > else > - unbound += obj->base.size; > + unbound += obj->base.size >> PAGE_SHIFT; > } > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { > if (!obj->base.filp) > continue; > > - if (obj->pages_pin_count > num_vma_bound(obj)) > - pinned += obj->base.size; > + if (!can_release_pages(obj)) > + unevictable += obj->base.size >> PAGE_SHIFT; > else > - bound += obj->base.size; > + bound += obj->base.size >> PAGE_SHIFT; > } > > i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu); > > if (freed_pages || unbound || bound) > - pr_info("Purging GPU memory, %lu bytes freed, %lu bytes still pinned.\n", > - freed_pages << PAGE_SHIFT, pinned); > + pr_info("Purging GPU memory, %lu pages freed, %lu pages still pinned.\n", > + freed_pages, unevictable); My code checker senses are tingling, this looks very much like oversized line, mind cutting it down? With that addressed, Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > if (unbound || bound) > - pr_err("%lu and %lu bytes still available in the " > + pr_err("%lu and %lu pages still available in the " > "bound and unbound GPU page lists.\n", > bound, unbound); >
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c index 4e4fcfa76b4c..cb225e039d48 100644 --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c @@ -336,7 +336,7 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr) container_of(nb, struct drm_i915_private, mm.oom_notifier); struct shrinker_lock_uninterruptible slu; struct drm_i915_gem_object *obj; - unsigned long pinned, bound, unbound, freed_pages; + unsigned long unevictable, bound, unbound, freed_pages; if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000)) return NOTIFY_DONE; @@ -347,33 +347,33 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr) * assert that there are no objects with pinned pages that are not * being pointed to by hardware. */ - unbound = bound = pinned = 0; + unbound = bound = unevictable = 0; list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list) { if (!obj->base.filp) /* not backed by a freeable object */ continue; - if (obj->pages_pin_count) - pinned += obj->base.size; + if (!can_release_pages(obj)) + unevictable += obj->base.size >> PAGE_SHIFT; else - unbound += obj->base.size; + unbound += obj->base.size >> PAGE_SHIFT; } list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { if (!obj->base.filp) continue; - if (obj->pages_pin_count > num_vma_bound(obj)) - pinned += obj->base.size; + if (!can_release_pages(obj)) + unevictable += obj->base.size >> PAGE_SHIFT; else - bound += obj->base.size; + bound += obj->base.size >> PAGE_SHIFT; } i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu); if (freed_pages || unbound || bound) - pr_info("Purging GPU memory, %lu bytes freed, %lu bytes still pinned.\n", - freed_pages << PAGE_SHIFT, pinned); + pr_info("Purging GPU memory, %lu pages freed, %lu pages still pinned.\n", + freed_pages, unevictable); if (unbound || bound) - pr_err("%lu and %lu bytes still available in the " + pr_err("%lu and %lu pages still available in the " "bound and unbound GPU page lists.\n", bound, unbound);
Inside the shrinker we call can_release_pages() to indicate whether or not we can make forward progress in freeing up memory by unbinding that object. When adding our report to oom, we should be using the same logic. Whilst here, change the reporting from bytes to pages so that it looks smaller to the user!, is consistent with the neighbouring oom report itself which displays counts in pages, and makes the unsigned long overflow less likely. 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 | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)