[RFC PATCH v3 02/11] cgroup: Add mechanism to register DRM devices

[Daniel Vetter]

On Wed, Jun 26, 2019 at 10:37 PM Kenny Ho <y2kenny at gmail.com> 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 <y2kenny at gmail.com> wrote:
>
> On Wed, Jun 26, 2019 at 11:56 AM Daniel Vetter <daniel at ffwll.ch> 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