[RFC 0/2] Introduce a sysfs interface for lmem information

[Tvrtko Ursulin]

On 20/05/2025 16:01, Joonas Lahtinen wrote:
> (+ Tvrtko, Rodrigo and Jani)
> 
> Quoting Krzysztof Niemiec (2025-05-19 18:34:14)
>> Hi,
>>
>> This series introduces a way for applications to read local memory
>> information via files in the sysfs. So far the only way to do this was
>> via i915_query ioctl. This is slightly less handy than sysfs for
>> external users. Additionally, the ioctl has a capability check which
>> limits which users of a system might use it to get information.
>>
>> The goals of this series are:
>>
>>          1) Introduce a simpler interface to access lmem information,
>>          2) Lift the CAP_PERFMON check on that information, OR provide
>>             the administrator with a way to optionally lift it.
>>
>> The first patch introduces the general mechanism without protections.
>> This will effectively lift a capability check on obtaining the memory
>> information. The second patch introduces that check back inside the
>> _show() functions, but also adds a sysctl parameter allowing to override
>> the checks, if an administrator so decides.
>>
>> I'm sending this as RFC because I have a feeling that there's no
>> consensus whether memory information exposed in the patch should be
>> protected or not. Showing it to any user is strictly speaking an info
>> leak, but the severity thereof might be considered not that high, so I'd
>> rather leave it up to discussion first.
>>
>> If we decide for lifting the check, the first patch is sufficient.
> 
> Nack on that.
> 
> CPU memory footprint and GPU memory footprint have a very different
> nature. This was discussed to quite a length, please refer to mailing
> list archives.
> 
>> If we
>> decide against it, the second patch protects the information by default,
>> but with a way to expose it as a conscious decision of the admin. I find
>> it a decent compromise.
> 
> No need for the added complexity if we were to add a sysfs.
> 
> If a sysfs is added, it can be made root readable by default but system
> admin is free to chown or chmod the file for more relaxed access. Back
> in the original discussion time, this was omitted for lack of users.

Agreed, no need to complicate with redundant access controls in the kernel.

> Even now, userspace/sysadmin could already essentially use setuid helper
> process that will only report the memory statistics.
> 
> So I'm not really fully convinced this is needed at all.
> 
> And if it is to be added for the convenience of usersppace, it should
> probably then be considered to be a standard interface across DRM drivers
> ala fdinfo or cgroups.

Cgroup visibility for device memory exists in principle although i915 
hasn't been wired up to it.

For system memory (integrated GPUs) there is some work in progress 
around memcg accounting, although I am unsure if i915 would be 
automatically covered or not.

Also going a step back, from MangoHUDs point of view, I don't really see 
why total GPU memory is very interesting? I would have thought it is 
more interesting to know how much the monitored client is using, which 
is already available via fdinfo. Total memory sounds like something 
which it could leave to interpretation by the entity looking at the 
traces. (If the monitored client is running alone then total_free =~ 
total - client, and if it isn't running alone then it doesn't matter, no?)

Regards,

Tvrtko

>> This change has been requested in these parallel issues for i915 and Xe:
>>
>> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14153
>> https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/4861
>>
>> Thanks
>> Krzysztof
>>
>> Krzysztof Niemiec (2):
>>    drm/i915: Expose local memory information via sysfs
>>    drm/i915: Add protections to sysfs local memory information
>>
>>   drivers/gpu/drm/i915/i915_sysfs.c          |   6 +
>>   drivers/gpu/drm/i915/intel_memory_region.c | 136 +++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_memory_region.h |   3 +
>>   3 files changed, 145 insertions(+)
>>
>> -- 
>> 2.45.2
>> _