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);