Message ID | 20221019173254.3361334-3-tvrtko.ursulin@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp451340wrs; Wed, 19 Oct 2022 10:36:52 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4IUSm2jU37KKsf4x8oox1XitZqt4z5THvb9WVZeQEN+bNwx/jQcGc6GV9RUvj9iDWKJXQy X-Received: by 2002:a05:6402:170f:b0:458:9653:6466 with SMTP id y15-20020a056402170f00b0045896536466mr8605432edu.181.1666201011871; Wed, 19 Oct 2022 10:36:51 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666201011; cv=none; d=google.com; s=arc-20160816; b=j2b8TQ8r8SDfpWpWAhsAVbGEPzIcRUguGeADHxmlJ3rMFgXiZ8MN1hAC1kI2rVUGtK /90CLhcVk4WSREXdDrdKxNH79SqWTlYslE1abRW9CPPxSGBtiuw47CI+rOIAt1rSvmnU B1xYYx9P/5gjcqvBhKvcYOt+M1sPAJPXVRS+s6eyfXP9S8wAskfHBuDFkmgoPEF/pjK3 T+iapfCltaLzwpDqQeR6MGlPMJ1pC4rzsMVDfRj/szZhhyd41P1bqfkvy5ndSlPAbCGh dwChyA69ghTk97Hpi5DF1zHerQsQCDjPqYJS/JoBIUEZ+wMAc5SpPQZBNXXExdb7z37h 6ehw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=WadaNEmSLG49Eme1He5P3yIfjQs3fNME429HI1S5wKU=; b=j14WfKzke9brOZo/8PPELSlkmc1eZ4D5S2zRHndq9HcVsIKNErC15MJgpVQvfGs8KK yfRcw6RpgO02MwururYY/yw4xWqidRSiDZ/Qc75yw38mU++M0ElpujBCtG96KMXeQqQy zxNuoUDhtqdBNCryyKbUOCeRsXhk+OUpB/9XvsVmN09JaaMR6dqMVzHmjOpX5Efuf5Gx OAtIPo48yguCbYBZ5W3QSbdb+ck15gvoNQhXA4utAEBW6S4TE0g65Bn+PncUKqnAJSMN TWvODwx22jKyKRhvJ8a2idLZmmr9/i9uPnH7oEINCKIeuoseQhuR34xju6+GY7X1c6Is CHoQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=SxQl+4FR; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id j20-20020a1709066dd400b0078d9b2facaesi13418575ejt.339.2022.10.19.10.36.23; Wed, 19 Oct 2022 10:36:51 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=SxQl+4FR; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230501AbiJSRdd (ORCPT <rfc822;samuel.l.nystrom@gmail.com> + 99 others); Wed, 19 Oct 2022 13:33:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60524 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230136AbiJSRd1 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 19 Oct 2022 13:33:27 -0400 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C9B391C20BF; Wed, 19 Oct 2022 10:33:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1666200806; x=1697736806; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=34jcEyaboM7Cpl5hmtqGUgBiY4OSzKr0bOyVxNTPRuA=; b=SxQl+4FRxYkORNOe0E29yxbuq18rp/vkb10ImMboVEMmhd5MSqrwwD5s pjefJ4UB4COb2dMzKcAwHXB55DqqpjDKwjCrvGaH6wXldzVng4z/4CGGT ZvZYTYbPskHJhLakqLVJsZeqhP5bHNqikUfnr9kHr096R2hDMih5CGQYP NA6eOPh8Xo4JlLDgy7rjqaVisHisBnnlkqg747G7nWK0ujN2xMJ3t2XEM LmuSdb2ubbUC84Stmt5F84WEdJdeBdE870AH6gy10ksRb32HqJZHsSmi3 NVbd8eb/U4VhsnC+Un2XLGRShFwcwrfY1reyokCg6YLz2BBAjjNgTRFbu g==; X-IronPort-AV: E=McAfee;i="6500,9779,10505"; a="306474309" X-IronPort-AV: E=Sophos;i="5.95,196,1661842800"; d="scan'208";a="306474309" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Oct 2022 10:33:26 -0700 X-IronPort-AV: E=McAfee;i="6500,9779,10505"; a="607204700" X-IronPort-AV: E=Sophos;i="5.95,196,1661842800"; d="scan'208";a="607204700" Received: from mjmcener-mobl1.amr.corp.intel.com (HELO localhost.localdomain) ([10.213.233.40]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Oct 2022 10:33:22 -0700 From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> To: 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, =?utf-8?q?Chris?= =?utf-8?q?tian_K=C3=B6nig?= <christian.koenig@amd.com>, Brian Welty <brian.welty@intel.com>, Tvrtko Ursulin <tvrtko.ursulin@intel.com> Subject: [RFC 02/17] drm: Track clients per owning process Date: Wed, 19 Oct 2022 18:32:39 +0100 Message-Id: <20221019173254.3361334-3-tvrtko.ursulin@linux.intel.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20221019173254.3361334-1-tvrtko.ursulin@linux.intel.com> References: <20221019173254.3361334-1-tvrtko.ursulin@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,HK_RANDOM_ENVFROM,HK_RANDOM_FROM, RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_NONE 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747138392276466543?= X-GMAIL-MSGID: =?utf-8?q?1747138392276466543?= |
Series |
DRM scheduling cgroup controller
|
|
Commit Message
Tvrtko Ursulin
Oct. 19, 2022, 5:32 p.m. UTC
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. 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
Comments
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: > *
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@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. 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. > 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. 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? Regards, Tvrtko > > 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: >> * >
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@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. > > 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@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: >>> * >>
On 20/10/2022 12:33, Christian König wrote: > 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@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. >> >> 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've put it on my todo list to try and come up with something standalone for this problem. Will see if I manage to send it separately or perhaps will start the next cgroup controller RFC with those patches. >>> 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. Yeah I think I have all that. I track struct pid, with a reference, in drm client, and release it when file descriptor is closed (indirectly via the DRM close hook). All I need, I think, is for that mapping to answer me "which drm_file objects" are in use by this struct pid pointer. I don't see a problem with lifetimes or scope yet. Regards, Tvrtko
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: *