Re: [PATCH 031/262] drm/i915: Report all objects with allocated pages to the shrinker

[Matthew Auld <matthew.william.auld@gmail.com>]

On 17 May 2018 at 07:03, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Currently, we try to report to the shrinker the precise number of
> objects (pages) that are available to be reaped at this moment. This
> requires searching all objects with allocated pages to see if they
> fulfill the search criteria, and this count is performed quite
> frequently. (The shrinker tries to free ~128 pages on each invocation,
> before which we count all the objects; counting takes longer than
> unbinding the objects!) If we take the pragmatic view that with
> sufficient desire, all objects are eventually reapable (they become
> inactive, or no longer used as framebuffer etc), we can simply return
> the count of pinned pages maintained during get_pages/put_pages rather
> than walk the lists every time.
>
> The downside is that we may (slightly) over-report the number of
> objects/pages we could shrink and so penalize ourselves by shrinking
> more than required. This is mitigated by keeping the order in which we
> shrink objects such that we avoid penalizing active and frequently used
> objects, and if memory is so tight that we need to free them we would
> need to anyway.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c      |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h          |  1 -
>  drivers/gpu/drm/i915/i915_gem.c          | 27 ++++-------------------
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++-------------------
>  4 files changed, 11 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index ee8e2ff2c426..72d2238755db 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -434,7 +434,7 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
>         if (ret)
>                 return ret;
>
> -       seq_printf(m, "%u objects, %llu bytes\n",
> +       seq_printf(m, "%u active objects, %llu bytes\n",
>                    dev_priv->mm.object_count,
>                    dev_priv->mm.object_memory);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1eeee043e164..7fa727e62d6f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -986,7 +986,6 @@ struct i915_gem_mm {
>         uint32_t bit_6_swizzle_y;
>
>         /* accounting, useful for userland debugging */
> -       spinlock_t object_stat_lock;
>         u64 object_memory;
>         u32 object_count;
>  };
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4e480874563f..a5694b0a7e6a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -77,25 +77,6 @@ remove_mappable_node(struct drm_mm_node *node)
>         drm_mm_remove_node(node);
>  }
>
> -/* some bookkeeping */
> -static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv,
> -                                 u64 size)
> -{
> -       spin_lock(&dev_priv->mm.object_stat_lock);
> -       dev_priv->mm.object_count++;
> -       dev_priv->mm.object_memory += size;
> -       spin_unlock(&dev_priv->mm.object_stat_lock);
> -}
> -
> -static void i915_gem_info_remove_obj(struct drm_i915_private *dev_priv,
> -                                    u64 size)
> -{
> -       spin_lock(&dev_priv->mm.object_stat_lock);
> -       dev_priv->mm.object_count--;
> -       dev_priv->mm.object_memory -= size;
> -       spin_unlock(&dev_priv->mm.object_stat_lock);
> -}
> -
>  static int
>  i915_gem_wait_for_error(struct i915_gpu_error *error)
>  {
> @@ -2422,6 +2403,8 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
>
>         spin_lock(&i915->mm.obj_lock);
>         list_del(&obj->mm.link);
> +       i915->mm.object_count--;
> +       i915->mm.object_memory -= obj->base.size;
>         spin_unlock(&i915->mm.obj_lock);
>
>         if (obj->mm.mapping) {
> @@ -2708,6 +2691,8 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
>         GEM_BUG_ON(!HAS_PAGE_SIZES(i915, obj->mm.page_sizes.sg));
>
>         spin_lock(&i915->mm.obj_lock);
> +       i915->mm.object_count++;
> +       i915->mm.object_memory += obj->base.size;

Is it not worthwhile keeping the i915_gem_object_is_shrinkable() check?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx