Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <77499370-bb0e-7f7e-ac1b-ad14f47578d9@amd.com>
Date:   Thu, 20 Oct 2022 08:40:28 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [RFC 02/17] drm: Track clients per owning process
Content-Language: en-US
To:     Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Intel-gfx@lists.freedesktop.org
Cc:     cgroups@vger.kernel.org, linux-kernel@vger.kernel.org,
        Tejun Heo <tj@kernel.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Zefan Li <lizefan.x@bytedance.com>,
        Dave Airlie <airlied@redhat.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Rob Clark <robdclark@chromium.org>,
        =?UTF-8?Q?St=c3=a9phane_Marchesin?= <marcheu@chromium.org>,
        "T . J . Mercier" <tjmercier@google.com>, Kenny.Ho@amd.com,
        Brian Welty <brian.welty@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@intel.com>
References: <20221019173254.3361334-1-tvrtko.ursulin@linux.intel.com>
 <20221019173254.3361334-3-tvrtko.ursulin@linux.intel.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <20221019173254.3361334-3-tvrtko.ursulin@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N3BtRnJicHZnSVZOa0FweVExTWdlSmNjTDQwQ3JEeCtTV3Y0NjNvUDdBY1B1?=
 =?utf-8?B?dDg0cCt4aTZ6K1pPK1JZSUlYU2FwT0N5azdpM2NCajNnWjFzWE1Wazh3Yzkw?=
 =?utf-8?B?TU5qa3g1U1pEekx4b2tqVnk2VFNzaE16TGFXZHlJbkRvSXl1UXl5Wk9USitM?=
 =?utf-8?B?b0JnT3hBQmFHWXBwempJWEdDQjB2TWdvMktpeXVJTGNJTTNIelR6Smo5c0tW?=
 =?utf-8?B?Q3NVUURNWEp0U0o0bDFPbWh1cUVMVnBUREc2VUh0TUZWOC92Q2hXV1V2b0ZN?=
 =?utf-8?B?b2U4Y2FmODhDcFpoOXJQY1o0UVlqZHdvcmtIZEREWkhOZVVLZ2JEODVVWENu?=
 =?utf-8?B?MnU4WEpGbHRROTFvSHBzL1lmalpXTUh6azUzSytFZVhjOGNEQlNtdzVrSnNK?=
 =?utf-8?B?YWYxWnd6aCtYb0wzQVU2a0ZzLzIrejZScUZBZmZJTnJlUUZOTytOcEtYMlZM?=
 =?utf-8?B?NmExdEVreWJOVnNuUklQYUtvNjNWaVBiejJhUStyTDBOcWtrUTUxVVpzb2hU?=
 =?utf-8?B?eHZNK3h4eEdjT0c5bXFLSXA3MmV3Z2ppTUtMbWtlWXRaR29aWmxEUkd2cVVM?=
 =?utf-8?B?d0RWeE9JcStFS2VYT1ZzUXhvTVFUSVBaMVNmZnFFY0NuY0Rrd0RWOHlmRlVj?=
 =?utf-8?B?c2hUUXZmMmQwem1TRGJFckVqbmlPem9IMndNVzdDc1VseERUOEpRa3dGZTRu?=
 =?utf-8?B?a0NHdFY4QTJCTzY1cnZHVDYwcUEvbDJUazhPT0hLN0Rwc3l2bk95d0RLMHRq?=
 =?utf-8?B?Yy9EdjFPUmZEZ0NXYVIrWlNwUVk2MDFrVG1qdkNpY3RwVkNPL2VtS3Z6T3pW?=
 =?utf-8?B?YXJtOHBtOUY5a01McDlKRzhwMW12Qy9DVUtiMDlEM0QrTW5CTHJidXpOUGhL?=
 =?utf-8?B?eWRZZWNIbkFqMkI3eUVLVzBLRlJoODExZjFGQTg4bzZwSTRHeUxicFpZa3VH?=
 =?utf-8?B?R1krNE5YRmVGcVIrSnk0NlhQL3p5bHZBZWYzUHpYbHlUbFB5L2FDbk9FNFc1?=
 =?utf-8?B?QXJCT2lNMHZPNkQ1OTZwMUhMeEhnSmREUUtiTzJUZUFnWXkwdGVZVys3a2Jj?=
 =?utf-8?B?aFV5MDdnd201SS9PM3lwblgxdHowTkR3RmcwU1pnWmxnYmFPb0hEdERFa1pC?=
 =?utf-8?B?QzZId2VJUHRvN1E0RGNrN04yZHpPWjkrNWVLZVhxVW92NkNXUm43NzFOZnNr?=
 =?utf-8?B?S01kWURkSVhEOUxMazNMUnBiRmwyVEV2Mk4vc25MaDNtemxIL0NVQkozUkxw?=
 =?utf-8?B?b0ZDSnRhQ0tUWC9vUUdtdElRdllwZ0hTdEs5aXFKVWhpc0t1MFFwRFVoc2pp?=
 =?utf-8?B?MkJhajFlU2ZOdjJEYlZvUGdjbXFQQURhTnMyNTVQcmttYlQrSUEzakpsWW9i?=
 =?utf-8?B?eW5FSVc2alJQdjRPb1VOU2lmMGZnTVBGbVZvV1A5NjRhYnBjZlhxSHYzNmRP?=
 =?utf-8?B?ZkM4QnZVd2lzY2xxV0VrdWRRNm1ybmkwWlNPSlFGanEwV0VjOWx6MkRXRDdQ?=
 =?utf-8?B?TkZUeEI4eVFrTmNyYXJHeElndmNTQ2xsVGtTTlhKaVIvSFVGYm5CaDJucUVW?=
 =?utf-8?B?Y3BiWVBaTmdjVCtNbWY3T05STVNoQTdtaUk3MGN2TC9ESGptRnhodmZxY0sx?=
 =?utf-8?B?ZXR6RTdQN0lOakRyS0ZXQ3MzbHU4S1ZKSngrYTdkdzZCY3FXd1EvbFBoVHd2?=
 =?utf-8?B?S3hEYnNuMkhJdlVVZVRHMUpSbHgybDBBYW53MVp3VG1hYXh2ejlqY0dMSm1R?=
 =?utf-8?B?dy9vbHpXa29kYmdPL0p3UXdpL2hiOHI5Um9EZzNPNHZlQUxRSjh2ZGVHdnM5?=
 =?utf-8?B?Nmh5WGdFQjlQWkUrNVhEN0k4Ui9manNCSDlkMmRYRkUzWUoxQ08wSEV6ZlZC?=
 =?utf-8?B?Tmg4eWhURUMreWlWUTJwcW9Bb3FMWUJURXJxUHJkTWRGQU9wUFNGNmpsWDZD?=
 =?utf-8?B?Y2kraWRhV2pnYS9iNFcrWkJCVDN2NjdOTlBIc3QyaHprVE15VUJFS2pWcXR1?=
 =?utf-8?B?QkN0YkVTQmtNRWFoSzcwWHdOdFNrVkJlZHJJWG5tZHZVQklQem96NG9lRGhH?=
 =?utf-8?B?QkpKM0p1UGNGQjFPUmtUUVloSml2eVl6bHU3WnhDTXlUR3p2bWxVMVJ1WHNE?=
 =?utf-8?Q?2IqwIJQKlFzSc+XuCfTpI+Byv?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 43bcb665-a850-4bff-3fba-08dab265fd38
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Oct 2022 06:40:33.7515
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ldqjbDHW4AByf1vZ+aYbKbckJMOoKVBnP6aCBCsFIVaUSB73fRLrTk8NDIOEf5Yj
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB6062
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,URIBL_BLOCKED autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 19.10.22 um 19:32 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> 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.

I think you need to fix this problem first. And BTW: and unsigned long 
doesn't work as PID either with containers.

Regards,
Christian.

>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   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:
>   	 *