Re: [RFC PATCH v3 02/11] cgroup: Add mechanism to register DRM devices
On Wed, Jun 26, 2019 at 5:04 PM Daniel Vetter wrote: > On Wed, Jun 26, 2019 at 10:37 PM Kenny Ho wrote: > > (sending again, I keep missing the reply-all in gmail.) > You can make it the default somewhere in the gmail options. Um... interesting, my option was actually not set (neither reply or reply-all.) > > On Wed, Jun 26, 2019 at 11:56 AM Daniel Vetter wrote: > > > > > > Why the separate, explicit registration step? I think a simpler design for > > > drivers would be that we set up cgroups if there's anything to be > > > controlled, and then for GEM drivers the basic GEM stuff would be set up > > > automically (there's really no reason not to I think). > > > > Is this what you mean with the comment about drm_dev_register below? > > I think I understand what you are saying but not super clear. Are you > > suggesting the use of driver feature bits (drm_core_check_feature, > > etc.) similar to the way Brian Welty did in his proposal in May? > > Also not exactly a fan of driver feature bits tbh. What I had in mind was: > > - For stuff like the GEM accounting which we can do for all drivers > easily (we can't do the enforcment, that needs a few changes), just > roll it out for everyone. I.e. if you enable the DRMCG Kconfig, all > DRIVER_GEM would get that basic gem cgroup accounting. > > - for other bits the driver just registers certain things, like "I can > enforce gem limits" or "I have gpu memory regions vram, tt, and system > and can enforce them" in their normal driver setup. Then at > drm_dev_register time we register all these additional cgroups, like > we today register all the other interafaces and pieces of a drm_device > (drm_minor, drm_connectors, debugfs files, sysfs stuff, all these > things). > > Since the concepts are still a bit in flux, let's take an example from > the modeset side: > - driver call drm_connector_init() to create connector object > - drm_dev_register() also sets up all the public interfaces for that > connector (debugfs, sysfs, ...) > > I think a similar setup would be good for cgroups here, you just > register your special ttm_mem_reg or whatever, and the magic happens > automatically. Ok, I will look into those (I am not too familiar about those at this point.) > > > I have no idea, but is this guaranteed to get them all? > > > > I believe so, base on my understanding about > > css_for_each_descendant_pre and how I am starting from the root > > cgroup. Hopefully I didn't miss anything. > > Well it's rcu, so I expect it'll race with concurrent > addition/removal. And the kerneldoc has some complicated sounding > comments about how to synchronize that with some locks that I don't > fully understand, but I think you're also not having any additional > locking so not sure this all works correctly ... > > Do we still need the init_dmcgrp stuff if we'd just embedd? That would > probably be the simplest way to solve this all :-) I will need to dig into it a bit more to know for sure. I think I still need the init_drmcgrp stuff. I implemented it like this because the cgroup subsystem appear to be initialized before the drm subsystem so the root cgroup does not know any drm devices and the per device default limits are not set. In theory, I should only need to set the root cgroup (so I don't need to use css_for_each_descendant_pre, which requires the rcu_lock.) But I am not 100% confident there won't be any additional cgroup being added to the hierarchy between cgroup subsystem init and drm subsystem init. Alternatively I can protect it with an additional mutex but I am not sure if that's needed. Regards, Kenny ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 02/11] cgroup: Add mechanism to register DRM devices
On Wed, Jun 26, 2019 at 10:37 PM Kenny Ho wrote:
> (sending again, I keep missing the reply-all in gmail.)
You can make it the default somewhere in the gmail options.
(also resending, I missed that you didn't group-replied).
On Wed, Jun 26, 2019 at 10:25 PM Kenny Ho wrote:
>
> On Wed, Jun 26, 2019 at 11:56 AM Daniel Vetter wrote:
> >
> > Why the separate, explicit registration step? I think a simpler design for
> > drivers would be that we set up cgroups if there's anything to be
> > controlled, and then for GEM drivers the basic GEM stuff would be set up
> > automically (there's really no reason not to I think).
>
> Is this what you mean with the comment about drm_dev_register below?
> I think I understand what you are saying but not super clear. Are you
> suggesting the use of driver feature bits (drm_core_check_feature,
> etc.) similar to the way Brian Welty did in his proposal in May?
Also not exactly a fan of driver feature bits tbh. What I had in mind was:
- For stuff like the GEM accounting which we can do for all drivers
easily (we can't do the enforcment, that needs a few changes), just
roll it out for everyone. I.e. if you enable the DRMCG Kconfig, all
DRIVER_GEM would get that basic gem cgroup accounting.
- for other bits the driver just registers certain things, like "I can
enforce gem limits" or "I have gpu memory regions vram, tt, and system
and can enforce them" in their normal driver setup. Then at
drm_dev_register time we register all these additional cgroups, like
we today register all the other interafaces and pieces of a drm_device
(drm_minor, drm_connectors, debugfs files, sysfs stuff, all these
things).
Since the concepts are still a bit in flux, let's take an example from
the modeset side:
- driver call drm_connector_init() to create connector object
- drm_dev_register() also sets up all the public interfaces for that
connector (debugfs, sysfs, ...)
I think a similar setup would be good for cgroups here, you just
register your special ttm_mem_reg or whatever, and the magic happens
automatically.
> > Also tying to the minor is a bit funky, since we have multiple of these.
> > Need to make sure were at least consistent with whether we use the primary
> > or render minor - I'd always go with the primary one like you do here.
>
> Um... come to think of it, I can probably embed struct drmcgrp_device
> into drm_device and that way I don't really need to keep a separate
> array of
> known_drmcgrp_devs and get rid of that max_minor thing. Not sure why
> I didn't think of this before.
Yeah if that's possible, embedding is definitely the preferred way.
drm_device is huge already, and the per-device overhead really doesn't
matter.
> > > +
> > > +int drmcgrp_register_device(struct drm_device *dev)
> >
> > Imo this should be done as part of drm_dev_register (maybe only if the
> > driver has set up a controller or something). Definitely with the
> > unregister logic below. Also anything used by drivers needs kerneldoc.
> >
> >
> > > + /* init cgroups created before registration (i.e. root cgroup) */
> > > + if (root_drmcgrp != NULL) {
> > > + struct cgroup_subsys_state *pos;
> > > + struct drmcgrp *child;
> > > +
> > > + rcu_read_lock();
> > > + css_for_each_descendant_pre(pos, &root_drmcgrp->css) {
> > > + child = css_drmcgrp(pos);
> > > + init_drmcgrp(child, dev);
> > > + }
> > > + rcu_read_unlock();
> >
> > I have no idea, but is this guaranteed to get them all?
>
> I believe so, base on my understanding about
> css_for_each_descendant_pre and how I am starting from the root
> cgroup. Hopefully I didn't miss anything.
Well it's rcu, so I expect it'll race with concurrent
addition/removal. And the kerneldoc has some complicated sounding
comments about how to synchronize that with some locks that I don't
fully understand, but I think you're also not having any additional
locking so not sure this all works correctly ...
Do we still need the init_dmcgrp stuff if we'd just embedd? That would
probably be the simplest way to solve this all :-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 02/11] cgroup: Add mechanism to register DRM devices
(sending again, I keep missing the reply-all in gmail.)
On Wed, Jun 26, 2019 at 11:56 AM Daniel Vetter wrote:
>
> Why the separate, explicit registration step? I think a simpler design for
> drivers would be that we set up cgroups if there's anything to be
> controlled, and then for GEM drivers the basic GEM stuff would be set up
> automically (there's really no reason not to I think).
Is this what you mean with the comment about drm_dev_register below?
I think I understand what you are saying but not super clear. Are you
suggesting the use of driver feature bits (drm_core_check_feature,
etc.) similar to the way Brian Welty did in his proposal in May?
> Also tying to the minor is a bit funky, since we have multiple of these.
> Need to make sure were at least consistent with whether we use the primary
> or render minor - I'd always go with the primary one like you do here.
Um... come to think of it, I can probably embed struct drmcgrp_device
into drm_device and that way I don't really need to keep a separate
array of
known_drmcgrp_devs and get rid of that max_minor thing. Not sure why
I didn't think of this before.
> > +
> > +int drmcgrp_register_device(struct drm_device *dev)
>
> Imo this should be done as part of drm_dev_register (maybe only if the
> driver has set up a controller or something). Definitely with the
> unregister logic below. Also anything used by drivers needs kerneldoc.
>
>
> > + /* init cgroups created before registration (i.e. root cgroup) */
> > + if (root_drmcgrp != NULL) {
> > + struct cgroup_subsys_state *pos;
> > + struct drmcgrp *child;
> > +
> > + rcu_read_lock();
> > + css_for_each_descendant_pre(pos, &root_drmcgrp->css) {
> > + child = css_drmcgrp(pos);
> > + init_drmcgrp(child, dev);
> > + }
> > + rcu_read_unlock();
>
> I have no idea, but is this guaranteed to get them all?
I believe so, base on my understanding about
css_for_each_descendant_pre and how I am starting from the root
cgroup. Hopefully I didn't miss anything.
Regards,
Kenny
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 02/11] cgroup: Add mechanism to register DRM devices
On Wed, Jun 26, 2019 at 11:05:13AM -0400, Kenny Ho wrote:
> Change-Id: I908ee6975ea0585e4c30eafde4599f87094d8c65
> Signed-off-by: Kenny Ho
Why the separate, explicit registration step? I think a simpler design for
drivers would be that we set up cgroups if there's anything to be
controlled, and then for GEM drivers the basic GEM stuff would be set up
automically (there's really no reason not to I think).
Also tying to the minor is a bit funky, since we have multiple of these.
Need to make sure were at least consistent with whether we use the primary
or render minor - I'd always go with the primary one like you do here.
> ---
> include/drm/drm_cgroup.h | 24
> include/linux/cgroup_drm.h | 10
> kernel/cgroup/drm.c| 116 +
> 3 files changed, 150 insertions(+)
> create mode 100644 include/drm/drm_cgroup.h
>
> diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h
> new file mode 100644
> index ..ddb9eab64360
> --- /dev/null
> +++ b/include/drm/drm_cgroup.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: MIT
> + * Copyright 2019 Advanced Micro Devices, Inc.
> + */
> +#ifndef __DRM_CGROUP_H__
> +#define __DRM_CGROUP_H__
> +
> +#ifdef CONFIG_CGROUP_DRM
> +
> +int drmcgrp_register_device(struct drm_device *device);
> +
> +int drmcgrp_unregister_device(struct drm_device *device);
> +
> +#else
> +static inline int drmcgrp_register_device(struct drm_device *device)
> +{
> + return 0;
> +}
> +
> +static inline int drmcgrp_unregister_device(struct drm_device *device)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_CGROUP_DRM */
> +#endif /* __DRM_CGROUP_H__ */
> diff --git a/include/linux/cgroup_drm.h b/include/linux/cgroup_drm.h
> index 9928e60037a5..27497f786c93 100644
> --- a/include/linux/cgroup_drm.h
> +++ b/include/linux/cgroup_drm.h
> @@ -6,10 +6,20 @@
>
> #ifdef CONFIG_CGROUP_DRM
>
> +#include
> #include
> +#include
> +
> +/* limit defined per the way drm_minor_alloc operates */
> +#define MAX_DRM_DEV (64 * DRM_MINOR_RENDER)
> +
> +struct drmcgrp_device_resource {
> + /* for per device stats */
> +};
>
> struct drmcgrp {
> struct cgroup_subsys_state css;
> + struct drmcgrp_device_resource *dev_resources[MAX_DRM_DEV];
> };
>
> static inline struct drmcgrp *css_drmcgrp(struct cgroup_subsys_state *css)
> diff --git a/kernel/cgroup/drm.c b/kernel/cgroup/drm.c
> index 66cb1dda023d..7da6e0d93991 100644
> --- a/kernel/cgroup/drm.c
> +++ b/kernel/cgroup/drm.c
> @@ -1,28 +1,99 @@
> // SPDX-License-Identifier: MIT
> // Copyright 2019 Advanced Micro Devices, Inc.
> +#include
> #include
> #include
> +#include
> +#include
> +#include
> #include
> +#include
> +#include
> +#include
> +
> +static DEFINE_MUTEX(drmcgrp_mutex);
> +
> +struct drmcgrp_device {
> + struct drm_device *dev;
> + struct mutexmutex;
> +};
> +
> +/* indexed by drm_minor for access speed */
> +static struct drmcgrp_device *known_drmcgrp_devs[MAX_DRM_DEV];
> +
> +static int max_minor;
Uh no global stuff like this please. Or some explanation in the commit
message why we really cant avoid this.
> +
>
> static struct drmcgrp *root_drmcgrp __read_mostly;
>
> static void drmcgrp_css_free(struct cgroup_subsys_state *css)
> {
> struct drmcgrp *drmcgrp = css_drmcgrp(css);
> + int i;
> +
> + for (i = 0; i <= max_minor; i++) {
> + if (drmcgrp->dev_resources[i] != NULL)
> + kfree(drmcgrp->dev_resources[i]);
> + }
>
> kfree(drmcgrp);
> }
>
> +static inline int init_drmcgrp_single(struct drmcgrp *drmcgrp, int minor)
> +{
> + struct drmcgrp_device_resource *ddr = drmcgrp->dev_resources[minor];
> +
> + if (ddr == NULL) {
> + ddr = kzalloc(sizeof(struct drmcgrp_device_resource),
> + GFP_KERNEL);
> +
> + if (!ddr)
> + return -ENOMEM;
> +
> + drmcgrp->dev_resources[minor] = ddr;
> + }
> +
> + /* set defaults here */
> +
> + return 0;
> +}
> +
> +static inline int init_drmcgrp(struct drmcgrp *drmcgrp, struct drm_device
> *dev)
> +{
> + int rc = 0;
> + int i;
> +
> + if (dev != NULL) {
> + rc = init_drmcgrp_single(drmcgrp, dev->primary->index);
> + return rc;
> + }
> +
> + for (i = 0; i <= max_minor; i++) {
> + rc = init_drmcgrp_single(drmcgrp, i);
> + if (rc)
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> static struct cgroup_subsys_state *
> drmcgrp_css_alloc(struct cgroup_subsys_state *parent_css)
> {
> struct drmcgrp *parent = css_drmcgrp(parent_css);
> struct drmcgrp *drmcgrp;
> + int rc;
>
> drmcgrp = kzalloc(sizeof(struct drmcgrp), GFP_KERNEL);
> if (!drmcgrp)
> return ERR_PTR(-ENOMEM);
>
> + rc = init_drmcgrp(drmcgrp, NULL);
> + if (rc) {
> + drmcgrp_css_free(&drmcgrp-
[RFC PATCH v3 02/11] cgroup: Add mechanism to register DRM devices
Change-Id: I908ee6975ea0585e4c30eafde4599f87094d8c65
Signed-off-by: Kenny Ho
---
include/drm/drm_cgroup.h | 24
include/linux/cgroup_drm.h | 10
kernel/cgroup/drm.c| 116 +
3 files changed, 150 insertions(+)
create mode 100644 include/drm/drm_cgroup.h
diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h
new file mode 100644
index ..ddb9eab64360
--- /dev/null
+++ b/include/drm/drm_cgroup.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: MIT
+ * Copyright 2019 Advanced Micro Devices, Inc.
+ */
+#ifndef __DRM_CGROUP_H__
+#define __DRM_CGROUP_H__
+
+#ifdef CONFIG_CGROUP_DRM
+
+int drmcgrp_register_device(struct drm_device *device);
+
+int drmcgrp_unregister_device(struct drm_device *device);
+
+#else
+static inline int drmcgrp_register_device(struct drm_device *device)
+{
+ return 0;
+}
+
+static inline int drmcgrp_unregister_device(struct drm_device *device)
+{
+ return 0;
+}
+#endif /* CONFIG_CGROUP_DRM */
+#endif /* __DRM_CGROUP_H__ */
diff --git a/include/linux/cgroup_drm.h b/include/linux/cgroup_drm.h
index 9928e60037a5..27497f786c93 100644
--- a/include/linux/cgroup_drm.h
+++ b/include/linux/cgroup_drm.h
@@ -6,10 +6,20 @@
#ifdef CONFIG_CGROUP_DRM
+#include
#include
+#include
+
+/* limit defined per the way drm_minor_alloc operates */
+#define MAX_DRM_DEV (64 * DRM_MINOR_RENDER)
+
+struct drmcgrp_device_resource {
+ /* for per device stats */
+};
struct drmcgrp {
struct cgroup_subsys_state css;
+ struct drmcgrp_device_resource *dev_resources[MAX_DRM_DEV];
};
static inline struct drmcgrp *css_drmcgrp(struct cgroup_subsys_state *css)
diff --git a/kernel/cgroup/drm.c b/kernel/cgroup/drm.c
index 66cb1dda023d..7da6e0d93991 100644
--- a/kernel/cgroup/drm.c
+++ b/kernel/cgroup/drm.c
@@ -1,28 +1,99 @@
// SPDX-License-Identifier: MIT
// Copyright 2019 Advanced Micro Devices, Inc.
+#include
#include
#include
+#include
+#include
+#include
#include
+#include
+#include
+#include
+
+static DEFINE_MUTEX(drmcgrp_mutex);
+
+struct drmcgrp_device {
+ struct drm_device *dev;
+ struct mutexmutex;
+};
+
+/* indexed by drm_minor for access speed */
+static struct drmcgrp_device *known_drmcgrp_devs[MAX_DRM_DEV];
+
+static int max_minor;
+
static struct drmcgrp *root_drmcgrp __read_mostly;
static void drmcgrp_css_free(struct cgroup_subsys_state *css)
{
struct drmcgrp *drmcgrp = css_drmcgrp(css);
+ int i;
+
+ for (i = 0; i <= max_minor; i++) {
+ if (drmcgrp->dev_resources[i] != NULL)
+ kfree(drmcgrp->dev_resources[i]);
+ }
kfree(drmcgrp);
}
+static inline int init_drmcgrp_single(struct drmcgrp *drmcgrp, int minor)
+{
+ struct drmcgrp_device_resource *ddr = drmcgrp->dev_resources[minor];
+
+ if (ddr == NULL) {
+ ddr = kzalloc(sizeof(struct drmcgrp_device_resource),
+ GFP_KERNEL);
+
+ if (!ddr)
+ return -ENOMEM;
+
+ drmcgrp->dev_resources[minor] = ddr;
+ }
+
+ /* set defaults here */
+
+ return 0;
+}
+
+static inline int init_drmcgrp(struct drmcgrp *drmcgrp, struct drm_device *dev)
+{
+ int rc = 0;
+ int i;
+
+ if (dev != NULL) {
+ rc = init_drmcgrp_single(drmcgrp, dev->primary->index);
+ return rc;
+ }
+
+ for (i = 0; i <= max_minor; i++) {
+ rc = init_drmcgrp_single(drmcgrp, i);
+ if (rc)
+ return rc;
+ }
+
+ return 0;
+}
+
static struct cgroup_subsys_state *
drmcgrp_css_alloc(struct cgroup_subsys_state *parent_css)
{
struct drmcgrp *parent = css_drmcgrp(parent_css);
struct drmcgrp *drmcgrp;
+ int rc;
drmcgrp = kzalloc(sizeof(struct drmcgrp), GFP_KERNEL);
if (!drmcgrp)
return ERR_PTR(-ENOMEM);
+ rc = init_drmcgrp(drmcgrp, NULL);
+ if (rc) {
+ drmcgrp_css_free(&drmcgrp->css);
+ return ERR_PTR(rc);
+ }
+
if (!parent)
root_drmcgrp = drmcgrp;
@@ -40,3 +111,48 @@ struct cgroup_subsys drm_cgrp_subsys = {
.legacy_cftypes = files,
.dfl_cftypes= files,
};
+
+int drmcgrp_register_device(struct drm_device *dev)
+{
+ struct drmcgrp_device *ddev;
+
+ ddev = kzalloc(sizeof(struct drmcgrp_device), GFP_KERNEL);
+ if (!ddev)
+ return -ENOMEM;
+
+ ddev->dev = dev;
+ mutex_init(&ddev->mutex);
+
+ mutex_lock(&drmcgrp_mutex);
+ known_drmcgrp_devs[dev->primary->index] = ddev;
+ max_minor = max(max_minor, dev->primary->index);
+ mutex_unlock(&drmcgrp_mutex);
+
+ /* init cgroups created before registration (i.e. root cgroup) */
+ if (root_drmcgrp != NULL) {
+ struct cgroup_subsys_state *pos;
+ struc
