Re: [RFC PATCH 05/42] drm/i915/region: support basic eviction

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Matthew Auld (2019-02-14 14:57:03)
> Support basic eviction for regions.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h               |  2 +
>  drivers/gpu/drm/i915/i915_gem.c               | 16 ++++
>  drivers/gpu/drm/i915/i915_gem_object.h        |  7 ++
>  drivers/gpu/drm/i915/i915_gem_shrinker.c      | 59 ++++++++++++++
>  drivers/gpu/drm/i915/intel_memory_region.c    | 40 +++++++++-
>  drivers/gpu/drm/i915/intel_memory_region.h    |  7 ++
>  .../drm/i915/selftests/intel_memory_region.c  | 76 +++++++++++++++++++
>  drivers/gpu/drm/i915/selftests/mock_region.c  |  1 +
>  8 files changed, 204 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0bea7d889284..3df27769b978 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3196,6 +3196,8 @@ void i915_gem_shrinker_register(struct drm_i915_private *i915);
>  void i915_gem_shrinker_unregister(struct drm_i915_private *i915);
>  void i915_gem_shrinker_taints_mutex(struct drm_i915_private *i915,
>                                     struct mutex *mutex);
> +int i915_gem_shrink_memory_region(struct intel_memory_region *mem,
> +                                 resource_size_t target);
>  
>  /* i915_gem_tiling.c */
>  static inline bool i915_gem_object_needs_bit17_swizzle(struct drm_i915_gem_object *obj)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 92768ab294a4..7f044b643a75 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4095,6 +4095,22 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
>             !i915_gem_object_has_pages(obj))
>                 i915_gem_object_truncate(obj);
>  
> +       if (obj->memory_region) {
> +               mutex_lock(&obj->memory_region->obj_lock);
> +
> +               switch (obj->mm.madv) {
> +               case I915_MADV_WILLNEED:
> +                       list_move(&obj->region_link, &obj->memory_region->objects);
> +                       break;
> +               default:
> +                       list_move(&obj->region_link,
> +                                 &obj->memory_region->purgeable);
> +                       break;
> +               }
> +
> +               mutex_unlock(&obj->memory_region->obj_lock);
> +       }
> +
>         args->retained = obj->mm.madv != __I915_MADV_PURGED;
>         mutex_unlock(&obj->mm.lock);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
> index ac52f61e8ad1..76947a6f49f1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> @@ -95,6 +95,13 @@ struct drm_i915_gem_object {
>          * List of memory region blocks allocated for this object.
>          */
>         struct list_head blocks;
> +       /**
> +        * Element within memory_region->objects or memory_region->purgeable if
> +        * the object is marked as DONTNEED. Access is protected by
> +        * memory_region->obj_lock.

Lies. ;-p

> +        */
> +       struct list_head region_link;
> +       struct list_head tmp_link;
>  
>         struct {
>                 /**
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index 6da795c7e62e..713c6c93cf30 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -308,6 +308,65 @@ unsigned long i915_gem_shrink_all(struct drm_i915_private *i915)
>         return freed;
>  }
>  
> +int i915_gem_shrink_memory_region(struct intel_memory_region *mem,
> +                                 resource_size_t target)

If it's not going to be coupled into the mm.shrinker callback, do not put
it here! And there's no reason why we would ever couple local memory to
the generic mm shrinker!

> +{
> +       struct drm_i915_private *i915 = mem->i915;
> +       struct drm_i915_gem_object *obj, *on;
> +       resource_size_t found;
> +       LIST_HEAD(purgeable);
> +       bool unlock;
> +       int err;
> +
> +       if (!shrinker_lock(i915, 0, &unlock))
> +               return 0;

Don't...

> +
> +       i915_retire_requests(i915);

And this, don't do this.

> +       err = 0;
> +       found = 0;
> +
> +       mutex_lock(&mem->obj_lock);

That's all the top-level locking we should ever need.

> +       list_for_each_entry(obj, &mem->purgeable, region_link) {
> +               if (!i915_gem_object_has_pages(obj))
> +                       continue;
> +
> +               if (READ_ONCE(obj->pin_global))
> +                       continue;
> +
> +               if (atomic_read(&obj->mm.pages_pin_count) > obj->bind_count)
> +                       continue;
> +
> +               list_add(&obj->tmp_link, &purgeable);

Oh crikey.

> +
> +               found += obj->base.size;
> +               if (found >= target)
> +                       goto found;
> +       }
> +
> +       err = -ENOSPC;
> +found:
> +       mutex_unlock(&mem->obj_lock);
> +
> +       list_for_each_entry_safe(obj, on, &purgeable, tmp_link) {
> +               if (!err)
> +                       err = i915_gem_object_unbind(obj);

I would advise not to worry about unbinding until you have it decoupled
from struct_mutex. Or at least defer struct_mutex until you truly need
it to access the vma, so that it doesn't get abused for anything else.

> +               if (!err) {
> +                       __i915_gem_object_put_pages(obj,
> +                                                   I915_MM_SHRINKER);
> +                       if (!i915_gem_object_has_pages(obj))
> +                               obj->mm.madv = __I915_MADV_PURGED;

But this is racy.

> +               }
> +
> +               list_del(&obj->tmp_link);

I'm still going crikey. That's no excuse to invoke struct_mutex.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx