Re: [Intel-gfx] [PATCH v9 2/8] drm/i915/ttm: add tt shmem backend

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

On 07/12/2021 14:05, Matthew Auld wrote:
> On 07/12/2021 13:10, Tvrtko Ursulin wrote:
>>
>> On 18/10/2021 10:10, Matthew Auld wrote:
>>> For cached objects we can allocate our pages directly in shmem. This
>>> should make it possible(in a later patch) to utilise the existing
>>> i915-gem shrinker code for such objects. For now this is still disabled.
>>>
>>> v2(Thomas):
>>>    - Add optional try_to_writeback hook for objects. Importantly we need
>>>      to check if the object is even still shrinkable; in between us
>>>      dropping the shrinker LRU lock and acquiring the object lock it 
>>> could for
>>>      example have been moved. Also we need to differentiate between
>>>      "lazy" shrinking and the immediate writeback mode. Also later we 
>>> need to
>>>      handle objects which don't even have mm.pages, so bundling this 
>>> into
>>>      put_pages() would require somehow handling that edge case, hence
>>>      just letting the ttm backend handle everything in try_to_writeback
>>>      doesn't seem too bad.
>>> v3(Thomas):
>>>    - Likely a bad idea to touch the object from the unpopulate hook,
>>>      since it's not possible to hold a reference, without also creating
>>>      circular dependency, so likely this is too fragile. For now just
>>>      ensure we at least mark the pages as dirty/accessed when called 
>>> from the
>>>      shrinker on WILLNEED objects.
>>>    - s/try_to_writeback/shrinker_release_pages, since this can do more
>>>      than just writeback.
>>>    - Get rid of do_backup boolean and just set the SWAPPED flag prior to
>>>      calling unpopulate.
>>>    - Keep shmem_tt as lowest priority for the TTM LRU bo_swapout 
>>> walk, since
>>>      these just get skipped anyway. We can try to come up with something
>>>      better later.
>>> v4(Thomas):
>>>    - s/PCI_DMA/DMA/. Also drop NO_KERNEL_MAPPING and NO_WARN, which
>>>      apparently doesn't do anything with streaming mappings.
>>>    - Just pass along the error for ->truncate, and assume nothing.
>>>
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Oak Zeng <oak.zeng@intel.com>
>>> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> Acked-by: Oak Zeng <oak.zeng@intel.com>
>>
>> [snip]
>>
>>> -static void try_to_writeback(struct drm_i915_gem_object *obj,
>>> -                 unsigned int flags)
>>> +static int try_to_writeback(struct drm_i915_gem_object *obj, 
>>> unsigned int flags)
>>>   {
>>> +    if (obj->ops->shrinker_release_pages)
>>> +        return obj->ops->shrinker_release_pages(obj,
>>> +                            flags & I915_SHRINK_WRITEBACK);
>>
>> I have a high level question about how does this new vfunc fit with 
>> the existing code.
>>
>> Because I notice in the implementation 
>> (i915_ttm_shrinker_release_pages) it ends up doing:
>> ...
>>
>>         switch (obj->mm.madv) {
>>         case I915_MADV_DONTNEED:
>>                 return i915_ttm_purge(obj);
>>         case __I915_MADV_PURGED:
>>                 return 0;
>>         }
>>
>> ... and then...
>>
>>         if (should_writeback)
>>                 __shmem_writeback(obj->base.size, 
>> i915_tt->filp->f_mapping);
>>
>> Which on a glance looks like a possible conceptual duplication of the 
>> concepts in this very function (try_to_writeback):
>>
>>> +
>>>       switch (obj->mm.madv) {
>>>       case I915_MADV_DONTNEED:
>>>           i915_gem_object_truncate(obj);
>>> -        return;
>>> +        return 0;
>>>       case __I915_MADV_PURGED:
>>> -        return;
>>> +        return 0;
>>>       }
>>>       if (flags & I915_SHRINK_WRITEBACK)
>>>           i915_gem_object_writeback(obj);
>>
>> So question is this the final design or some futher tidy is 
>> possible/planned?
> 
> It seems ok to me, all things considered. The TTM version needs to check 
> if the object is still shrinkable at the start(plus some other stuff), 
> upon acquiring the object lock. If that succeeds we can do the above 
> madv checks and potentially just call truncate. Otherwise we can proceed 
> with shrinking, but again TTM is special here, and we have to call 
> ttm_bo_validate() underneath(we might not even have mm.pages here). And 
> then if that all works we might be able to also perform the writeback, 
> if needed. So I suppose we could add all that directly in 
> try_to_writeback(), and make it conditional for TTM devices, or I guess 
> we need two separate hooks, one to do some pre-checking and another do 
> the actual swap step. Not sure if that is better or worse though.

Would implementing the shrinker_release_pages for all objects work? It 
would contain what currently is in try_to_writeback and so the two 
paths, if not compatible, would diverge cleanly on the same level of 
indirection. I mean we would not have two effectively mutually exclusive 
vfuncs (shrinker_release_pages and writeback) and could unexport 
i915_gem_object_writeback.

>> Background to my question is that I will float a patch which proposes 
>> to remove writeback altogether. There are reports from the fields that 
>> it causes deadlocks and looking at 2d6692e642e7 ("drm/i915: Start 
>> writeback from the shrinker") and its history it seems like it was a 
>> known risk.
>>
>> Apart from the code organisation questions, on the practical level - 
>> do you need writeback from the TTM backend or while I am proposing to 
>> remove it from the "legacy" paths, I can propose removing it from the 
>> TTM flow as well?
> 
> Yeah, if that is somehow busted then we should remove from TTM backend 
> also.

Okay thanks, I wanted to check in case there was an extra need in TTM. I 
will float a patch soon hopefully but testing will be a problem since it 
seems very hard to repro at the moment.

Regards,

Tvrtko