Re: [Intel-gfx] [PATCH] drm/i915: Allow shrinking of userptr objects once again

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 01/11/2016 13:52, Chris Wilson wrote:
> On Tue, Nov 01, 2016 at 01:28:06PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Commit 1bec9b0bda3d ("drm/i915/shrinker: Only shmemfs objects
>> are backed by swap") stopped considering the userptr objects
>> in shrinker callbacks.
>>
>> Restore that so idle userptr objects can be discarded in order
>> to free up memory.
>>
>> One change further to what was introduced in 1bec9b0bda3d is
>> to start considering userptr objects in oom but that should
>> also be a correct thing to do.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Fixes: 1bec9b0bda3d ("drm/i915/shrinker: Only shmemfs objects are backed by swap")
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
>> index 0993afc0e725..dfca1f6b3630 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
>> @@ -83,8 +83,8 @@ static bool can_release_pages(struct drm_i915_gem_object *obj)
>>  	if (!obj->mm.pages)
>>  		return false;
>>
>> -	/* Only shmemfs objects are backed by swap */
>> -	if (!obj->base.filp)
>> +	/* shmemfs and userptr objects are backed by swap */
>> +	if (!obj->base.filp && !obj->userptr.mm)
>
> Hmm. Another sticking point if we want to use the union again.
>
> How about obj->ops->flags & I915_GEM_OBJECT_HAS_BACKING_STORE ?
> Or I915_GEM_OBJECT_CAN_SWAP since we want this for i915_gem_internal.c
> as well, and that techinically doesn't have a backing store but can be
> reaped. Hmm.
>
> if (!(obj->ops->flags & I915_GEM_OBJECT_HAS_BACKING_STORE ||
>       obj->mm.madv == I915_MADV_DONTNEED))

Hm I915_GEM_OBJECT_HAS_STRUCT_PAGE happens to be right - shm, userptr 
and internal. But that would be bad. Neither do I like HAS_BACKING_STORE.

Maybe I915_GEM_OBJECT_IS_SHRINKABLE, fully dumb and explicit?

Regards,

Tvrtko