Hi Tejun,
On 09/11/2012 11:01 PM, Tejun Heo wrote:
Hello, Daniel.
I generally like this but I still think it's too big a patch.
Yes, I agree it is a bit too big.
diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index c75e3f9..6bc460c 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -326,9 +326,7 @@ struct cgroup_subsys net_prio_subsys = {
.create = cgrp_create,
.destroy = cgrp_destroy,
.attach = net_prio_attach,
-#ifdef CONFIG_NETPRIO_CGROUP
.subsys_id = net_prio_subsys_id,
-#endif
.base_cftypes = ss_files,
.module = THIS_MODULE
};
@@ -366,10 +364,6 @@ static int __init init_cgroup_netprio(void)
ret = cgroup_load_subsys(&net_prio_subsys);
if (ret)
goto out;
-#ifndef CONFIG_NETPRIO_CGROUP
- smp_wmb();
- net_prio_subsys_id = net_prio_subsys.subsys_id;
-#endif
register_netdevice_notifier(&netprio_device_notifier);
@@ -386,11 +380,6 @@ static void __exit exit_cgroup_netprio(void)
cgroup_unload_subsys(&net_prio_subsys);
-#ifndef CONFIG_NETPRIO_CGROUP
- net_prio_subsys_id = -1;
- synchronize_rcu();
For example, it's not evident the above is correct and it's burried
with all other changes. Can't we just assign the fixed subsys ID to
net_prio_subsys_id in this patch? This patch would be correct without
any netprio changes, no?
If net_prio_subsys_id is changed to be an enum, then the compiler will
report an error:
error: lvalue required as left operand of assignment
that was the reason why I kept this change here. I think I just don't
get what you are trying to tell me.
Please separate these changes and explain them.
I will do that as soon I figured out what you are telling me.
BTW, people who use barriers of any kind without explicitly explaining
what's going on need to be kicked hard in the ass. :(
I looked up the commit message when the synchronze_rcu() was added. It
was not really explaining it. I really spend a few hours starring at
this code.
thanks for the review,
daniel
--
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
