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

[Krzysztof Niemiec]

Hi,

On 2025-05-20 at 18:01:12 GMT, 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.
> 

This is something I missed, my bad. That is of course the better way to
handle the access control.

> 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.
> 

This is in fact mostly a matter of convenience, as for the usecase under
consideration here (mangohud) it can be assumed that root can be
obtained anyway. It would just be MUCH more convenient for users of the
driver to just chmod the appropriate files and go, instead of setting up
whole other binary just to invoke the ioctl and parse it, and then have
it do IPC with the main binary that can't be run as root. Just way less
hoops to go through. We could just tell userspace to "deal with it", but
then I don't know why can't we just play nice. Is this kind of change
too intrusive for us to warrant the userspace gain?

And it's true that reporting this via sysfs is redundant, but then, for
example, engine information is also available via both sysfs and
i915_query. Is there a particular reason for it to be the case for
engine info that can't be applied to memory info? Access control
notwithstanding, as it can be handled by file permissions.

Thanks
Krzysztof

> 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.
> 
> Regards, Joonas
> 
> > 
> > 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
> > _