[PATCH 12/17] ttm: add objcg pointer to bo and tt

[Christian König]

On 01.07.25 10:06, David Airlie wrote:
> On Tue, Jul 1, 2025 at 5:22 PM Christian König <christian.koenig at amd.com> wrote:
>>>>> diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
>>>>> index 15d4019685f6..c13fea4c2915 100644
>>>>> --- a/include/drm/ttm/ttm_tt.h
>>>>> +++ b/include/drm/ttm/ttm_tt.h
>>>>> @@ -126,6 +126,8 @@ struct ttm_tt {
>>>>>       enum ttm_caching caching;
>>>>>       /** @restore: Partial restoration from backup state. TTM private */
>>>>>       struct ttm_pool_tt_restore *restore;
>>>>> +     /** @objcg: Object cgroup for this TT allocation */
>>>>> +     struct obj_cgroup *objcg;
>>>>>  };
>>>>
>>>> We should probably keep that out of the pool and account the memory to the BO instead.
>>>>
>>>
>>> I tried that like 2-3 patch posting iterations ago, you suggested it
>>> then, it didn't work. It has to be done at the pool level, I think it
>>> was due to swap handling.
>>
>> When you do it at the pool level the swap/shrink handling is broken as well, just not for amdgpu.
>>
>> See xe_bo_shrink() and drivers/gpu/drm/xe/xe_shrinker.c on how XE does it.
> 
> I've read all of that, but I don't think it needs changing yet, though
> I do think I probably need to do a bit more work on the ttm
> backup/restore paths to account things, but again we suffer from the
> what happens if your cgroup runs out of space on a restore path,
> similiar to eviction.

My thinking was rather that because of this we do it at the resource level and keep memory accounted to whoever allocated it even if it's backed up or swapped out.

> Blocking the problems we can solve now on the problems we've no idea
> how to solve means nobody gets experience with solving anything.

Well that's exactly the reason why I'm suggesting this. Ignoring swapping/backup for now seems to make things much easier.

>> So the best we can do is to do it at the resource level because that is common for everybody.
>>
>> This doesn't takes swapping on amdgpu into account, but that should not be that relevant since we wanted to remove that and switch to the XE approach anyway.
> 
> I don't understand, we cannot do it at the resource level, I sent
> patches to try, they don't fundamentally work properly, so it isn't
> going to fly. We can solve it at the pool level, so we should, if we
> somehow rearchitect things later to solve it at the resource level,
> but I feel we'd have to make swap handling operate at the resource
> level instead of tt level to have any chance.
> 
> Swapping via the backup/restore paths should be accounted properly,
> since moving pages out to swap one way cgroups can reduce the memory
> usage, if we can't account that swapped pages aren't removed from the
> page count, then it isn't going to work properly.

But that is only possible if you generalize the shrinker XE has implemented for at least amdgpu as well (and make it memcg aware).

That's exactly what I would try to avoid because it means tons of additional driver specific work before we can get at least a foundation for memcg awareness.

Alternatively we could implement a shrinker for the existing swapping path, but I tried that before and Sima, Thomas and I agreed that this is not the right thing todo and rather came up with the XE shrinker.

Christian.

> 
> Dave.
>