Re: [Intel-gfx] [PATCH v5 11/13] drm/i915/ttm: make evicted shmem pages visible to the shrinker

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Mon, 2021-09-27 at 12:41 +0100, Matthew Auld wrote:
> We currently just evict lmem objects to system memory when under
> memory
> pressure, and in the next patch we want to use the shmem backend even
> for this case. For this case we lack the usual object mm.pages, which
> effectively hides the pages from the i915-gem shrinker, until we
> actually "attach" the TT to the object, or in the case of lmem-only
> objects it just gets migrated back to lmem when touched again.
> 
> For all cases we can just adjust the i915 shrinker LRU each time we
> also
> adjust the TTM LRU. The two cases we care about are:
> 
>   1) When something is moved by TTM, including when initially
> populating
>      an object. Importantly this covers the case where TTM moves
> something from
>      lmem <-> smem, outside of the normal get_pages() interface,
> which
>      should still ensure the shmem pages underneath are reclaimable.
> 
>   2) When calling into i915_gem_object_unlock(). The unlock should
>      ensure the object is removed from the shinker LRU, if it was
> indeed
>      swapped out, or just purged, when the shrinker drops the object
> lock.
> 
> We can optimise this(if needed) by tracking if the object is already
> visible to the shrinker(protected by the object lock), so we don't
> touch
> the shrinker LRU more than needed.
> 
> v2(Thomas)
>   - Handle managing the shrinker LRU in adjust_lru, where it is
> always
>     safe to touch the object.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_object.h   |  1 +
>  drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 29 +++++++++++++++---
> --
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c      | 28 +++++++++++++++---
> -
>  3 files changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index 1c9a1d8d3434..640dfbf1f01e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -523,6 +523,7 @@ i915_gem_object_pin_to_display_plane(struct
> drm_i915_gem_object *obj,
>  
>  void i915_gem_object_make_unshrinkable(struct drm_i915_gem_object
> *obj);
>  void i915_gem_object_make_shrinkable(struct drm_i915_gem_object
> *obj);
> +void __i915_gem_object_make_shrinkable(struct drm_i915_gem_object
> *obj);
>  void i915_gem_object_make_purgeable(struct drm_i915_gem_object
> *obj);
>  
>  static inline bool cpu_write_needs_clflush(struct
> drm_i915_gem_object *obj)
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> index 0440696f786a..4b6b2bb6f180 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> @@ -487,13 +487,12 @@ void i915_gem_object_make_unshrinkable(struct
> drm_i915_gem_object *obj)
>         spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
>  }
>  
> -static void __i915_gem_object_make_shrinkable(struct
> drm_i915_gem_object *obj,
> -                                             struct list_head *head)
> +static void ___i915_gem_object_make_shrinkable(struct
> drm_i915_gem_object *obj,
> +                                              struct list_head
> *head)
>  {
>         struct drm_i915_private *i915 = obj_to_i915(obj);
>         unsigned long flags;
>  
> -       GEM_BUG_ON(!i915_gem_object_has_pages(obj));
>         if (!i915_gem_object_is_shrinkable(obj))
>                 return;
>  
> @@ -512,6 +511,21 @@ static void
> __i915_gem_object_make_shrinkable(struct drm_i915_gem_object *obj,
>         spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
>  }
>  
> +/**
> + * __i915_gem_object_make_shrinkable - Move the object to the tail
> of the
> + * shrinkable list. Objects on this list might be swapped out. Used
> with
> + * WILLNEED objects.
> + * @obj: The GEM object.
> + *
> + * DO NOT USE. This is intended to be called on very special objects
> that don't
> + * yet have mm.pages, but are guaranteed to have potentially
> reclaimable pages
> + * underneath.
> + */
> +void __i915_gem_object_make_shrinkable(struct drm_i915_gem_object
> *obj)
> +{
> +       ___i915_gem_object_make_shrinkable(obj,
> +                                          &obj_to_i915(obj)-
> >mm.shrink_list);
> +}
>  
>  /**
>   * i915_gem_object_make_shrinkable - Move the object to the tail of
> the
> @@ -523,8 +537,8 @@ static void
> __i915_gem_object_make_shrinkable(struct drm_i915_gem_object *obj,
>   */
>  void i915_gem_object_make_shrinkable(struct drm_i915_gem_object
> *obj)
>  {
> -       __i915_gem_object_make_shrinkable(obj,
> -                                         &obj_to_i915(obj)-
> >mm.shrink_list);
> +       GEM_BUG_ON(!i915_gem_object_has_pages(obj));
> +       __i915_gem_object_make_shrinkable(obj);
>  }
>  
>  /**
> @@ -538,6 +552,7 @@ void i915_gem_object_make_shrinkable(struct
> drm_i915_gem_object *obj)
>   */
>  void i915_gem_object_make_purgeable(struct drm_i915_gem_object *obj)
>  {
> -       __i915_gem_object_make_shrinkable(obj,
> -                                         &obj_to_i915(obj)-
> >mm.purge_list);
> +       GEM_BUG_ON(!i915_gem_object_has_pages(obj));
> +       ___i915_gem_object_make_shrinkable(obj,
> +                                          &obj_to_i915(obj)-
> >mm.purge_list);
>  }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index c7402995a8f9..194e5f1deda8 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -749,6 +749,8 @@ static int i915_ttm_move(struct ttm_buffer_object
> *bo, bool evict,
>                         return ret;
>         }
>  
> +       i915_ttm_adjust_lru(obj);
> +

This will put the object on the shrinker list a little earlier than if
we rely on the adjust_lru() from object_unlock() only, but is that
strictly necessary? I figure even if the shrinker picks the object up,
it will fail in the object trylock and ignore the object, until we call
object_unlock() anyway?


>         dst_st = i915_ttm_resource_get_st(obj, dst_mem);
>         if (IS_ERR(dst_st))
>                 return PTR_ERR(dst_st);
> @@ -856,7 +858,6 @@ static int __i915_ttm_get_pages(struct
> drm_i915_gem_object *obj,
>                         return i915_ttm_err_to_gem(ret);
>         }
>  
> -       i915_ttm_adjust_lru(obj);
>         if (bo->ttm && !ttm_tt_is_populated(bo->ttm)) {
>                 ret = ttm_tt_populate(bo->bdev, bo->ttm, &ctx);
>                 if (ret)
> @@ -876,10 +877,10 @@ static int __i915_ttm_get_pages(struct
> drm_i915_gem_object *obj,
>                         return PTR_ERR(st);
>  
>                 __i915_gem_object_set_pages(obj, st,
> i915_sg_dma_sizes(st->sgl));
> -               if (!bo->ttm || !i915_tt->is_shmem)
> -                       i915_gem_object_make_unshrinkable(obj);
>         }
>  
> +       i915_ttm_adjust_lru(obj);
> +
>         return ret;
>  }
>  
> @@ -950,8 +951,6 @@ static void i915_ttm_put_pages(struct
> drm_i915_gem_object *obj,
>          * If the object is not destroyed next, The TTM eviction
> logic
>          * and shrinkers will move it out if needed.
>          */
> -
> -       i915_ttm_adjust_lru(obj);
>  }
>  
>  static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj)
> @@ -967,6 +966,17 @@ static void i915_ttm_adjust_lru(struct
> drm_i915_gem_object *obj)
>         if (!kref_read(&bo->kref))
>                 return;
>  
> +       /*
> +        * Even if we lack mm.pages for this object(which will be the
> case when
> +        * something is evicted to system memory by TTM), we still
> want to make
> +        * this object visible to the shrinker, since the underlying
> ttm_tt
> +        * still has the real shmem pages.
> +        */
> +       if (bo->ttm && i915_tt->filp && ttm_tt_is_populated(bo->ttm))
> +               __i915_gem_object_make_shrinkable(obj);
> +       else
> +               i915_gem_object_make_unshrinkable(obj);
> +
>         /*
>          * Put on the correct LRU list depending on the MADV status
>          */
> @@ -1006,6 +1016,14 @@ static void i915_ttm_adjust_lru(struct
> drm_i915_gem_object *obj)
>  static void i915_ttm_delayed_free(struct drm_i915_gem_object *obj)
>  {
>         if (obj->ttm.created) {
> +               /*
> +                * We freely manage the shrinker LRU outide of the
> mm.pages life
> +                * cycle. As a result when destroying the object it's
> up to us
> +                * to ensure we remove it from the LRU, before we
> free the
> +                * object.
> +                */
> +               i915_gem_object_make_unshrinkable(obj);
> +

I guess this is not *strictly* necessary at this point, since the
shrinker has a kref_get_unless_zero() guard, but I guess we need to
remove the object from the shrinker LRU at some point during
destruction anyway.

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>


>                 ttm_bo_put(i915_gem_to_ttm(obj));
>         } else {
>                 __i915_gem_free_object(obj);