On Wed, Jun 26, 2019 at 10:37 PM Kenny Ho <y2kenny@xxxxxxxxx> 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@xxxxxxxxx> wrote: > > On Wed, Jun 26, 2019 at 11:56 AM Daniel Vetter <daniel@xxxxxxxx> 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