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@xxxxxxxxx>
> Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> ttm_bo_put(i915_gem_to_ttm(obj));
> } else {
> __i915_gem_free_object(obj);
