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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
