Re: [RFC v5 00/17] DRM cgroup controller with scheduling control and memory stats

[Tvrtko Ursulin]

Hi,

On 19/07/2023 21:31, T.J. Mercier wrote:
> On Wed, Jul 12, 2023 at 4:47 AM Tvrtko Ursulin
> <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> >    drm.memory.stat
> >          A nested file containing cumulative memory statistics for the whole
> >          sub-hierarchy, broken down into separate GPUs and separate memory
> >          regions supported by the latter.
> >
> >          For example::
> >
> >            $ cat drm.memory.stat
> >            card0 region=system total=12898304 shared=0 active=0 resident=12111872 purgeable=167936
> >            card0 region=stolen-system total=0 shared=0 active=0 resident=0 purgeable=0
> >
> >          Card designation corresponds to the DRM device names and multiple line
> >          entries can be present per card.
> >
> >          Memory region names should be expected to be driver specific with the
> >          exception of 'system' which is standardised and applicable for GPUs
> >          which can operate on system memory buffers.
> >
> >          Sub-keys 'resident' and 'purgeable' are optional.
> >
> >          Per category region usage is reported in bytes.
> >
> >   * Feedback from people interested in drm.active_us and drm.memory.stat is
> >     required to understand the use cases and their usefulness (of the fields).
> >
> >     Memory stats are something which was easy to add to my series, since I was
> >     already working on the fdinfo memory stats patches, but the question is how
> >     useful it is.
> Hi Tvrtko,
>
> I think this style of driver-defined categories for reporting of
> memory could potentially allow us to eliminate the GPU memory tracking
> tracepoint used on Android (gpu_mem_total). This would involve reading
> drm.memory.stat at the root cgroup (I see it's currently disabled on

I can put it available under root too, don't think there is any 
technical reason to not have it. In fact, now that I look at it again, 
memory.stat is present on root so that would align with my general 
guideline to keep the two as similar as possible.

> the root), which means traversing the whole cgroup tree under the
> cgroup lock to generate the values on-demand. This would be done
> rarely, but I still wonder what the cost of that would turn out to be.

Yeah that's ugly. I could eliminate cgroup_lock by being a bit smarter. 
Just didn't think it worth it for the RFC.

Basically to account memory stats for any sub-tree I need the equivalent 
one struct drm_memory_stats per DRM device present in the hierarchy. So 
I could pre-allocate a few and restart if run out of spares, or 
something. They are really small so pre-allocating a good number, based 
on past state or something, should would good enough. Or even total 
number of DRM devices in a system as a pessimistic and safe option for 
most reasonable deployments.

> The drm_memory_stats categories in the output don't seem like a big
> value-add for this use-case, but no real objection to them being

You mean the fact there are different categories is not a value add for 
your use case because you would only use one?

The idea was to align 1:1 with DRM memory stats fdinfo and somewhat 
emulate how memory.stat also offers a breakdown.

> there. I know it's called the DRM cgroup controller, but it'd be nice
> if there were a way to make the mem tracking part work for any driver
> that wishes to participate as many of our devices don't use a DRM
> driver. But making that work doesn't look like it would fit very

Ah that would be a challenge indeed to which I don't have any answers 
right now.

Hm if you have a DRM device somewhere in the chain memory stats would 
still show up. Like if you had a dma-buf producer which is not a DRM 
driver, but then that buffer was imported by a DRM driver, it would show 
up in a cgroup. Or vice-versa. But if there aren't any in the whole 
chain then it would not.

> cleanly into this controller, so I'll just shut up now.

Not all all, good feedback!

Regards,

Tvrtko