[Intel-gfx] [PATCH] drm/i915/gem: Fix gem_madvise for ttm+shmem objects

[Thomas Hellström]

On 11/5/21 16:18, Matthew Auld wrote:
> On 05/11/2021 13:03, Thomas Hellström wrote:
>> Gem-TTM objects that are backed by shmem might have populated
>> page-vectors without having the Gem pages set. Those objects
>> aren't moved to the correct shrinker / purge list by the
>> gem_madvise. Furthermore they are purged directly on
>> MADV_DONTNEED rather than waiting for the shrinker to do that.
>>
>> For such objects, identified by having the
>> _SELF_MANAGED_SHRINK_LIST set, make sure they end up on the
>> correct list and defer purging to the shrinker.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index d0e642c82064..da972c8d45b1 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1005,7 +1005,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, 
>> void *data,
>>               obj->ops->adjust_lru(obj);
>>       }
>>   -    if (i915_gem_object_has_pages(obj)) {
>> +    if (i915_gem_object_has_pages(obj) ||
>> +        i915_gem_object_has_self_managed_shrink_list(obj)) {
>
> Makes sense.
>
>>           unsigned long flags;
>>             spin_lock_irqsave(&i915->mm.obj_lock, flags);
>> @@ -1024,7 +1025,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, 
>> void *data,
>>         /* if the object is no longer attached, discard its backing 
>> storage */
>>       if (obj->mm.madv == I915_MADV_DONTNEED &&
>> -        !i915_gem_object_has_pages(obj))
>> +        !i915_gem_object_has_pages(obj) &&
>> +        !i915_gem_object_has_self_managed_shrink_list(obj))
>>           i915_gem_object_truncate(obj);
>
> And it looks like this also matches the workings of lmem, where under 
> memory pressure we also just purge such objects, instead of moving 
> them, making sure to keep them first in the LRU?
>
> One thing is to maybe immediately discard already swapped-out objects 
> here, since the shrinker will be oblivious to them, and they sort of 
> just linger in swap until the object is destroyed?

This might be a bit ugly if we want to avoid exposing even more gem 
object ops.

Could we perhaps for the truncate callback only truncate swapped-out 
objects if we have a self-managed shrinker list? That will match all the 
current call-sites AFAICT since truncate is never called from the 
shrinker  with the self-managed shrinker list...

/Thomas

>
>>         args->retained = obj->mm.madv != __I915_MADV_PURGED;
>>