Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [Monitors]

  Powered by Linux