[Intel-gfx] [PATCH v2 03/37] drm/i915/region: support basic eviction

[Chris Wilson]

Quoting Matthew Auld (2019-06-27 21:55:59)
> +int i915_memory_region_evict(struct intel_memory_region *mem,

What type is this again?

> +                            resource_size_t target)
> +{
> +       struct drm_i915_gem_object *obj, *on;
> +       resource_size_t found;
> +       LIST_HEAD(purgeable);
> +       int err;
> +
> +       err = 0;
> +       found = 0;
> +
> +       mutex_lock(&mem->obj_lock);
> +
> +       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->bind_count))
> +                       continue;
> +
> +               list_add(&obj->eviction_link, &purgeable);
> +
> +               found += obj->base.size;
> +               if (found >= target)
> +                       goto found;
> +       }
> +
> +       err = -ENOSPC;
> +found:
> +       list_for_each_entry_safe(obj, on, &purgeable, eviction_link) {
> +               if (!err) {
> +                       __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);

How come put_pages is not taking mm->obj_lock to remove the
obj->region_link?

I'm getting fishy vibes.

> +
> +                       mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);

> +                       if (!i915_gem_object_has_pages(obj))
> +                               obj->mm.madv = __I915_MADV_PURGED;

That should be pushed to put_pages() as reason. The unlock/lock is just
asking for trouble.

> +                       mutex_unlock(&obj->mm.lock);
> +               }
> +
> +               list_del(&obj->eviction_link);
> +       }

You will have noticed that a separate eviction_link is superfluous? If
both region_link and evction_link are only valid underneath obj_lock,
you can list_move(&obj->region_link, &purgeable) in the first pass, and
unwind on error.

However, I'm going hmm.

So you keep all objects on the shrink lists even when not allocated. Ho
hum. With a bit more creative locking, read careful acquisition of
resources then dropping the lock before actually evicting, it should
work out.
-Chris