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

[Thomas Hellström]

On 11/8/21 09:08, Thomas Hellström wrote:
>
> 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...
>
Or actually, Thinking of it, if we don't have GEM pages, even if there 
are CPU PTEs set up, I can't see why we shouldn't purge this object 
immediately. I'll just revert that last hunk.

/Thomas



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