Hello, Glauber. On Wed, Mar 21, 2012 at 11:36:19AM +0400, Glauber Costa wrote: > On 03/20/2012 10:31 PM, Tejun Heo wrote: > >Hello, Glauber. > > > >On Tue, Mar 20, 2012 at 08:50:56PM +0400, Glauber Costa wrote: > >>@@ -4929,7 +4929,9 @@ mem_cgroup_create(struct cgroup *cont) > >> atomic_set(&memcg->refcnt, 1); > >> memcg->move_charge_at_immigrate = 0; > >> mutex_init(&memcg->thresholds_lock); > >>- return&memcg->css; > >>+ > >>+ if (!register_kmem_files(memcg,&mem_cgroup_subsys)) > >>+ return&memcg->css; > > > >After the change, I think register_kmem_files() is a quite misleading > >name. > > how about init_kmem() ? > > Remember the slab bits will are likely to end up here as well in the end. I don't know. Whatever which describes what's going on. memcg_init_kmem()? > >So, init_cgroup() is overloaded to do two things - one load time init > >and per-cgroup init, depending on the args. > > Yes. I don't love it, but there is quite a bunch of precedents for this. > Like the shrinkers in vmscan, for instance. > > a NULL argument is a probe, a valid argument should have action taken. Please don't. Just add a new callback if necessary. > >What I don't get is why you can't just keep this. Is it because the > >files might appear before the protocol is registered? Wouldn't it be > >much better to add ipv4_tcp_init_cgroup() or whatever call to > >inet_init() instead of overloading init_cgroup() with mostly unrelated > >stuff? > > > > The reason is that this has to be kept generic for protocols that > may want to implement this in the future - since the pressure > controls themselves are generic, the per-cgroup versions should be > as well. > > And in general, a protocol can live in a module, or not be registered > despite being compiled in. Hmmmm... yeah, CGROUP_SUBSYS_CFTYPES() would register the files on module load but won't unregister them on unload. Will fix that. However, the fact that files living in modules shouldn't be a problem in itself. If those file handlers can cope with protocol not being registered yet, everything should be fine. > Now, what we do with the files, are our decision in the end. If you > want, we can use CGROUP_SUBSYS_CFTYPES(mem_cgroup_subsys, tcp_files) > as you suggested. tcp itself is always available if it is compiled in. > Then in the future, if anyone cares about adding support for a > protocol that may differ in that aspect, we can put the files > nevertheless, and > use ENOTSUPP as kame suggested for the swap accounting. I don't quite get why a protocol module would be loaded but not reigstered. Do we actually have cases like that? I know it's mechanically possible but don't think there's any actual use case or existing code which does that, so no need to worry about them. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
