Re: [RFC 02/17] drm: Track clients per owning process

[Christian König <christian.koenig@xxxxxxx>]

Am 20.10.22 um 09:34 schrieb Tvrtko Ursulin:
> On 20/10/2022 07:40, Christian König wrote:
> > Am 19.10.22 um 19:32 schrieb Tvrtko Ursulin:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > >
> > > To enable propagation of settings from the cgroup drm controller to 
> > > drm we
> > > need to start tracking which processes own which drm clients.
> > >
> > > Implement that by tracking the struct pid pointer of the owning 
> > > process in
> > > a new XArray, pointing to a structure containing a list of associated
> > > struct drm_file pointers.
> > >
> > > Clients are added and removed under the filelist mutex and RCU list
> > > operations are used below it to allow for lockless lookup.
> >
> > That won't work easily like this. The problem is that file_priv->pid 
> > is usually not accurate these days:
> >
> >  From the debugfs clients file:
> >
> >        systemd-logind   773   0   y    y     0          0
> >                  Xorg  1639 128   n    n  1000          0
> >                  Xorg  1639 128   n    n  1000          0
> >                  Xorg  1639 128   n    n  1000          0
> >               firefox  2945 128   n    n  1000          0
> >                  Xorg  1639 128   n    n  1000          0
> >                  Xorg  1639 128   n    n  1000          0
> >                  Xorg  1639 128   n    n  1000          0
> >                  Xorg  1639 128   n    n  1000          0
> >                chrome 35940 128   n    n  1000          0
> >                chrome 35940   0   n    y  1000          1
> >                chrome 35940   0   n    y  1000          2
> >                  Xorg  1639 128   n    n  1000          0
> >                  Xorg  1639 128   n    n  1000          0
> >                  Xorg  1639 128   n    n  1000          0
> >
> > This is with glxgears and a bunch other OpenGL applications running.
> >
> > The problem is that for most applications the X/Wayland server is now 
> > opening the render node. The only exceptions in this case are apps 
> > using DRI2 (VA-API?).
> >
> > I always wanted to fix this and actually track who is using the file 
> > descriptor instead of who opened it, but never had the time to do this.
>
> There's a patch later in the series which allows client records to be 
> migrated to a new PID, and then i915 patch to do that when fd is used 
> for context create. That approach I think worked well enough in the 
> past. So maybe it could be done in the DRM core at some suitable entry 
> point.

Yeah, that makes some sense. I think you should wire that inside 
drm_ioctl(), as far as I know more or less all uses of a file descriptor 
would go through that function.

And maybe make that a stand alone patch, cause that can go upstream as a 
bug fix independently if you ask me.

> > I think you need to fix this problem first. And BTW: and unsigned 
> > long doesn't work as PID either with containers.
>
> This I am not familiar with so would like to hear more if you could 
> point me in the right direction at least.

Uff, I'm the wrong person to ask stuff like that. I just can say from 
experience because I've ran into that trap as well.

> My assumption was that struct pid *, which is what I store in unsigned 
> long, would be unique in a system where there is a single kernel 
> running, so as long as lifetimes are correct (released from tracking 
> here when fd is closed, which is implicit on process exit) would work. 
> You are suggesting that is not so?

I think you should have the pointer to struct pid directly here since 
that is a reference counted structure IIRC. But don't ask me what the 
semantics is how to get or put a reference.

Regards,
Christian.

> Regards,
>
> Tvrtko
>
> > Regards,
> > Christian.
> >
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/Makefile     |  1 +
> > >   drivers/gpu/drm/drm_cgroup.c | 60 
> > > ++++++++++++++++++++++++++++++++++++
> > >   drivers/gpu/drm/drm_file.c   | 18 ++++++++---
> > >   include/drm/drm_clients.h    | 31 +++++++++++++++++++
> > >   include/drm/drm_file.h       |  4 +++
> > >   5 files changed, 110 insertions(+), 4 deletions(-)
> > >   create mode 100644 drivers/gpu/drm/drm_cgroup.c
> > >   create mode 100644 include/drm/drm_clients.h
> > >
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 6e55c47288e4..0719970d17ee 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -59,6 +59,7 @@ drm-$(CONFIG_DRM_LEGACY) += \
> > >       drm_scatter.o \
> > >       drm_vm.o
> > >   drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > > +drm-$(CONFIG_CGROUP_DRM) += drm_cgroup.o
> > >   drm-$(CONFIG_COMPAT) += drm_ioc32.o
> > >   drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> > >   drm-$(CONFIG_OF) += drm_of.o
> > > diff --git a/drivers/gpu/drm/drm_cgroup.c 
> > > b/drivers/gpu/drm/drm_cgroup.c
> > > new file mode 100644
> > > index 000000000000..a31ff1d593ab
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_cgroup.c
> > > @@ -0,0 +1,60 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright © 2022 Intel Corporation
> > > + */
> > > +
> > > +#include <drm/drm_drv.h>
> > > +#include <drm/drm_clients.h>
> > > +
> > > +static DEFINE_XARRAY(drm_pid_clients);
> > > +
> > > +void drm_clients_close(struct drm_file *file_priv)
> > > +{
> > > +    unsigned long pid = (unsigned long)file_priv->pid;
> > > +    struct drm_device *dev = file_priv->minor->dev;
> > > +    struct drm_pid_clients *clients;
> > > +
> > > +    lockdep_assert_held(&dev->filelist_mutex);
> > > +
> > > +    clients = xa_load(&drm_pid_clients, pid);
> > > +    list_del_rcu(&file_priv->clink);
> > > +    if (atomic_dec_and_test(&clients->num)) {
> > > +        xa_erase(&drm_pid_clients, pid);
> > > +        kfree_rcu(clients, rcu);
> > > +    }
> > > +}
> > > +
> > > +int drm_clients_open(struct drm_file *file_priv)
> > > +{
> > > +    unsigned long pid = (unsigned long)file_priv->pid;
> > > +    struct drm_device *dev = file_priv->minor->dev;
> > > +    struct drm_pid_clients *clients;
> > > +    bool new_client = false;
> > > +
> > > +    lockdep_assert_held(&dev->filelist_mutex);
> > > +
> > > +    clients = xa_load(&drm_pid_clients, pid);
> > > +    if (!clients) {
> > > +        clients = kmalloc(sizeof(*clients), GFP_KERNEL);
> > > +        if (!clients)
> > > +            return -ENOMEM;
> > > +        atomic_set(&clients->num, 0);
> > > +        INIT_LIST_HEAD(&clients->file_list);
> > > +        init_rcu_head(&clients->rcu);
> > > +        new_client = true;
> > > +    }
> > > +    atomic_inc(&clients->num);
> > > +    list_add_tail_rcu(&file_priv->clink, &clients->file_list);
> > > +    if (new_client) {
> > > +        void *xret;
> > > +
> > > +        xret = xa_store(&drm_pid_clients, pid, clients, GFP_KERNEL);
> > > +        if (xa_err(xret)) {
> > > +            list_del_init(&file_priv->clink);
> > > +            kfree(clients);
> > > +            return PTR_ERR(clients);
> > > +        }
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > > index a8b4d918e9a3..ce58d5c513db 100644
> > > --- a/drivers/gpu/drm/drm_file.c
> > > +++ b/drivers/gpu/drm/drm_file.c
> > > @@ -40,6 +40,7 @@
> > >   #include <linux/slab.h>
> > >   #include <drm/drm_client.h>
> > > +#include <drm/drm_clients.h>
> > >   #include <drm/drm_drv.h>
> > >   #include <drm/drm_file.h>
> > >   #include <drm/drm_print.h>
> > > @@ -298,6 +299,7 @@ static void drm_close_helper(struct file *filp)
> > >       mutex_lock(&dev->filelist_mutex);
> > >       list_del(&file_priv->lhead);
> > > +    drm_clients_close(file_priv);
> > >       mutex_unlock(&dev->filelist_mutex);
> > >       drm_file_free(file_priv);
> > > @@ -349,10 +351,8 @@ static int drm_open_helper(struct file *filp, 
> > > struct drm_minor *minor)
> > >       if (drm_is_primary_client(priv)) {
> > >           ret = drm_master_open(priv);
> > > -        if (ret) {
> > > -            drm_file_free(priv);
> > > -            return ret;
> > > -        }
> > > +        if (ret)
> > > +            goto err_free;
> > >       }
> > >       filp->private_data = priv;
> > > @@ -360,6 +360,9 @@ static int drm_open_helper(struct file *filp, 
> > > struct drm_minor *minor)
> > >       priv->filp = filp;
> > >       mutex_lock(&dev->filelist_mutex);
> > > +    ret = drm_clients_open(priv);
> > > +    if (ret)
> > > +        goto err_unlock;
> > >       list_add(&priv->lhead, &dev->filelist);
> > >       mutex_unlock(&dev->filelist_mutex);
> > > @@ -387,6 +390,13 @@ static int drm_open_helper(struct file *filp, 
> > > struct drm_minor *minor)
> > >   #endif
> > >       return 0;
> > > +
> > > +err_unlock:
> > > +    mutex_unlock(&dev->filelist_mutex);
> > > +err_free:
> > > +    drm_file_free(priv);
> > > +
> > > +    return ret;
> > >   }
> > >   /**
> > > diff --git a/include/drm/drm_clients.h b/include/drm/drm_clients.h
> > > new file mode 100644
> > > index 000000000000..4ae553a03d1e
> > > --- /dev/null
> > > +++ b/include/drm/drm_clients.h
> > > @@ -0,0 +1,31 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright © 2022 Intel Corporation
> > > + */
> > > +
> > > +#ifndef _DRM_CLIENTS_H_
> > > +#define _DRM_CLIENTS_H_
> > > +
> > > +#include <drm/drm_file.h>
> > > +
> > > +struct drm_pid_clients {
> > > +    atomic_t num;
> > > +    struct list_head file_list;
> > > +    struct rcu_head rcu;
> > > +};
> > > +
> > > +#if IS_ENABLED(CONFIG_CGROUP_DRM)
> > > +void drm_clients_close(struct drm_file *file_priv);
> > > +int drm_clients_open(struct drm_file *file_priv);
> > > +#else
> > > +static inline void drm_clients_close(struct drm_file *file_priv)
> > > +{
> > > +}
> > > +
> > > +static inline int drm_clients_open(struct drm_file *file_priv)
> > > +{
> > > +    return 0;
> > > +}
> > > +#endif
> > > +
> > > +#endif
> > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > > index d780fd151789..0965eb111f24 100644
> > > --- a/include/drm/drm_file.h
> > > +++ b/include/drm/drm_file.h
> > > @@ -268,6 +268,10 @@ struct drm_file {
> > >       /** @minor: &struct drm_minor for this file. */
> > >       struct drm_minor *minor;
> > > +#if IS_ENABLED(CONFIG_CGROUP_DRM)
> > > +    struct list_head clink;
> > > +#endif
> > > +
> > >       /**
> > >        * @object_idr:
> > >        *