[PATCH v2 00/10] cgroup: Assign subsystem IDs during compile time

[PATCH v2 00/10] cgroup: Assign subsystem IDs during compile time

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

Hi,

Most notable changes are, that enabling/disabling of the jump labels
are not inside the cgroup_lock anymore (create/destroy cb). Instead
the corresponding functions will be called on module load or unload.

CGROUP_BUILTIN_SUBSYS_COUNT is also gone in this version.  This time I
trade space for speed. Some extra cycles are spend to identify the
modules in the for loops, e.g.

for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
	struct cgroup_subsys_state *ss = cgrp->subsys[i];

	/* at bootup time, we don't worry about modular subsystems */
	if (!ss || (ss && ss->module))
		continue;

	[...]
}

CGROUP_SUBSYS_COUNT is currently 12 if all controllers are built.  I
haven't found any other way to get rid of CGROUP_BUILTIN_SUBSYS_COUNT
without real dirty preprocessor tricks.

Finally, the two versions of task_cls_classid() and task_netprioidx()
are merged together.

cheers,
daniel


Original cover letter:

The patch #1 and #2 are there to be able to introduce (#3, #4) the 
jump labels in task_cls_classid() and task_netprioidx(). The jump
labels are needed to know when it is safe to access the controller. 
For example not safe means the module is not yet loaded.

All those patches are just preparation for the center piece (#5) 
of these series. This one will remove the dynamic subsystem ID
generation and falls back to compile time generated IDs. 

This is the first result from the discussion around on the
"cgroup cls & netprio 'cleanups'" patches.

This patches are against net-next

v2: - do not use dirty precompiler tricks:
      use ss->module to identify modules in the loops.
    - enable/disable jump labels in module load/unload functions
    - merge builtin/module versions of task_cls_classid() and task_netprioidx
v1: - only use jump labels when built as module (#3, #4)
    - get rid of the additional 'pointer' (#5)
v0: - initial version

Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: Jamal Hadi Salim <jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Kamezawa Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Daniel Wagner (10):
  cgroup: net_cls: Use empty task_cls_classid() when
    !CONFIG_NET_CLS(_MODULE)
  cgroup: net_cls: Move sock_update_classid() decleration to
    cls_cgroup.h
  cgroup: net_cls: Protect access to task_cls_classid() when built as
    module
  cgroup: net_prio: Protect access to task_netprioidx() when built as
    module
  cgroup: Remove CGROUP_BUILTIN_SUBSYS_COUNT
  cgroup: Assign subsystem IDs during compile time
  cgroup: net_cls: Simplify ifdef logic
  cgroup: net_cls: Merge builtin and module version of
    task_cls_classid()
  cgroup: net_prio: Simplify ifdef logic
  cgroup: net_prio: Merge builtin and module version of
    task_netprioidx()

 include/linux/cgroup.h        | 30 ++++++++-----
 include/linux/cgroup_subsys.h | 24 +++++------
 include/net/cls_cgroup.h      | 51 +++++++++++-----------
 include/net/netprio_cgroup.h  | 48 ++++++++-------------
 include/net/sock.h            |  8 ----
 kernel/cgroup.c               | 98 +++++++++++++++++++++++--------------------
 net/core/netprio_cgroup.c     | 14 +++----
 net/core/sock.c               | 16 ++++---
 net/sched/cls_cgroup.c        | 20 ++++-----
 9 files changed, 151 insertions(+), 158 deletions(-)

-- 
1.7.12.rc1.16.g05a20c8

[PATCH v2 01/10] cgroup: net_cls: Use empty task_cls_classid() when !CONFIG_NET_CLS(_MODULE)

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Gao feng <gaofeng@cn.fujitsu.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: netdev@vger.kernel.org
Cc: cgroups@vger.kernel.org
---
 include/net/cls_cgroup.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index a4dc5b0..e2fe2b9 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -24,7 +24,8 @@ struct cgroup_cls_state
 	u32 classid;
 };
 
-#ifdef CONFIG_NET_CLS_CGROUP
+#if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
+
 static inline u32 task_cls_classid(struct task_struct *p)
 {
 	int classid;
@@ -39,7 +40,9 @@ static inline u32 task_cls_classid(struct task_struct *p)
 
 	return classid;
 }
-#else
+
+#elif IS_MODULE(CONFIG_NET_CLS_CGROUP)
+
 extern int net_cls_subsys_id;
 
 static inline u32 task_cls_classid(struct task_struct *p)
@@ -60,11 +63,16 @@ static inline u32 task_cls_classid(struct task_struct *p)
 
 	return classid;
 }
-#endif
+
 #else
+
 static inline u32 task_cls_classid(struct task_struct *p)
 {
 	return 0;
 }
-#endif
+
+#endif /* CONFIG_NET_CLS_CGROUP */
+
+#endif /* CONFIG_CGROURPS */
+
 #endif  /* _NET_CLS_CGROUP_H */
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v2 02/10] cgroup: net_cls: Move sock_update_classid() decleration to cls_cgroup.h

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

The only user of sock_update_classid() is net/socket.c which
happens to include cls_cgroup.h direclty.

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Jamal Hadi Salim <jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 include/net/cls_cgroup.h | 8 ++++++++
 include/net/sock.h       | 8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index e2fe2b9..401672c 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -24,6 +24,8 @@ struct cgroup_cls_state
 	u32 classid;
 };
 
+extern void sock_update_classid(struct sock *sk);
+
 #if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
 
 static inline u32 task_cls_classid(struct task_struct *p)
@@ -73,6 +75,12 @@ static inline u32 task_cls_classid(struct task_struct *p)
 
 #endif /* CONFIG_NET_CLS_CGROUP */
 
+#else /* !CONFIG_CGROUPS */
+
+static inline void sock_update_classid(struct sock *sk)
+{
+}
+
 #endif /* CONFIG_CGROURPS */
 
 #endif  /* _NET_CLS_CGROUP_H */
diff --git a/include/net/sock.h b/include/net/sock.h
index 72132ae..160a680 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1486,14 +1486,6 @@ extern void *sock_kmalloc(struct sock *sk, int size,
 extern void sock_kfree_s(struct sock *sk, void *mem, int size);
 extern void sk_send_sigurg(struct sock *sk);
 
-#ifdef CONFIG_CGROUPS
-extern void sock_update_classid(struct sock *sk);
-#else
-static inline void sock_update_classid(struct sock *sk)
-{
-}
-#endif
-
 /*
  * Functions to fill in entries in struct proto_ops when a protocol
  * does not implement a particular function.
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v2 03/10] cgroup: net_cls: Protect access to task_cls_classid() when built as module

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

The module version of task_cls_classid() checks if net_cls_sbusys_id
is valid to indentify when it is okay to access the controller.

Instead relying on the subusys_id to be set, make it explicit
with a jump label.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Gao feng <gaofeng@cn.fujitsu.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: netdev@vger.kernel.org
Cc: cgroups@vger.kernel.org
---
 include/net/cls_cgroup.h |  6 +++++-
 net/core/sock.c          |  5 +++++
 net/sched/cls_cgroup.c   | 11 +++++++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index 401672c..24443d2 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -16,6 +16,7 @@
 #include <linux/cgroup.h>
 #include <linux/hardirq.h>
 #include <linux/rcupdate.h>
+#include <linux/jump_label.h>
 
 #ifdef CONFIG_CGROUPS
 struct cgroup_cls_state
@@ -45,6 +46,9 @@ static inline u32 task_cls_classid(struct task_struct *p)
 
 #elif IS_MODULE(CONFIG_NET_CLS_CGROUP)
 
+extern struct static_key cgroup_cls_enabled;
+#define clscg_enabled static_key_false(&cgroup_cls_enabled)
+
 extern int net_cls_subsys_id;
 
 static inline u32 task_cls_classid(struct task_struct *p)
@@ -52,7 +56,7 @@ static inline u32 task_cls_classid(struct task_struct *p)
 	int id;
 	u32 classid = 0;
 
-	if (in_interrupt())
+	if (!clscg_enabled || in_interrupt())
 		return 0;
 
 	rcu_read_lock();
diff --git a/net/core/sock.c b/net/core/sock.c
index 8f67ced..8106e77 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -327,6 +327,11 @@ int __sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
 EXPORT_SYMBOL(__sk_backlog_rcv);
 
 #if defined(CONFIG_CGROUPS)
+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
+struct static_key cgroup_cls_enabled = STATIC_KEY_INIT_FALSE;
+EXPORT_SYMBOL_GPL(cgroup_cls_enabled);
+#endif
+
 #if !defined(CONFIG_NET_CLS_CGROUP)
 int net_cls_subsys_id = -1;
 EXPORT_SYMBOL_GPL(net_cls_subsys_id);
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 7743ea8..554dc5b 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -293,6 +293,12 @@ static int __init init_cgroup_cls(void)
 	if (ret)
 		cgroup_unload_subsys(&net_cls_subsys);
 
+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
+	if (ret)
+		goto out;
+	static_key_slow_inc(&cgroup_cls_enabled);
+#endif
+
 out:
 	return ret;
 }
@@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void)
 	synchronize_rcu();
 #endif
 
+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
+	static_key_slow_dec(&cgroup_cls_enabled);
+	rcu_barrier();
+#endif
+
 	cgroup_unload_subsys(&net_cls_subsys);
 }
 
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v2 04/10] cgroup: net_prio: Protect access to task_netprioidx() when built as module

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

The module version of task_netprioidex() checks if net_prio_subsys_id
is valid to indentify when it is okay to access the controller.

Instead relying on the net_prio_subsys_id to be set, make it explicit
with a jump label.

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Jamal Hadi Salim <jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 include/net/netprio_cgroup.h | 8 +++++++-
 net/core/netprio_cgroup.c    | 9 +++++++++
 net/core/sock.c              | 4 ++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/net/netprio_cgroup.h b/include/net/netprio_cgroup.h
index 2719dec..9ff58e4 100644
--- a/include/net/netprio_cgroup.h
+++ b/include/net/netprio_cgroup.h
@@ -16,7 +16,7 @@
 #include <linux/cgroup.h>
 #include <linux/hardirq.h>
 #include <linux/rcupdate.h>
-
+#include <linux/jump_label.h>
 
 struct netprio_map {
 	struct rcu_head rcu;
@@ -54,12 +54,18 @@ static inline u32 task_netprioidx(struct task_struct *p)
 
 #elif IS_MODULE(CONFIG_NETPRIO_CGROUP)
 
+extern struct static_key cgroup_netprio_enabled;
+#define netpriocg_enabled static_key_false(&cgroup_netprio_enabled)
+
 static inline u32 task_netprioidx(struct task_struct *p)
 {
 	struct cgroup_netprio_state *state;
 	int subsys_id;
 	u32 idx = 0;
 
+	if (!netpriocg_enabled)
+		return 0;
+
 	rcu_read_lock();
 	subsys_id = rcu_dereference_index_check(net_prio_subsys_id,
 						rcu_read_lock_held());
diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index c75e3f9..400ab94 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -371,6 +371,10 @@ static int __init init_cgroup_netprio(void)
 	net_prio_subsys_id = net_prio_subsys.subsys_id;
 #endif
 
+#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
+	static_key_slow_inc(&cgroup_netprio_enabled);
+#endif
+
 	register_netdevice_notifier(&netprio_device_notifier);
 
 out:
@@ -391,6 +395,11 @@ static void __exit exit_cgroup_netprio(void)
 	synchronize_rcu();
 #endif
 
+#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
+	static_key_slow_dec(&cgroup_netprio_enabled);
+	rcu_barrier();
+#endif
+
 	rtnl_lock();
 	for_each_netdev(&init_net, dev) {
 		old = rtnl_dereference(dev->priomap);
diff --git a/net/core/sock.c b/net/core/sock.c
index 8106e77..1f119d2 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -331,6 +331,10 @@ EXPORT_SYMBOL(__sk_backlog_rcv);
 struct static_key cgroup_cls_enabled = STATIC_KEY_INIT_FALSE;
 EXPORT_SYMBOL_GPL(cgroup_cls_enabled);
 #endif
+#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
+struct static_key cgroup_netprio_enabled = STATIC_KEY_INIT_FALSE;
+EXPORT_SYMBOL_GPL(cgroup_netprio_enabled);
+#endif
 
 #if !defined(CONFIG_NET_CLS_CGROUP)
 int net_cls_subsys_id = -1;
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v2 05/10] cgroup: Remove CGROUP_BUILTIN_SUBSYS_COUNT

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

CGROUP_BUILTIN_SUBSYS_COUNT is used as start or stop point
when looping over the subsys array. Since the subsys array is
64 entries long this is a good thing to do. Though we'd like
to reduce the array size considerable but we need to get rid
of CGROUP_BUILTIN_SUBSYS_COUNT to ease up the review process.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: cgroups@vger.kernel.org
---
 include/linux/cgroup.h |  1 -
 kernel/cgroup.c        | 75 +++++++++++++++++++++++++++++++-------------------
 2 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index c90eaa8..3787872 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -47,7 +47,6 @@ extern const struct file_operations proc_cgroup_operations;
 #define SUBSYS(_x) _x ## _subsys_id,
 enum cgroup_subsys_id {
 #include <linux/cgroup_subsys.h>
-	CGROUP_BUILTIN_SUBSYS_COUNT
 };
 #undef SUBSYS
 /*
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 7981850..f9433b4 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -88,7 +88,7 @@ static DEFINE_MUTEX(cgroup_root_mutex);
 
 /*
  * Generate an array of cgroup subsystem pointers. At boot time, this is
- * populated up to CGROUP_BUILTIN_SUBSYS_COUNT, and modular subsystems are
+ * populated with the built in subsystems, and modular subsystems are
  * registered after that. The mutable section of this array is protected by
  * cgroup_mutex.
  */
@@ -1291,11 +1291,13 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 	 * take duplicate reference counts on a subsystem that's already used,
 	 * but rebind_subsystems handles this case.
 	 */
-	for (i = CGROUP_BUILTIN_SUBSYS_COUNT; i < CGROUP_SUBSYS_COUNT; i++) {
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 		unsigned long bit = 1UL << i;
 
 		if (!(bit & opts->subsys_bits))
 			continue;
+		if (!subsys[i]->module)
+			continue;
 		if (!try_module_get(subsys[i]->module)) {
 			module_pin_failed = true;
 			break;
@@ -1307,12 +1309,14 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 		 * raced with a module_delete call, and to the user this is
 		 * essentially a "subsystem doesn't exist" case.
 		 */
-		for (i--; i >= CGROUP_BUILTIN_SUBSYS_COUNT; i--) {
+		for (i--; i > 1; i--) {
 			/* drop refcounts only on the ones we took */
 			unsigned long bit = 1UL << i;
 
 			if (!(bit & opts->subsys_bits))
 				continue;
+			if (!subsys[i]->module)
+				continue;
 			module_put(subsys[i]->module);
 		}
 		return -ENOENT;
@@ -1324,11 +1328,13 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 static void drop_parsed_module_refcounts(unsigned long subsys_bits)
 {
 	int i;
-	for (i = CGROUP_BUILTIN_SUBSYS_COUNT; i < CGROUP_SUBSYS_COUNT; i++) {
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 		unsigned long bit = 1UL << i;
 
 		if (!(bit & subsys_bits))
 			continue;
+		if (!subsys[i]->module)
+			continue;
 		module_put(subsys[i]->module);
 	}
 }
@@ -1401,6 +1407,7 @@ static void init_cgroup_housekeeping(struct cgroup *cgrp)
 	mutex_init(&cgrp->pidlist_mutex);
 	INIT_LIST_HEAD(&cgrp->event_list);
 	spin_lock_init(&cgrp->event_list_lock);
+	memset(cgrp->subsys, 0, sizeof(cgrp->subsys));
 }
 
 static void init_cgroup_root(struct cgroupfs_root *root)
@@ -4321,8 +4328,7 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
 	 * since cgroup_init_subsys will have already taken care of it.
 	 */
 	if (ss->module == NULL) {
-		/* a few sanity checks */
-		BUG_ON(ss->subsys_id >= CGROUP_BUILTIN_SUBSYS_COUNT);
+		/* a sanity check */
 		BUG_ON(subsys[ss->subsys_id] != ss);
 		return 0;
 	}
@@ -4336,7 +4342,7 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
 	 */
 	mutex_lock(&cgroup_mutex);
 	/* find the first empty slot in the array */
-	for (i = CGROUP_BUILTIN_SUBSYS_COUNT; i < CGROUP_SUBSYS_COUNT; i++) {
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 		if (subsys[i] == NULL)
 			break;
 	}
@@ -4439,7 +4445,6 @@ void cgroup_unload_subsys(struct cgroup_subsys *ss)
 
 	mutex_lock(&cgroup_mutex);
 	/* deassign the subsys_id */
-	BUG_ON(ss->subsys_id < CGROUP_BUILTIN_SUBSYS_COUNT);
 	subsys[ss->subsys_id] = NULL;
 
 	/* remove subsystem from rootnode's list of subsystems */
@@ -4502,10 +4507,13 @@ int __init cgroup_init_early(void)
 	for (i = 0; i < CSS_SET_TABLE_SIZE; i++)
 		INIT_HLIST_HEAD(&css_set_table[i]);
 
-	/* at bootup time, we don't worry about modular subsystems */
-	for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 		struct cgroup_subsys *ss = subsys[i];
 
+		/* at bootup time, we don't worry about modular subsystems */
+		if (!ss || (ss && ss->module))
+			continue;
+
 		BUG_ON(!ss->name);
 		BUG_ON(strlen(ss->name) > MAX_CGROUP_TYPE_NAMELEN);
 		BUG_ON(!ss->create);
@@ -4538,9 +4546,12 @@ int __init cgroup_init(void)
 	if (err)
 		return err;
 
-	/* at bootup time, we don't worry about modular subsystems */
-	for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 		struct cgroup_subsys *ss = subsys[i];
+
+		/* at bootup time, we don't worry about modular subsystems */
+		if (!ss || (ss && ss->module))
+			continue;
 		if (!ss->early_init)
 			cgroup_init_subsys(ss);
 		if (ss->use_id)
@@ -4735,13 +4746,16 @@ void cgroup_fork_callbacks(struct task_struct *child)
 {
 	if (need_forkexit_callback) {
 		int i;
-		/*
-		 * forkexit callbacks are only supported for builtin
-		 * subsystems, and the builtin section of the subsys array is
-		 * immutable, so we don't need to lock the subsys array here.
-		 */
-		for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
+		for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 			struct cgroup_subsys *ss = subsys[i];
+
+			/*
+			 * forkexit callbacks are only supported for
+			 * builtin subsystems.
+			 */
+			if (!ss || (ss && ss->module))
+				continue;
+
 			if (ss->fork)
 				ss->fork(child);
 		}
@@ -4846,12 +4860,13 @@ void cgroup_exit(struct task_struct *tsk, int run_callbacks)
 	tsk->cgroups = &init_css_set;
 
 	if (run_callbacks && need_forkexit_callback) {
-		/*
-		 * modular subsystems can't use callbacks, so no need to lock
-		 * the subsys array
-		 */
-		for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
+		for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 			struct cgroup_subsys *ss = subsys[i];
+
+			/* modular subsystems can't use callbacks */
+			if (!ss || (ss && ss->module))
+				continue;
+
 			if (ss->exit) {
 				struct cgroup *old_cgrp =
 					rcu_dereference_raw(cg->subsys[i])->cgroup;
@@ -5037,13 +5052,17 @@ static int __init cgroup_disable(char *str)
 	while ((token = strsep(&str, ",")) != NULL) {
 		if (!*token)
 			continue;
-		/*
-		 * cgroup_disable, being at boot time, can't know about module
-		 * subsystems, so we don't worry about them.
-		 */
-		for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
+		for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 			struct cgroup_subsys *ss = subsys[i];
 
+			/*
+			 * cgroup_disable, being at boot time, can't
+			 * know about module subsystems, so we don't
+			 * worry about them.
+			 */
+			if (!ss || (ss && ss->module))
+				continue;
+
 			if (!strcmp(token, ss->name)) {
 				ss->disabled = 1;
 				printk(KERN_INFO "Disabling %s control group"
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v2 06/10] cgroup: Assign subsystem IDs during compile time

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

We are able to safe some space when we assign the subsystem
IDs at compile time. Instead of allocating per cgroup
cgroup->subsys[CGROUP_SUBSYS_COUNT] where CGROUP_SUBSYS_COUNT is
always 64, we allocate at max 12 (at this point there are 12
subsystem).

task_cls_classid() and task_netprioidx() (when built as
module) are protected by a jump label and therefore we can
simply replace the subsystem index lookup with the enum.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Gao feng <gaofeng@cn.fujitsu.com>
Cc: Glauber Costa <glommer@parallels.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Cc: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: netdev@vger.kernel.org
Cc: cgroups@vger.kernel.org
---
 include/linux/cgroup.h        | 29 ++++++++++++++++++++---------
 include/linux/cgroup_subsys.h | 24 ++++++++++++------------
 include/net/cls_cgroup.h      | 12 +++---------
 include/net/netprio_cgroup.h  | 17 ++++-------------
 kernel/cgroup.c               | 25 ++++++-------------------
 net/core/netprio_cgroup.c     | 11 -----------
 net/core/sock.c               |  9 ---------
 net/sched/cls_cgroup.c        | 13 -------------
 8 files changed, 45 insertions(+), 95 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 3787872..ada517f 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -43,18 +43,27 @@ extern void cgroup_unload_subsys(struct cgroup_subsys *ss);
 
 extern const struct file_operations proc_cgroup_operations;
 
-/* Define the enumeration of all builtin cgroup subsystems */
-#define SUBSYS(_x) _x ## _subsys_id,
+/*
+ * Define the enumeration of all builtin cgroup subsystems.
+ * For the builtin subsystems the subsys_id needs to be indentical
+ * with the index in css->subsys. Therefore, all the builtin
+ * subsys are listed first and then the modules ids.
+ */
 enum cgroup_subsys_id {
+#define SUBSYS(_x) _x ## _subsys_id,
+
+#define IS_SUBSYS_ENABLED(option) IS_BUILTIN(option)
 #include <linux/cgroup_subsys.h>
-};
+#undef IS_SUBSYS_ENABLED
+
+#define IS_SUBSYS_ENABLED(option) IS_MODULE(option)
+#include <linux/cgroup_subsys.h>
+#undef IS_SUBSYS_ENABLED
+
 #undef SUBSYS
-/*
- * This define indicates the maximum number of subsystems that can be loaded
- * at once. We limit to this many since cgroupfs_root has subsys_bits to keep
- * track of all of them.
- */
-#define CGROUP_SUBSYS_COUNT (BITS_PER_BYTE*sizeof(unsigned long))
+
+	CGROUP_SUBSYS_COUNT,
+};
 
 /* Per-subsystem/per-cgroup state maintained by the system. */
 struct cgroup_subsys_state {
@@ -520,8 +529,10 @@ struct cgroup_subsys {
 };
 
 #define SUBSYS(_x) extern struct cgroup_subsys _x ## _subsys;
+#define IS_SUBSYS_ENABLED(option) IS_ENABLED(option)
 #include <linux/cgroup_subsys.h>
 #undef SUBSYS
+#undef IS_SUBSYS_ENABLED
 
 static inline struct cgroup_subsys_state *cgroup_subsys_state(
 	struct cgroup *cgrp, int subsys_id)
diff --git a/include/linux/cgroup_subsys.h b/include/linux/cgroup_subsys.h
index dfae957..f204a7a 100644
--- a/include/linux/cgroup_subsys.h
+++ b/include/linux/cgroup_subsys.h
@@ -7,73 +7,73 @@
 
 /* */
 
-#ifdef CONFIG_CPUSETS
+#if IS_SUBSYS_ENABLED(CONFIG_CPUSETS)
 SUBSYS(cpuset)
 #endif
 
 /* */
 
-#ifdef CONFIG_CGROUP_DEBUG
+#if IS_SUBSYS_ENABLED(CONFIG_CGROUP_DEBUG)
 SUBSYS(debug)
 #endif
 
 /* */
 
-#ifdef CONFIG_CGROUP_SCHED
+#if IS_SUBSYS_ENABLED(CONFIG_CGROUP_SCHED)
 SUBSYS(cpu_cgroup)
 #endif
 
 /* */
 
-#ifdef CONFIG_CGROUP_CPUACCT
+#if IS_SUBSYS_ENABLED(CONFIG_CGROUP_CPUACCT)
 SUBSYS(cpuacct)
 #endif
 
 /* */
 
-#ifdef CONFIG_MEMCG
+#if IS_SUBSYS_ENABLED(CONFIG_MEMCG)
 SUBSYS(mem_cgroup)
 #endif
 
 /* */
 
-#ifdef CONFIG_CGROUP_DEVICE
+#if IS_SUBSYS_ENABLED(CONFIG_CGROUP_DEVICE)
 SUBSYS(devices)
 #endif
 
 /* */
 
-#ifdef CONFIG_CGROUP_FREEZER
+#if IS_SUBSYS_ENABLED(CONFIG_CGROUP_FREEZER)
 SUBSYS(freezer)
 #endif
 
 /* */
 
-#ifdef CONFIG_NET_CLS_CGROUP
+#if IS_SUBSYS_ENABLED(CONFIG_NET_CLS_CGROUP)
 SUBSYS(net_cls)
 #endif
 
 /* */
 
-#ifdef CONFIG_BLK_CGROUP
+#if IS_SUBSYS_ENABLED(CONFIG_BLK_CGROUP)
 SUBSYS(blkio)
 #endif
 
 /* */
 
-#ifdef CONFIG_CGROUP_PERF
+#if IS_SUBSYS_ENABLED(CONFIG_CGROUP_PERF)
 SUBSYS(perf)
 #endif
 
 /* */
 
-#ifdef CONFIG_NETPRIO_CGROUP
+#if IS_SUBSYS_ENABLED(CONFIG_NETPRIO_CGROUP)
 SUBSYS(net_prio)
 #endif
 
 /* */
 
-#ifdef CONFIG_CGROUP_HUGETLB
+#if IS_SUBSYS_ENABLED(CONFIG_CGROUP_HUGETLB)
 SUBSYS(hugetlb)
 #endif
 
diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index 24443d2..43fae13 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -49,22 +49,16 @@ static inline u32 task_cls_classid(struct task_struct *p)
 extern struct static_key cgroup_cls_enabled;
 #define clscg_enabled static_key_false(&cgroup_cls_enabled)
 
-extern int net_cls_subsys_id;
-
 static inline u32 task_cls_classid(struct task_struct *p)
 {
-	int id;
-	u32 classid = 0;
+	u32 classid;
 
 	if (!clscg_enabled || in_interrupt())
 		return 0;
 
 	rcu_read_lock();
-	id = rcu_dereference_index_check(net_cls_subsys_id,
-					 rcu_read_lock_held());
-	if (id >= 0)
-		classid = container_of(task_subsys_state(p, id),
-				       struct cgroup_cls_state, css)->classid;
+	classid = container_of(task_subsys_state(p, net_cls_subsys_id),
+			       struct cgroup_cls_state, css)->classid;
 	rcu_read_unlock();
 
 	return classid;
diff --git a/include/net/netprio_cgroup.h b/include/net/netprio_cgroup.h
index 9ff58e4..66241c6 100644
--- a/include/net/netprio_cgroup.h
+++ b/include/net/netprio_cgroup.h
@@ -31,10 +31,6 @@ struct cgroup_netprio_state {
 	u32 prioidx;
 };
 
-#ifndef CONFIG_NETPRIO_CGROUP
-extern int net_prio_subsys_id;
-#endif
-
 extern void sock_update_netprioidx(struct sock *sk, struct task_struct *task);
 
 #if IS_BUILTIN(CONFIG_NETPRIO_CGROUP)
@@ -60,20 +56,15 @@ extern struct static_key cgroup_netprio_enabled;
 static inline u32 task_netprioidx(struct task_struct *p)
 {
 	struct cgroup_netprio_state *state;
-	int subsys_id;
-	u32 idx = 0;
+	u32 idx;
 
 	if (!netpriocg_enabled)
 		return 0;
 
 	rcu_read_lock();
-	subsys_id = rcu_dereference_index_check(net_prio_subsys_id,
-						rcu_read_lock_held());
-	if (subsys_id >= 0) {
-		state = container_of(task_subsys_state(p, subsys_id),
-				     struct cgroup_netprio_state, css);
-		idx = state->prioidx;
-	}
+	state = container_of(task_subsys_state(p, net_prio_subsys_id),
+			     struct cgroup_netprio_state, css);
+	idx = state->prioidx;
 	rcu_read_unlock();
 	return idx;
 }
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index f9433b4..ee55f84 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -93,8 +93,11 @@ static DEFINE_MUTEX(cgroup_root_mutex);
  * cgroup_mutex.
  */
 #define SUBSYS(_x) &_x ## _subsys,
+#define IS_SUBSYS_ENABLED(option) IS_BUILTIN(option)
 static struct cgroup_subsys *subsys[CGROUP_SUBSYS_COUNT] = {
 #include <linux/cgroup_subsys.h>
+#undef IS_SUBSYS_ENABLED
+#undef SUBSYS
 };
 
 #define MAX_CGROUP_ROOT_NAMELEN 64
@@ -4336,24 +4339,8 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
 	/* init base cftset */
 	cgroup_init_cftsets(ss);
 
-	/*
-	 * need to register a subsys id before anything else - for example,
-	 * init_cgroup_css needs it.
-	 */
 	mutex_lock(&cgroup_mutex);
-	/* find the first empty slot in the array */
-	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
-		if (subsys[i] == NULL)
-			break;
-	}
-	if (i == CGROUP_SUBSYS_COUNT) {
-		/* maximum number of subsystems already registered! */
-		mutex_unlock(&cgroup_mutex);
-		return -EBUSY;
-	}
-	/* assign ourselves the subsys_id */
-	ss->subsys_id = i;
-	subsys[i] = ss;
+	subsys[ss->subsys_id] = ss;
 
 	/*
 	 * no ss->create seems to need anything important in the ss struct, so
@@ -4362,7 +4349,7 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
 	css = ss->create(dummytop);
 	if (IS_ERR(css)) {
 		/* failure case - need to deassign the subsys[] slot. */
-		subsys[i] = NULL;
+		subsys[ss->subsys_id] = NULL;
 		mutex_unlock(&cgroup_mutex);
 		return PTR_ERR(css);
 	}
@@ -4378,7 +4365,7 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
 		if (ret) {
 			dummytop->subsys[ss->subsys_id] = NULL;
 			ss->destroy(dummytop);
-			subsys[i] = NULL;
+			subsys[ss->subsys_id] = NULL;
 			mutex_unlock(&cgroup_mutex);
 			return ret;
 		}
diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index 400ab94..a029ffe 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
 
 #if IS_MODULE(CONFIG_NETPRIO_CGROUP)
 	static_key_slow_inc(&cgroup_netprio_enabled);
@@ -390,11 +384,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();
-#endif
-
 #if IS_MODULE(CONFIG_NETPRIO_CGROUP)
 	static_key_slow_dec(&cgroup_netprio_enabled);
 	rcu_barrier();
diff --git a/net/core/sock.c b/net/core/sock.c
index 1f119d2..aa762d9 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -335,15 +335,6 @@ EXPORT_SYMBOL_GPL(cgroup_cls_enabled);
 struct static_key cgroup_netprio_enabled = STATIC_KEY_INIT_FALSE;
 EXPORT_SYMBOL_GPL(cgroup_netprio_enabled);
 #endif
-
-#if !defined(CONFIG_NET_CLS_CGROUP)
-int net_cls_subsys_id = -1;
-EXPORT_SYMBOL_GPL(net_cls_subsys_id);
-#endif
-#if !defined(CONFIG_NETPRIO_CGROUP)
-int net_prio_subsys_id = -1;
-EXPORT_SYMBOL_GPL(net_prio_subsys_id);
-#endif
 #endif
 
 static int sock_set_timeout(long *timeo_p, char __user *optval, int optlen)
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 554dc5b..a196b77 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -77,9 +77,7 @@ struct cgroup_subsys net_cls_subsys = {
 	.name		= "net_cls",
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
-#ifdef CONFIG_NET_CLS_CGROUP
 	.subsys_id	= net_cls_subsys_id,
-#endif
 	.base_cftypes	= ss_files,
 	.module		= THIS_MODULE,
 };
@@ -283,12 +281,6 @@ static int __init init_cgroup_cls(void)
 	if (ret)
 		goto out;
 
-#ifndef CONFIG_NET_CLS_CGROUP
-	/* We can't use rcu_assign_pointer because this is an int. */
-	smp_wmb();
-	net_cls_subsys_id = net_cls_subsys.subsys_id;
-#endif
-
 	ret = register_tcf_proto_ops(&cls_cgroup_ops);
 	if (ret)
 		cgroup_unload_subsys(&net_cls_subsys);
@@ -307,11 +299,6 @@ static void __exit exit_cgroup_cls(void)
 {
 	unregister_tcf_proto_ops(&cls_cgroup_ops);
 
-#ifndef CONFIG_NET_CLS_CGROUP
-	net_cls_subsys_id = -1;
-	synchronize_rcu();
-#endif
-
 #if IS_MODULE(CONFIG_NET_CLS_CGROUP)
 	static_key_slow_dec(&cgroup_cls_enabled);
 	rcu_barrier();
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v2 07/10] cgroup: net_cls: Simplify ifdef logic

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Jamal Hadi Salim <jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 include/net/cls_cgroup.h | 12 +++++-------
 net/core/sock.c          |  2 ++
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index 43fae13..5906a25 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -18,7 +18,7 @@
 #include <linux/rcupdate.h>
 #include <linux/jump_label.h>
 
-#ifdef CONFIG_CGROUPS
+#if IS_ENABLED(CONFIG_NET_CLS_CGROUP)
 struct cgroup_cls_state
 {
 	struct cgroup_subsys_state css;
@@ -64,21 +64,19 @@ static inline u32 task_cls_classid(struct task_struct *p)
 	return classid;
 }
 
-#else
+#endif
+
+#else /* !CONFIG_NET_CLS_CGROUP */
 
 static inline u32 task_cls_classid(struct task_struct *p)
 {
 	return 0;
 }
 
-#endif /* CONFIG_NET_CLS_CGROUP */
-
-#else /* !CONFIG_CGROUPS */
-
 static inline void sock_update_classid(struct sock *sk)
 {
 }
 
-#endif /* CONFIG_CGROURPS */
+#endif /* CONFIG_NET_CLS_CGROUP */
 
 #endif  /* _NET_CLS_CGROUP_H */
diff --git a/net/core/sock.c b/net/core/sock.c
index aa762d9..b1fadfd 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1223,6 +1223,7 @@ static void sk_prot_free(struct proto *prot, struct sock *sk)
 }
 
 #ifdef CONFIG_CGROUPS
+#if IS_ENABLED(CONFIG_NET_CLS_CGROUP)
 void sock_update_classid(struct sock *sk)
 {
 	u32 classid;
@@ -1234,6 +1235,7 @@ void sock_update_classid(struct sock *sk)
 		sk->sk_classid = classid;
 }
 EXPORT_SYMBOL(sock_update_classid);
+#endif
 
 void sock_update_netprioidx(struct sock *sk, struct task_struct *task)
 {
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v2 08/10] cgroup: net_cls: Merge builtin and module version of task_cls_classid()

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Gao feng <gaofeng@cn.fujitsu.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: netdev@vger.kernel.org
Cc: cgroups@vger.kernel.org
---
 include/net/cls_cgroup.h | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index 5906a25..d96dc59 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -29,26 +29,15 @@ extern void sock_update_classid(struct sock *sk);
 
 #if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
 
-static inline u32 task_cls_classid(struct task_struct *p)
-{
-	int classid;
-
-	if (in_interrupt())
-		return 0;
-
-	rcu_read_lock();
-	classid = container_of(task_subsys_state(p, net_cls_subsys_id),
-			       struct cgroup_cls_state, css)->classid;
-	rcu_read_unlock();
-
-	return classid;
-}
+#define clscg_enabled 1
 
 #elif IS_MODULE(CONFIG_NET_CLS_CGROUP)
 
 extern struct static_key cgroup_cls_enabled;
 #define clscg_enabled static_key_false(&cgroup_cls_enabled)
 
+#endif
+
 static inline u32 task_cls_classid(struct task_struct *p)
 {
 	u32 classid;
@@ -64,8 +53,6 @@ static inline u32 task_cls_classid(struct task_struct *p)
 	return classid;
 }
 
-#endif
-
 #else /* !CONFIG_NET_CLS_CGROUP */
 
 static inline u32 task_cls_classid(struct task_struct *p)
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v2 09/10] cgroup: net_prio: Simplify ifdef logic

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Jamal Hadi Salim <jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 include/net/netprio_cgroup.h | 14 +++++++-------
 net/core/sock.c              |  2 ++
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/net/netprio_cgroup.h b/include/net/netprio_cgroup.h
index 66241c6..ab22019 100644
--- a/include/net/netprio_cgroup.h
+++ b/include/net/netprio_cgroup.h
@@ -18,14 +18,14 @@
 #include <linux/rcupdate.h>
 #include <linux/jump_label.h>
 
+#if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
+
 struct netprio_map {
 	struct rcu_head rcu;
 	u32 priomap_len;
 	u32 priomap[];
 };
 
-#ifdef CONFIG_CGROUPS
-
 struct cgroup_netprio_state {
 	struct cgroup_subsys_state css;
 	u32 prioidx;
@@ -69,17 +69,17 @@ static inline u32 task_netprioidx(struct task_struct *p)
 	return idx;
 }
 
-#else
+#endif
+
+#else  /* !CONFIG_NETPRIO_CGROUP */
 
 static inline u32 task_netprioidx(struct task_struct *p)
 {
 	return 0;
 }
 
-#endif /* CONFIG_NETPRIO_CGROUP */
-
-#else
 #define sock_update_netprioidx(sk, task)
-#endif
+
+#endif  /* CONFIG_NETPRIO_CGROUP */
 
 #endif  /* _NET_CLS_CGROUP_H */
diff --git a/net/core/sock.c b/net/core/sock.c
index b1fadfd..115cd59 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1237,6 +1237,7 @@ void sock_update_classid(struct sock *sk)
 EXPORT_SYMBOL(sock_update_classid);
 #endif
 
+#if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
 void sock_update_netprioidx(struct sock *sk, struct task_struct *task)
 {
 	if (in_interrupt())
@@ -1246,6 +1247,7 @@ void sock_update_netprioidx(struct sock *sk, struct task_struct *task)
 }
 EXPORT_SYMBOL_GPL(sock_update_netprioidx);
 #endif
+#endif
 
 /**
  *	sk_alloc - All socket objects are allocated here
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v2 10/10] cgroup: net_prio: Merge builtin and module version of task_netprioidx()

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Jamal Hadi Salim <jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 include/net/netprio_cgroup.h | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/include/net/netprio_cgroup.h b/include/net/netprio_cgroup.h
index ab22019..180ff5e 100644
--- a/include/net/netprio_cgroup.h
+++ b/include/net/netprio_cgroup.h
@@ -35,24 +35,15 @@ extern void sock_update_netprioidx(struct sock *sk, struct task_struct *task);
 
 #if IS_BUILTIN(CONFIG_NETPRIO_CGROUP)
 
-static inline u32 task_netprioidx(struct task_struct *p)
-{
-	struct cgroup_netprio_state *state;
-	u32 idx;
-
-	rcu_read_lock();
-	state = container_of(task_subsys_state(p, net_prio_subsys_id),
-			     struct cgroup_netprio_state, css);
-	idx = state->prioidx;
-	rcu_read_unlock();
-	return idx;
-}
+#define netpriocg_enabled 1
 
 #elif IS_MODULE(CONFIG_NETPRIO_CGROUP)
 
 extern struct static_key cgroup_netprio_enabled;
 #define netpriocg_enabled static_key_false(&cgroup_netprio_enabled)
 
+#endif
+
 static inline u32 task_netprioidx(struct task_struct *p)
 {
 	struct cgroup_netprio_state *state;
@@ -69,8 +60,6 @@ static inline u32 task_netprioidx(struct task_struct *p)
 	return idx;
 }
 
-#endif
-
 #else  /* !CONFIG_NETPRIO_CGROUP */
 
 static inline u32 task_netprioidx(struct task_struct *p)
-- 
1.7.12.rc1.16.g05a20c8

Re: [PATCH v2 00/10] cgroup: Assign subsystem IDs during compile time

[Daniel Wagner]

On 24.08.2012 16:01, Daniel Wagner wrote:
> CGROUP_BUILTIN_SUBSYS_COUNT is also gone in this version.  This time I
> trade space for speed. Some extra cycles are spend to identify the
> modules in the for loops, e.g.
>
> for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> 	struct cgroup_subsys_state *ss = cgrp->subsys[i];
>
> 	/* at bootup time, we don't worry about modular subsystems */
> 	if (!ss || (ss && ss->module))
> 		continue;
>
> 	[...]
> }
>
> CGROUP_SUBSYS_COUNT is currently 12 if all controllers are built.  I
> haven't found any other way to get rid of CGROUP_BUILTIN_SUBSYS_COUNT
> without real dirty preprocessor tricks.

As usual, the good ideas come right after sending patches:

enum cgroup_subsys_id {
#define SUBSYS(_x) _x ## _subsys_id,

#define IS_SUBSYS_ENABLED(option) IS_BUILTIN(option)
#include <linux/cgroup_subsys.h>
#undef IS_SUBSYS_ENABLED

	CGROUP_BUILTIN_SUBSYS_COUNT,
	CGROUP_BUILTIN_SUBSYS_LAST = CGROUP_BUILTIN_SUBSYS_COUNT - 1,

#define IS_SUBSYS_ENABLED(option) IS_MODULE(option)
#include <linux/cgroup_subsys.h>
#undef IS_SUBSYS_ENABLED

#undef SUBSYS

	CGROUP_SUBSYS_COUNT,
};

Would that be an acceptable solution?

cheers,
daniel

Re: [PATCH v2 00/10] cgroup: Assign subsystem IDs during compile time

[Tejun Heo]

Hello, Daniel.

On Fri, Aug 24, 2012 at 04:01:34PM +0200, Daniel Wagner wrote:
> CGROUP_BUILTIN_SUBSYS_COUNT is also gone in this version.  This time I
> trade space for speed. Some extra cycles are spend to identify the
> modules in the for loops, e.g.

There's no point in optimizing either space or speed here.  It's not a
hot path by any stretch of imagination.  Please focus on making it
simple.

Thanks.

-- 
tejun

Re: [PATCH v2 01/10] cgroup: net_cls: Use empty task_cls_classid() when !CONFIG_NET_CLS(_MODULE)

[Tejun Heo]

On Fri, Aug 24, 2012 at 04:01:35PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

How about,

	When CONFIG_NET_CLS_CGROUP is N, task_cls_classid() doesn't
	have any user but the module version still gets defined.  Move
	it inside IS_MODULE(CONFIG_NET_CLS_CGROUP).

Thanks.

-- 
tejun

Re: [PATCH v2 03/10] cgroup: net_cls: Protect access to task_cls_classid() when built as module

[Tejun Heo]

On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote:
> @@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void)
>  	synchronize_rcu();
>  #endif
>  
> +#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
> +	static_key_slow_dec(&cgroup_cls_enabled);
> +	rcu_barrier();

Why is this rcu_barrier() necessary?  In general, please explain what
synchronization is going on when using sync constructs which aren't
obvious - e.g. memory barriers, rcu barriers.

Thanks.

-- 
tejun

Re: [PATCH v2 04/10] cgroup: net_prio: Protect access to task_netprioidx() when built as module

[Tejun Heo]

On Fri, Aug 24, 2012 at 04:01:38PM +0200, Daniel Wagner wrote:
> +#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
> +	static_key_slow_dec(&cgroup_netprio_enabled);
> +	rcu_barrier();

Ditto as patch 3.

-- 
tejun

Re: [PATCH v2 05/10] cgroup: Remove CGROUP_BUILTIN_SUBSYS_COUNT

[Tejun Heo]

On Fri, Aug 24, 2012 at 04:01:39PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> 
> CGROUP_BUILTIN_SUBSYS_COUNT is used as start or stop point
> when looping over the subsys array. Since the subsys array is
> 64 entries long this is a good thing to do. Though we'd like
> to reduce the array size considerable but we need to get rid
> of CGROUP_BUILTIN_SUBSYS_COUNT to ease up the review process.

Wouldn't it be better to explicitly state that a following patch would
reduce the SUBSYS_COUNT and stop putting builtin and module ones into
different sections?

Thanks.

-- 
tejun

Re: [PATCH v2 06/10] cgroup: Assign subsystem IDs during compile time

[Tejun Heo]

Hello,

On Fri, Aug 24, 2012 at 04:01:40PM +0200, Daniel Wagner wrote:
> We are able to safe some space when we assign the subsystem
                 ^               ^
		 save    if we assign both builtin and
		         module susbsystem IDs at compile time?

> IDs at compile time. Instead of allocating per cgroup
> cgroup->subsys[CGROUP_SUBSYS_COUNT] where CGROUP_SUBSYS_COUNT is
> always 64, we allocate at max 12 (at this point there are 12
> subsystem).

Please note (in big fat ugly way) that this disallows support for
modular controller which isn't known at kernel compile time.

> task_cls_classid() and task_netprioidx() (when built as
> module) are protected by a jump label and therefore we can
> simply replace the subsystem index lookup with the enum.

Can we put these in a separate patch?  ie. The first patch makes all
subsys IDs constant and then patches to simplify users.

> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 3787872..ada517f 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -43,18 +43,27 @@ extern void cgroup_unload_subsys(struct cgroup_subsys *ss);
>  
>  extern const struct file_operations proc_cgroup_operations;
>  
> -/* Define the enumeration of all builtin cgroup subsystems */
> -#define SUBSYS(_x) _x ## _subsys_id,
> +/*
> + * Define the enumeration of all builtin cgroup subsystems.
> + * For the builtin subsystems the subsys_id needs to be indentical
> + * with the index in css->subsys. Therefore, all the builtin
> + * subsys are listed first and then the modules ids.
> + */
>  enum cgroup_subsys_id {
> +#define SUBSYS(_x) _x ## _subsys_id,
> +
> +#define IS_SUBSYS_ENABLED(option) IS_BUILTIN(option)
>  #include <linux/cgroup_subsys.h>
> -};
> +#undef IS_SUBSYS_ENABLED
> +
> +#define IS_SUBSYS_ENABLED(option) IS_MODULE(option)
> +#include <linux/cgroup_subsys.h>
> +#undef IS_SUBSYS_ENABLED
> +

Why do we need to segregate in-kernel and modular ones at all?  What's
wrong with just defining them in one go?

> diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
> index 24443d2..43fae13 100644
> --- a/include/net/cls_cgroup.h
> +++ b/include/net/cls_cgroup.h
> @@ -49,22 +49,16 @@ static inline u32 task_cls_classid(struct task_struct *p)
>  extern struct static_key cgroup_cls_enabled;
>  #define clscg_enabled static_key_false(&cgroup_cls_enabled)
>  
> -extern int net_cls_subsys_id;
> -
>  static inline u32 task_cls_classid(struct task_struct *p)
>  {
> -	int id;
> -	u32 classid = 0;
> +	u32 classid;
>  
>  	if (!clscg_enabled || in_interrupt())
>  		return 0;
>  
>  	rcu_read_lock();
> -	id = rcu_dereference_index_check(net_cls_subsys_id,
> -					 rcu_read_lock_held());
> -	if (id >= 0)
> -		classid = container_of(task_subsys_state(p, id),
> -				       struct cgroup_cls_state, css)->classid;
> +	classid = container_of(task_subsys_state(p, net_cls_subsys_id),
> +			       struct cgroup_cls_state, css)->classid;
>  	rcu_read_unlock();
>  
>  	return classid;

Hmm... patch sequence looks odd to me.  If you first make all IDs
constant, you can first remove module specific ones and then later add
jump labels as separate patches.  Wouldn't that be simpler?

Thanks.

-- 
tejun

Re: [PATCH v2 07/10] cgroup: net_cls: Simplify ifdef logic

[Tejun Heo]

On Fri, Aug 24, 2012 at 04:01:41PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

I suppose this is equivalent conversion?  Maybe it would be nice to
note that in patch description?

Thanks.

-- 
tejun

Re: [PATCH v2 08/10] cgroup: net_cls: Merge builtin and module version of task_cls_classid()

[Tejun Heo]

On Fri, Aug 24, 2012 at 04:01:42PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

How about,

	Now that all cgroup subsys IDs are constant whether a subsys
	is built-in or modular, there's no reason to have separate
	versions of task_cls_classid().  Unify them.

-- 
tejun

Re: [PATCH v2 09/10] cgroup: net_prio: Simplify ifdef logic

[Tejun Heo]

On Fri, Aug 24, 2012 at 04:01:43PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

Okay, for a few trivial patches, $SUBJ as description is fine but this
series is pushing it too far.  Is this equivalent conversion?  Why
does sock_update_netprioidx() earn an addition ifdef around it?  :(

-- 
tejun

Re: [PATCH v2 00/10] cgroup: Assign subsystem IDs during compile time

[Daniel Wagner]

Hi Tejun,

On 25.08.2012 01:15, Tejun Heo wrote:
> Hello, Daniel.
>
> On Fri, Aug 24, 2012 at 04:01:34PM +0200, Daniel Wagner wrote:
>> CGROUP_BUILTIN_SUBSYS_COUNT is also gone in this version.  This time I
>> trade space for speed. Some extra cycles are spend to identify the
>> modules in the for loops, e.g.
>
> There's no point in optimizing either space or speed here.  It's not a
> hot path by any stretch of imagination.  Please focus on making it
> simple.

Will do. Thanks a lot for your feedback. I'll update the series accordingly.

cheers,
daniel

Re: [PATCH v2 03/10] cgroup: net_cls: Protect access to task_cls_classid() when built as module

[Daniel Wagner]

On 25.08.2012 01:26, Tejun Heo wrote:
> On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote:
>> @@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void)
>>   	synchronize_rcu();
>>   #endif
>>
>> +#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
>> +	static_key_slow_dec(&cgroup_cls_enabled);
>> +	rcu_barrier();
>
> Why is this rcu_barrier() necessary?

I have read the rcubarrier.txt document and I got from that that an 
rcu_barrier() is needed when unloading a module. But maybe I got it wrong.

So the idea after disabling the jump lables all pending readers in
task_cls_classid() have left. THe same thing is done in the old code 
with the dynamic id part. With the difference that synchronize_rcu() is 
used.

> In general, please explain what
> synchronization is going on when using sync constructs which aren't
> obvious - e.g. memory barriers, rcu barriers.

Sure, I will keep this in mind.

Re: [PATCH v2 05/10] cgroup: Remove CGROUP_BUILTIN_SUBSYS_COUNT

[Daniel Wagner]

On 25.08.2012 01:28, Tejun Heo wrote:
> On Fri, Aug 24, 2012 at 04:01:39PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>>
>> CGROUP_BUILTIN_SUBSYS_COUNT is used as start or stop point
>> when looping over the subsys array. Since the subsys array is
>> 64 entries long this is a good thing to do. Though we'd like
>> to reduce the array size considerable but we need to get rid
>> of CGROUP_BUILTIN_SUBSYS_COUNT to ease up the review process.
>
> Wouldn't it be better to explicitly state that a following patch would
> reduce the SUBSYS_COUNT and stop putting builtin and module ones into
> different sections?

Sure, can do that. I just to make sure I understand you correctly,
What do you mean with different section? Do you refer to the enum sorting?

Re: [PATCH v2 06/10] cgroup: Assign subsystem IDs during compile time

[Daniel Wagner]

On 25.08.2012 01:38, Tejun Heo wrote:
> Hello,
>
> On Fri, Aug 24, 2012 at 04:01:40PM +0200, Daniel Wagner wrote:
>> We are able to safe some space when we assign the subsystem
>                   ^               ^
> 		 save    if we assign both builtin and
> 		         module susbsystem IDs at compile time?
>
>> IDs at compile time. Instead of allocating per cgroup
>> cgroup->subsys[CGROUP_SUBSYS_COUNT] where CGROUP_SUBSYS_COUNT is
>> always 64, we allocate at max 12 (at this point there are 12
>> subsystem).
>
> Please note (in big fat ugly way) that this disallows support for
> modular controller which isn't known at kernel compile time.

yep, will do

>> task_cls_classid() and task_netprioidx() (when built as
>> module) are protected by a jump label and therefore we can
>> simply replace the subsystem index lookup with the enum.
>
> Can we put these in a separate patch?  ie. The first patch makes all
> subsys IDs constant and then patches to simplify users.

Wouldn't this break bisection? I merged this step so that all steps in 
this series are able to compile and run.

>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index 3787872..ada517f 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -43,18 +43,27 @@ extern void cgroup_unload_subsys(struct cgroup_subsys *ss);
>>
>>   extern const struct file_operations proc_cgroup_operations;
>>
>> -/* Define the enumeration of all builtin cgroup subsystems */
>> -#define SUBSYS(_x) _x ## _subsys_id,
>> +/*
>> + * Define the enumeration of all builtin cgroup subsystems.
>> + * For the builtin subsystems the subsys_id needs to be indentical
>> + * with the index in css->subsys. Therefore, all the builtin
>> + * subsys are listed first and then the modules ids.
>> + */
>>   enum cgroup_subsys_id {
>> +#define SUBSYS(_x) _x ## _subsys_id,
>> +
>> +#define IS_SUBSYS_ENABLED(option) IS_BUILTIN(option)
>>   #include <linux/cgroup_subsys.h>
>> -};
>> +#undef IS_SUBSYS_ENABLED
>> +
>> +#define IS_SUBSYS_ENABLED(option) IS_MODULE(option)
>> +#include <linux/cgroup_subsys.h>
>> +#undef IS_SUBSYS_ENABLED
>> +
>
> Why do we need to segregate in-kernel and modular ones at all?  What's
> wrong with just defining them in one go?

I have done that but the result was a panic. There seems some code which 
expects this ordering. Let me dig into this and fix it.

>> diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
>> index 24443d2..43fae13 100644
>> --- a/include/net/cls_cgroup.h
>> +++ b/include/net/cls_cgroup.h
>> @@ -49,22 +49,16 @@ static inline u32 task_cls_classid(struct task_struct *p)
>>   extern struct static_key cgroup_cls_enabled;
>>   #define clscg_enabled static_key_false(&cgroup_cls_enabled)
>>
>> -extern int net_cls_subsys_id;
>> -
>>   static inline u32 task_cls_classid(struct task_struct *p)
>>   {
>> -	int id;
>> -	u32 classid = 0;
>> +	u32 classid;
>>
>>   	if (!clscg_enabled || in_interrupt())
>>   		return 0;
>>
>>   	rcu_read_lock();
>> -	id = rcu_dereference_index_check(net_cls_subsys_id,
>> -					 rcu_read_lock_held());
>> -	if (id >= 0)
>> -		classid = container_of(task_subsys_state(p, id),
>> -				       struct cgroup_cls_state, css)->classid;
>> +	classid = container_of(task_subsys_state(p, net_cls_subsys_id),
>> +			       struct cgroup_cls_state, css)->classid;
>>   	rcu_read_unlock();
>>
>>   	return classid;
>
> Hmm... patch sequence looks odd to me.  If you first make all IDs
> constant, you can first remove module specific ones and then later add
> jump labels as separate patches.  Wouldn't that be simpler?

As said above, I tried to keep all steps usable so bisection would work. 
I think your steps would lead to non working versions of the kernel.

Re: [PATCH v2 01/10] cgroup: net_cls: Use empty task_cls_classid() when !CONFIG_NET_CLS(_MODULE)

[Li Zefan]

task_cls_classid() is called by sock_update_classid().

#ifdef CONFIG_CGROUPS
extern void sock_update_classid(struct sock *sk);
#else
static inline void sock_update_classid(struct sock *sk)
{
}
#endif

Change to define sock_update_classid() only when CONFIG_NET_CLS is enabled, and
then we don't need this patch, and the code will be simpler.

On 2012/8/24 22:01, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Gao feng <gaofeng@cn.fujitsu.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Cc: Li Zefan <lizefan@huawei.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: netdev@vger.kernel.org
> Cc: cgroups@vger.kernel.org
> ---
>  include/net/cls_cgroup.h | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
> index a4dc5b0..e2fe2b9 100644
> --- a/include/net/cls_cgroup.h
> +++ b/include/net/cls_cgroup.h
> @@ -24,7 +24,8 @@ struct cgroup_cls_state
>  	u32 classid;
>  };
>  
> -#ifdef CONFIG_NET_CLS_CGROUP
> +#if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
> +
>  static inline u32 task_cls_classid(struct task_struct *p)
>  {
>  	int classid;
> @@ -39,7 +40,9 @@ static inline u32 task_cls_classid(struct task_struct *p)
>  
>  	return classid;
>  }
> -#else
> +
> +#elif IS_MODULE(CONFIG_NET_CLS_CGROUP)
> +
>  extern int net_cls_subsys_id;
>  
>  static inline u32 task_cls_classid(struct task_struct *p)
> @@ -60,11 +63,16 @@ static inline u32 task_cls_classid(struct task_struct *p)
>  
>  	return classid;
>  }
> -#endif
> +
>  #else
> +
>  static inline u32 task_cls_classid(struct task_struct *p)
>  {
>  	return 0;
>  }
> -#endif
> +
> +#endif /* CONFIG_NET_CLS_CGROUP */
> +
> +#endif /* CONFIG_CGROURPS */
> +
>  #endif  /* _NET_CLS_CGROUP_H */
> 

Re: [PATCH v2 03/10] cgroup: net_cls: Protect access to task_cls_classid() when built as module

[Paul E. McKenney]

On Sat, Aug 25, 2012 at 06:56:29PM +0200, Daniel Wagner wrote:
> On 25.08.2012 01:26, Tejun Heo wrote:
> >On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote:
> >>@@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void)
> >>  	synchronize_rcu();
> >>  #endif
> >>
> >>+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
> >>+	static_key_slow_dec(&cgroup_cls_enabled);
> >>+	rcu_barrier();
> >
> >Why is this rcu_barrier() necessary?
> 
> I have read the rcubarrier.txt document and I got from that that an
> rcu_barrier() is needed when unloading a module. But maybe I got it
> wrong.
> 
> So the idea after disabling the jump lables all pending readers in
> task_cls_classid() have left. THe same thing is done in the old code
> with the dynamic id part. With the difference that synchronize_rcu()
> is used.

FWIW, the rcu_barrier() is needed only if the module uses call_rcu().
In that case it is required to ensure that all the resulting callbacks
execute before the module's .text is freed up.

							Thanx, Paul

> >In general, please explain what
> >synchronization is going on when using sync constructs which aren't
> >obvious - e.g. memory barriers, rcu barriers.
> 
> Sure, I will keep this in mind.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 03/10] cgroup: net_cls: Protect access to task_cls_classid() when built as module

[Daniel Wagner]

On Tue, Aug 28, 2012 at 07:47:25AM -0700, Paul E. McKenney wrote:
> On Sat, Aug 25, 2012 at 06:56:29PM +0200, Daniel Wagner wrote:
> > On 25.08.2012 01:26, Tejun Heo wrote:
> > >On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote:
> > >>@@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void)
> > >>  	synchronize_rcu();
> > >>  #endif
> > >>
> > >>+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
> > >>+	static_key_slow_dec(&cgroup_cls_enabled);
> > >>+	rcu_barrier();
> > >
> > >Why is this rcu_barrier() necessary?
> > 
> > I have read the rcubarrier.txt document and I got from that that an
> > rcu_barrier() is needed when unloading a module. But maybe I got it
> > wrong.
> > 
> > So the idea after disabling the jump lables all pending readers in
> > task_cls_classid() have left. THe same thing is done in the old code
> > with the dynamic id part. With the difference that synchronize_rcu()
> > is used.
> 
> FWIW, the rcu_barrier() is needed only if the module uses call_rcu().
> In that case it is required to ensure that all the resulting callbacks
> execute before the module's .text is freed up.

Thanks for the clarification. After reading the documentations again
and I think this here should do the trick:

static inline u32 task_cls_classid(struct task_struct *p)
{
	struct cgroup_cls_state *cs;
	u32 classid = 0;

	if (!clscg_enabled || in_interrupt())
		return 0;

	rcu_read_lock();
	cs = container_of(task_subsys_state(p, net_cls_subsys_id),
			struct cgroup_cls_state, css);
	if (cs)
		classid = cs->classid;
	rcu_read_unlock();

	return classid;
}

static void __exit exit_cgroup_cls(void)
{
	unregister_tcf_proto_ops(&cls_cgroup_ops);

#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
	static_key_slow_dec(&cgroup_cls_enabled);
	synchronize_rcu();
#endif

	cgroup_unload_subsys(&net_cls_subsys);
}

So when unloading the module, we first disable the static branch, then
we wait for all old readers leaving the reader side issuing a
synchronize_rcu(). New readers might already have passed the static
branch and now entering the reader side. So we need to test if the
pointer we retrieve is valid. Basically, this change is using the same
approach we had before. Instead looking at the id is valid we look at
the pointer if it is valid.

Re: [PATCH v2 05/10] cgroup: Remove CGROUP_BUILTIN_SUBSYS_COUNT

[Tejun Heo]

Hello, Daniel.

On Sat, Aug 25, 2012 at 06:59:25PM +0200, Daniel Wagner wrote:
> >Wouldn't it be better to explicitly state that a following patch would
> >reduce the SUBSYS_COUNT and stop putting builtin and module ones into
> >different sections?
> 
> Sure, can do that. I just to make sure I understand you correctly,
> What do you mean with different section? Do you refer to the enum sorting?

I meant that there's no reason to have all builtin ones contiguosly
and then the module ones using two cgroup_subsys.h inclusions.

Thanks!

-- 
tejun

Re: [PATCH v2 06/10] cgroup: Assign subsystem IDs during compile time

[Tejun Heo]

Hello, Daniel.

On Sat, Aug 25, 2012 at 07:11:31PM +0200, Daniel Wagner wrote:
> On 25.08.2012 01:38, Tejun Heo wrote:
> >>task_cls_classid() and task_netprioidx() (when built as
> >>module) are protected by a jump label and therefore we can
> >>simply replace the subsystem index lookup with the enum.
> >
> >Can we put these in a separate patch?  ie. The first patch makes all
> >subsys IDs constant and then patches to simplify users.
> 
> Wouldn't this break bisection? I merged this step so that all steps
> in this series are able to compile and run.

I don't see why it should but maybe I'm missing something.  If so,
please enlighten me.

> >>+#define IS_SUBSYS_ENABLED(option) IS_MODULE(option)
> >>+#include <linux/cgroup_subsys.h>
> >>+#undef IS_SUBSYS_ENABLED
> >>+
> >
> >Why do we need to segregate in-kernel and modular ones at all?  What's
> >wrong with just defining them in one go?
> 
> I have done that but the result was a panic. There seems some code
> which expects this ordering. Let me dig into this and fix it.

Yes, please.

> >Hmm... patch sequence looks odd to me.  If you first make all IDs
> >constant, you can first remove module specific ones and then later add
> >jump labels as separate patches.  Wouldn't that be simpler?
> 
> As said above, I tried to keep all steps usable so bisection would
> work. I think your steps would lead to non working versions of the
> kernel.

Hmmm... Yes, it should be bisectable but again I don't see why being
biesectable interferes with the patch ordering here.

Thanks.

-- 
tejun