Re: [PATCH v2 2/3] drm/i915/shrinker: Report "unevictable" pages

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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);
>  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx