[PATCH v3 0/6] cgroup cls & netprio 'cleanups'

[PATCH v3 0/6] cgroup cls & netprio 'cleanups'

[Daniel Wagner]

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

Hi,

Sorry for the delay on updating this series. The usual
excuse apply here.

I saw that John is busy improving net_prio so I took the
liberty to port his changes to net_cls (#1-3). Patch #3 will
collide with John's unapplied patches. I am happy
to rebase this series if needed. 

Patch #4 and #5 improve the readability with using
IS_MODULE/BUILTIN macros. This patches prepare the last
patch.

Patch #6 removes support for assigning subsystem IDs during
runtime. As it turns out this is not really needed. By doing
so we are able to free some unused memory.

The patches are against net-next.

cheers,
daniel

Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@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: Tim Chen <tim.c.chen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Daniel Wagner (6):
  cgroup: Move cls function definition to cls_cgroup.h
  cgroup: net_cls rework update socket logic
  cgroup: Update classid for fd pass in SCM_RIGHTS datagramm
  cgroup: Use IS_MODULE/BUITLIN for net_cls
  cgroup: Use IS_MODULE/BUITLIN for net_prio
  cgroup: Assign subsystem IDs during compile time

 include/linux/cgroup.h        | 20 ++++++++++------
 include/linux/cgroup_subsys.h | 24 +++++++++----------
 include/net/cls_cgroup.h      | 36 +++++++++++-----------------
 include/net/netprio_cgroup.h  | 38 +++++++----------------------
 include/net/sock.h            |  8 -------
 kernel/cgroup.c               | 31 +++++++-----------------
 net/core/netprio_cgroup.c     | 19 ++++++---------
 net/core/scm.c                |  7 +++++-
 net/core/sock.c               | 29 ++++++++++++----------
 net/sched/cls_cgroup.c        | 56 +++++++++++++++++++++++++++++++++----------
 net/socket.c                  |  8 -------
 11 files changed, 129 insertions(+), 147 deletions(-)

-- 
1.7.12.rc1.16.g05a20c8

[PATCH v3 1/6] cgroup: Move cls function definition to cls_cgroup.h

[Daniel Wagner]

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

sock_update_classid() is used by net/socket.c which includes
cls_cgroup.h directly.

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

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index a4dc5b0..5f49b69 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);
+
 #ifdef CONFIG_NET_CLS_CGROUP
 static inline u32 task_cls_classid(struct task_struct *p)
 {
@@ -62,9 +64,14 @@ static inline u32 task_cls_classid(struct task_struct *p)
 }
 #endif
 #else
+static inline void sock_update_classid(struct sock *sk)
+{
+}
+
 static inline u32 task_cls_classid(struct task_struct *p)
 {
 	return 0;
 }
 #endif
+
 #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 v3 4/6] cgroup: Use IS_MODULE/BUITLIN for net_cls

[Daniel Wagner]

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

Instead of using #ifdef variation for building the net_cls
in the different configuration use explicit IS_MODULE and
IS_BUILTIN macros.

This allows to reorder in readable way the cls_cgroup.h header file
so that the common definition of the struct is first followed
by the two different version for task_cls_classid().

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Glauber Costa <glommer@parallels.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/net/cls_cgroup.h | 32 ++++++++++++++++++++------------
 net/core/sock.c          |  4 +++-
 net/sched/cls_cgroup.c   |  6 +++---
 3 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index fc05f43..865ea49 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -17,7 +17,8 @@
 #include <linux/hardirq.h>
 #include <linux/rcupdate.h>
 
-#ifdef CONFIG_CGROUPS
+#if IS_ENABLED(CONFIG_NET_CLS_CGROUP)
+
 struct cgroup_cls_state
 {
 	struct cgroup_subsys_state css;
@@ -26,7 +27,21 @@ struct cgroup_cls_state
 
 extern void sock_update_classid(struct sock *sk, struct task_struct *task);
 
-#ifdef CONFIG_NET_CLS_CGROUP
+#else
+
+static inline void sock_update_classid(struct sock *sk, struct task_struct *task)
+{
+}
+
+static inline u32 task_cls_classid(struct task_struct *p)
+{
+	return 0;
+}
+
+#endif
+
+#if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
+
 static inline u32 task_cls_classid(struct task_struct *p)
 {
 	int classid;
@@ -38,7 +53,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)
@@ -56,16 +73,7 @@ static inline u32 task_cls_classid(struct task_struct *p)
 
 	return classid;
 }
-#endif
-#else
-static inline void sock_update_classid(struct sock *sk, struct task_struct *task)
-{
-}
 
-static inline u32 task_cls_classid(struct task_struct *p)
-{
-	return 0;
-}
 #endif
 
 #endif  /* _NET_CLS_CGROUP_H */
diff --git a/net/core/sock.c b/net/core/sock.c
index e08df6b..d2d920e 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -327,7 +327,7 @@ int __sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
 EXPORT_SYMBOL(__sk_backlog_rcv);
 
 #if defined(CONFIG_CGROUPS)
-#if !defined(CONFIG_NET_CLS_CGROUP)
+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
 int net_cls_subsys_id = -1;
 EXPORT_SYMBOL_GPL(net_cls_subsys_id);
 #endif
@@ -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, struct task_struct *task)
 {
 	u32 classid;
@@ -1235,6 +1236,7 @@ void sock_update_classid(struct sock *sk, struct task_struct *task)
 		sk->sk_classid = classid;
 }
 EXPORT_SYMBOL(sock_update_classid);
+#endif
 
 void sock_update_netprioidx(struct sock *sk, struct task_struct *task)
 {
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 3535771..9ffe9b5 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -115,7 +115,7 @@ struct cgroup_subsys net_cls_subsys = {
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
 	.attach		= cgrp_attach,
-#ifdef CONFIG_NET_CLS_CGROUP
+#if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
 	.subsys_id	= net_cls_subsys_id,
 #endif
 	.base_cftypes	= ss_files,
@@ -321,7 +321,7 @@ static int __init init_cgroup_cls(void)
 	if (ret)
 		goto out;
 
-#ifndef CONFIG_NET_CLS_CGROUP
+#if IS_MODULE(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;
@@ -339,7 +339,7 @@ static void __exit exit_cgroup_cls(void)
 {
 	unregister_tcf_proto_ops(&cls_cgroup_ops);
 
-#ifndef CONFIG_NET_CLS_CGROUP
+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
 	net_cls_subsys_id = -1;
 	synchronize_rcu();
 #endif
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v3 5/6] cgroup: Use IS_MODULE/BUITLIN for net_prio

[Daniel Wagner]

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

Instead of using #ifdef variation for building the net_prio
in the different configuration use explicit IS_MODULE and
IS_BUILTIN macros.

This allows to reorder in readable way the netprio_cgroup.h header file
so that the common definition of the struct is first followed
by the two different version for task_netprioidx().

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Glauber Costa <glommer@parallels.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/net/netprio_cgroup.h | 33 ++++++++++++++++-----------------
 net/core/netprio_cgroup.c    |  6 +++---
 net/core/sock.c              |  4 +++-
 3 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/include/net/netprio_cgroup.h b/include/net/netprio_cgroup.h
index 2719dec..c43461e 100644
--- a/include/net/netprio_cgroup.h
+++ b/include/net/netprio_cgroup.h
@@ -17,6 +17,7 @@
 #include <linux/hardirq.h>
 #include <linux/rcupdate.h>
 
+#if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
 
 struct netprio_map {
 	struct rcu_head rcu;
@@ -24,19 +25,26 @@ struct netprio_map {
 	u32 priomap[];
 };
 
-#ifdef CONFIG_CGROUPS
-
 struct cgroup_netprio_state {
 	struct cgroup_subsys_state css;
 	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);
 
+#else
+
+static inline void sock_update_netprioidx(struct sock *sk, struct task_struct *task)
+{
+}
+
+static inline u32 task_netprioidx(struct task_struct *p)
+{
+	return 0;
+}
+
+#endif
+
 #if IS_BUILTIN(CONFIG_NETPRIO_CGROUP)
 
 static inline u32 task_netprioidx(struct task_struct *p)
@@ -54,6 +62,8 @@ static inline u32 task_netprioidx(struct task_struct *p)
 
 #elif IS_MODULE(CONFIG_NETPRIO_CGROUP)
 
+extern int net_prio_subsys_id;
+
 static inline u32 task_netprioidx(struct task_struct *p)
 {
 	struct cgroup_netprio_state *state;
@@ -72,17 +82,6 @@ static inline u32 task_netprioidx(struct task_struct *p)
 	return idx;
 }
 
-#else
-
-static inline u32 task_netprioidx(struct task_struct *p)
-{
-	return 0;
-}
-
 #endif /* CONFIG_NETPRIO_CGROUP */
 
-#else
-#define sock_update_netprioidx(sk, task)
-#endif
-
 #endif  /* _NET_CLS_CGROUP_H */
diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index 98478aa..238ece6 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -342,7 +342,7 @@ struct cgroup_subsys net_prio_subsys = {
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
 	.attach		= net_prio_attach,
-#ifdef CONFIG_NETPRIO_CGROUP
+#if IS_BUILTIN(CONFIG_NETPRIO_CGROUP)
 	.subsys_id	= net_prio_subsys_id,
 #endif
 	.base_cftypes	= ss_files,
@@ -382,7 +382,7 @@ static int __init init_cgroup_netprio(void)
 	ret = cgroup_load_subsys(&net_prio_subsys);
 	if (ret)
 		goto out;
-#ifndef CONFIG_NETPRIO_CGROUP
+#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
 	smp_wmb();
 	net_prio_subsys_id = net_prio_subsys.subsys_id;
 #endif
@@ -402,7 +402,7 @@ static void __exit exit_cgroup_netprio(void)
 
 	cgroup_unload_subsys(&net_prio_subsys);
 
-#ifndef CONFIG_NETPRIO_CGROUP
+#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
 	net_prio_subsys_id = -1;
 	synchronize_rcu();
 #endif
diff --git a/net/core/sock.c b/net/core/sock.c
index d2d920e..bdd06af 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -331,7 +331,7 @@ EXPORT_SYMBOL(__sk_backlog_rcv);
 int net_cls_subsys_id = -1;
 EXPORT_SYMBOL_GPL(net_cls_subsys_id);
 #endif
-#if !defined(CONFIG_NETPRIO_CGROUP)
+#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
 int net_prio_subsys_id = -1;
 EXPORT_SYMBOL_GPL(net_prio_subsys_id);
 #endif
@@ -1238,6 +1238,7 @@ void sock_update_classid(struct sock *sk, struct task_struct *task)
 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())
@@ -1247,6 +1248,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 v3 6/6] 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 12 + 1 at max (at this point there are 12
subsystem). The additinal one is the price we have to pay to
distinguish between builtin and module subsystems.

We should only access task_cls_classid() and task_netprioidx()
if the subsystem is ready to be used using jump labels for this.

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        | 20 +++++++++++++-------
 include/linux/cgroup_subsys.h | 24 ++++++++++++------------
 include/net/cls_cgroup.h      | 41 ++++++++++++-----------------------------
 include/net/netprio_cgroup.h  | 43 ++++++++++++-------------------------------
 kernel/cgroup.c               | 31 +++++++++----------------------
 net/core/netprio_cgroup.c     | 17 ++++++-----------
 net/core/sock.c               | 14 ++++++--------
 net/sched/cls_cgroup.c        | 18 +++++-------------
 8 files changed, 75 insertions(+), 133 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index c90eaa8..995739f 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -46,16 +46,20 @@ extern const struct file_operations proc_cgroup_operations;
 /* Define the enumeration of all builtin cgroup subsystems */
 #define SUBSYS(_x) _x ## _subsys_id,
 enum cgroup_subsys_id {
+
+#define IS_SUBSYS_ENABLED(option) IS_BUILTIN(option)
+#include <linux/cgroup_subsys.h>
+#undef IS_SUBSYS_ENABLED
+
+	CGROUP_BUILTIN_SUBSYS_COUNT,
+
+#define IS_SUBSYS_ENABLED(option) IS_MODULE(option)
 #include <linux/cgroup_subsys.h>
-	CGROUP_BUILTIN_SUBSYS_COUNT
+#undef IS_SUBSYS_ENABLED
+
+	CGROUP_SUBSYS_COUNT
 };
 #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))
 
 /* Per-subsystem/per-cgroup state maintained by the system. */
 struct cgroup_subsys_state {
@@ -521,7 +525,9 @@ 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 IS_SUBSYS_ENABLED
 #undef SUBSYS
 
 static inline struct cgroup_subsys_state *cgroup_subsys_state(
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 865ea49..7834fd7 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>
 
 #if IS_ENABLED(CONFIG_NET_CLS_CGROUP)
 
@@ -25,27 +26,18 @@ struct cgroup_cls_state
 	u32 classid;
 };
 
-extern void sock_update_classid(struct sock *sk, struct task_struct *task);
-
-#else
-
-static inline void sock_update_classid(struct sock *sk, struct task_struct *task)
-{
-}
-
-static inline u32 task_cls_classid(struct task_struct *p)
-{
-	return 0;
-}
-
-#endif
+extern struct static_key cgroup_cls_enabled;
+#define clscg_enabled static_key_false(&cgroup_cls_enabled)
 
-#if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
+extern void sock_update_classid(struct sock *sk, struct task_struct *task);
 
 static inline u32 task_cls_classid(struct task_struct *p)
 {
 	int classid;
 
+	if (!clscg_enabled)
+		return 0;
+
 	rcu_read_lock();
 	classid = container_of(task_subsys_state(p, net_cls_subsys_id),
 			       struct cgroup_cls_state, css)->classid;
@@ -54,24 +46,15 @@ static inline u32 task_cls_classid(struct task_struct *p)
 	return classid;
 }
 
-#elif IS_MODULE(CONFIG_NET_CLS_CGROUP)
+#else
 
-extern int net_cls_subsys_id;
+static inline void sock_update_classid(struct sock *sk, struct task_struct *task)
+{
+}
 
 static inline u32 task_cls_classid(struct task_struct *p)
 {
-	int id;
-	u32 classid = 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;
-	rcu_read_unlock();
-
-	return classid;
+	return 0;
 }
 
 #endif
diff --git a/include/net/netprio_cgroup.h b/include/net/netprio_cgroup.h
index c43461e..4ebdae6 100644
--- a/include/net/netprio_cgroup.h
+++ b/include/net/netprio_cgroup.h
@@ -16,6 +16,7 @@
 #include <linux/cgroup.h>
 #include <linux/hardirq.h>
 #include <linux/rcupdate.h>
+#include <linux/jump_label.h>
 
 #if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
 
@@ -30,28 +31,19 @@ struct cgroup_netprio_state {
 	u32 prioidx;
 };
 
-extern void sock_update_netprioidx(struct sock *sk, struct task_struct *task);
-
-#else
-
-static inline void sock_update_netprioidx(struct sock *sk, struct task_struct *task)
-{
-}
-
-static inline u32 task_netprioidx(struct task_struct *p)
-{
-	return 0;
-}
-
-#endif
+extern struct static_key cgroup_netprio_enabled;
+#define netpriocg_enabled static_key_false(&cgroup_netprio_enabled)
 
-#if IS_BUILTIN(CONFIG_NETPRIO_CGROUP)
+extern void sock_update_netprioidx(struct sock *sk, struct task_struct *task);
 
 static inline u32 task_netprioidx(struct task_struct *p)
 {
 	struct cgroup_netprio_state *state;
 	u32 idx;
 
+	if (!netpriocg_enabled)
+		return 0;
+
 	rcu_read_lock();
 	state = container_of(task_subsys_state(p, net_prio_subsys_id),
 			     struct cgroup_netprio_state, css);
@@ -60,26 +52,15 @@ static inline u32 task_netprioidx(struct task_struct *p)
 	return idx;
 }
 
-#elif IS_MODULE(CONFIG_NETPRIO_CGROUP)
+#else
 
-extern int net_prio_subsys_id;
+static inline void sock_update_netprioidx(struct sock *sk, struct task_struct *task)
+{
+}
 
 static inline u32 task_netprioidx(struct task_struct *p)
 {
-	struct cgroup_netprio_state *state;
-	int subsys_id;
-	u32 idx = 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;
-	}
-	rcu_read_unlock();
-	return idx;
+	return 0;
 }
 
 #endif /* CONFIG_NETPRIO_CGROUP */
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 7981850..aa629ce 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -93,9 +93,12 @@ 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
 
@@ -1307,7 +1310,7 @@ 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 > CGROUP_BUILTIN_SUBSYS_COUNT; i--) {
 			/* drop refcounts only on the ones we took */
 			unsigned long bit = 1UL << i;
 
@@ -1324,7 +1327,7 @@ 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 = CGROUP_BUILTIN_SUBSYS_COUNT + 1; i < CGROUP_SUBSYS_COUNT; i++) {
 		unsigned long bit = 1UL << i;
 
 		if (!(bit & subsys_bits))
@@ -4322,7 +4325,7 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
 	 */
 	if (ss->module == NULL) {
 		/* a few sanity checks */
-		BUG_ON(ss->subsys_id >= CGROUP_BUILTIN_SUBSYS_COUNT);
+		BUG_ON(ss->subsys_id > CGROUP_BUILTIN_SUBSYS_COUNT);
 		BUG_ON(subsys[ss->subsys_id] != ss);
 		return 0;
 	}
@@ -4330,24 +4333,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 = CGROUP_BUILTIN_SUBSYS_COUNT; 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
@@ -4356,7 +4343,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);
 	}
@@ -4372,7 +4359,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 238ece6..c3f93a1 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -155,6 +155,9 @@ static struct cgroup_subsys_state *cgrp_create(struct cgroup *cgrp)
 		goto out;
 	}
 
+	if (!netpriocg_enabled && !cgrp->parent)
+                static_key_slow_inc(&cgroup_netprio_enabled);
+
 	ret = update_netdev_tables();
 	if (ret < 0) {
 		put_prioidx(cs->prioidx);
@@ -173,6 +176,9 @@ static void cgrp_destroy(struct cgroup *cgrp)
 	struct net_device *dev;
 	struct netprio_map *map;
 
+        if (netpriocg_enabled && !cgrp->parent)
+                static_key_slow_dec(&cgroup_netprio_enabled);
+
 	cs = cgrp_netprio_state(cgrp);
 	rtnl_lock();
 	for_each_netdev(&init_net, dev) {
@@ -342,9 +348,7 @@ struct cgroup_subsys net_prio_subsys = {
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
 	.attach		= net_prio_attach,
-#if IS_BUILTIN(CONFIG_NETPRIO_CGROUP)
 	.subsys_id	= net_prio_subsys_id,
-#endif
 	.base_cftypes	= ss_files,
 	.module		= THIS_MODULE
 };
@@ -382,10 +386,6 @@ static int __init init_cgroup_netprio(void)
 	ret = cgroup_load_subsys(&net_prio_subsys);
 	if (ret)
 		goto out;
-#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
-	smp_wmb();
-	net_prio_subsys_id = net_prio_subsys.subsys_id;
-#endif
 
 	register_netdevice_notifier(&netprio_device_notifier);
 
@@ -402,11 +402,6 @@ static void __exit exit_cgroup_netprio(void)
 
 	cgroup_unload_subsys(&net_prio_subsys);
 
-#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
-	net_prio_subsys_id = -1;
-	synchronize_rcu();
-#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 bdd06af..301243b 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -326,15 +326,13 @@ 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)
-int net_cls_subsys_id = -1;
-EXPORT_SYMBOL_GPL(net_cls_subsys_id);
-#endif
-#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
-int net_prio_subsys_id = -1;
-EXPORT_SYMBOL_GPL(net_prio_subsys_id);
+#if IS_ENABLED(CONFIG_NET_CLS_CGROUP)
+struct static_key cgroup_cls_enabled = STATIC_KEY_INIT_FALSE;
+EXPORT_SYMBOL_GPL(cgroup_cls_enabled);
 #endif
+#if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
+struct static_key cgroup_netprio_enabled = STATIC_KEY_INIT_FALSE;
+EXPORT_SYMBOL_GPL(cgroup_netprio_enabled);
 #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 9ffe9b5..80f71cd 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -45,12 +45,17 @@ static struct cgroup_subsys_state *cgrp_create(struct cgroup *cgrp)
 
 	if (cgrp->parent)
 		cs->classid = cgrp_cls_state(cgrp->parent)->classid;
+	else if (!clscg_enabled)
+                static_key_slow_inc(&cgroup_cls_enabled);
 
 	return &cs->css;
 }
 
 static void cgrp_destroy(struct cgroup *cgrp)
 {
+	if (!cgrp->parent && clscg_enabled)
+                static_key_slow_dec(&cgroup_cls_enabled);
+
 	kfree(cgrp_cls_state(cgrp));
 }
 
@@ -115,9 +120,7 @@ struct cgroup_subsys net_cls_subsys = {
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
 	.attach		= cgrp_attach,
-#if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
 	.subsys_id	= net_cls_subsys_id,
-#endif
 	.base_cftypes	= ss_files,
 	.module		= THIS_MODULE,
 };
@@ -321,12 +324,6 @@ static int __init init_cgroup_cls(void)
 	if (ret)
 		goto out;
 
-#if IS_MODULE(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);
@@ -339,11 +336,6 @@ static void __exit exit_cgroup_cls(void)
 {
 	unregister_tcf_proto_ops(&cls_cgroup_ops);
 
-#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
-	net_cls_subsys_id = -1;
-	synchronize_rcu();
-#endif
-
 	cgroup_unload_subsys(&net_cls_subsys);
 }
 
-- 
1.7.12.rc1.16.g05a20c8

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

[Tejun Heo]

On Tue, Aug 14, 2012 at 03:02:23PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> 
> 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 12 + 1 at max (at this point there are 12
> subsystem). The additinal one is the price we have to pay to
> distinguish between builtin and module subsystems.
> 
> We should only access task_cls_classid() and task_netprioidx()
> if the subsystem is ready to be used using jump labels for this.

I think I want to like this patch but it's kinda confusing to review.
Is there any reasonable way that you can split the core changes from
net_cls ones?

Thanks.

-- 
tejun

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

[Daniel Wagner]

Hi Tejun,

On 08/14/2012 07:27 PM, Tejun Heo wrote:
> On Tue, Aug 14, 2012 at 03:02:23PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>>
>> 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 12 + 1 at max (at this point there are 12
>> subsystem). The additinal one is the price we have to pay to
>> distinguish between builtin and module subsystems.
>>
>> We should only access task_cls_classid() and task_netprioidx()
>> if the subsystem is ready to be used using jump labels for this.
>
> I think I want to like this patch but it's kinda confusing to review.
> Is there any reasonable way that you can split the core changes from
> net_cls ones?

My bad, sorry about that. Sure, I'll split the patches into smaller 
pieces and remove net_cls updating part as Neil has requested. You will 
get someting to review on Thursday, since tomorrow I am out of office.

thanks,
daniel

[PATCH v3 2/6] cgroup: net_cls rework update socket logic

[Daniel Wagner]

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

Update net_cls do do the same as net_prio does:

commit 406a3c638ce8b17d9704052c07955490f732c2b8
Author: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date:   Fri Jul 20 10:39:25 2012 +0000

    net: netprio_cgroup: rework update socket logic

    Instead of updating the sk_cgrp_prioidx struct field on every send
    this only updates the field when a task is moved via cgroup
    infrastructure.

    This allows sockets that may be used by a kernel worker thread
    to be managed. For example in the iscsi case today a user can
    put iscsid in a netprio cgroup and control traffic will be sent
    with the correct sk_cgrp_prioidx value set but as soon as data
    is sent the kernel worker thread isssues a send and sk_cgrp_prioidx
    is updated with the kernel worker threads value which is the
    default case.

    It seems more correct to only update the field when the user
    explicitly sets it via control group infrastructure. This allows
    the users to manage sockets that may be used with other threads.

John already send an updated version for the attach function which
does not rely on the ugly scanf code, which I am using for this patch
here.

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
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 <lizf-BthXqXjhjHXQFUHtdCDX3A@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  | 10 ++--------
 net/core/netprio_cgroup.c |  2 +-
 net/core/sock.c           | 11 ++++++-----
 net/sched/cls_cgroup.c    | 38 ++++++++++++++++++++++++++++++++++++++
 net/socket.c              |  8 --------
 5 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index 5f49b69..fc05f43 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -24,16 +24,13 @@ struct cgroup_cls_state
 	u32 classid;
 };
 
-extern void sock_update_classid(struct sock *sk);
+extern void sock_update_classid(struct sock *sk, struct task_struct *task);
 
 #ifdef 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;
@@ -49,9 +46,6 @@ static inline u32 task_cls_classid(struct task_struct *p)
 	int id;
 	u32 classid = 0;
 
-	if (in_interrupt())
-		return 0;
-
 	rcu_read_lock();
 	id = rcu_dereference_index_check(net_cls_subsys_id,
 					 rcu_read_lock_held());
@@ -64,7 +58,7 @@ static inline u32 task_cls_classid(struct task_struct *p)
 }
 #endif
 #else
-static inline void sock_update_classid(struct sock *sk)
+static inline void sock_update_classid(struct sock *sk, struct task_struct *task)
 {
 }
 
diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index ed0c043..98478aa 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -274,7 +274,7 @@ out_free_devname:
 	return ret;
 }
 
-void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
+static void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
 {
 	struct task_struct *p;
 	char *tmp = kzalloc(sizeof(char) * PATH_MAX, GFP_KERNEL);
diff --git a/net/core/sock.c b/net/core/sock.c
index 8f67ced..e08df6b 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1223,13 +1223,14 @@ static void sk_prot_free(struct proto *prot, struct sock *sk)
 }
 
 #ifdef CONFIG_CGROUPS
-void sock_update_classid(struct sock *sk)
+void sock_update_classid(struct sock *sk, struct task_struct *task)
 {
 	u32 classid;
 
-	rcu_read_lock();  /* doing current task, which cannot vanish. */
-	classid = task_cls_classid(current);
-	rcu_read_unlock();
+	if (in_interrupt())
+		return;
+
+	classid = task_cls_classid(task);
 	if (classid && classid != sk->sk_classid)
 		sk->sk_classid = classid;
 }
@@ -1269,7 +1270,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
 		sock_net_set(sk, get_net(net));
 		atomic_set(&sk->sk_wmem_alloc, 1);
 
-		sock_update_classid(sk);
+		sock_update_classid(sk, current);
 		sock_update_netprioidx(sk, current);
 	}
 
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 7743ea8..3535771 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -17,6 +17,7 @@
 #include <linux/skbuff.h>
 #include <linux/cgroup.h>
 #include <linux/rcupdate.h>
+#include <linux/fdtable.h>
 #include <net/rtnetlink.h>
 #include <net/pkt_cls.h>
 #include <net/sock.h>
@@ -53,6 +54,42 @@ static void cgrp_destroy(struct cgroup *cgrp)
 	kfree(cgrp_cls_state(cgrp));
 }
 
+static void cgrp_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
+{
+	struct task_struct *p;
+
+	cgroup_taskset_for_each(p, cgrp, tset) {
+		unsigned int fd;
+		struct fdtable *fdt;
+		struct files_struct *files;
+
+		task_lock(p);
+		files = p->files;
+		if (!files) {
+			task_unlock(p);
+			continue;
+		}
+
+		spin_lock(&files->file_lock);
+		fdt = files_fdtable(files);
+		for (fd = 0; fd < fdt->max_fds; fd++) {
+			struct file *file;
+			struct socket *sock;
+			int err;
+
+			file = fcheck_files(files, fd);
+			if (!file)
+				continue;
+
+			sock = sock_from_file(file, &err);
+			if (sock)
+				sock_update_netprioidx(sock->sk, p);
+		}
+		spin_unlock(&files->file_lock);
+		task_unlock(p);
+	}
+}
+
 static u64 read_classid(struct cgroup *cgrp, struct cftype *cft)
 {
 	return cgrp_cls_state(cgrp)->classid;
@@ -77,6 +114,7 @@ struct cgroup_subsys net_cls_subsys = {
 	.name		= "net_cls",
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
+	.attach		= cgrp_attach,
 #ifdef CONFIG_NET_CLS_CGROUP
 	.subsys_id	= net_cls_subsys_id,
 #endif
diff --git a/net/socket.c b/net/socket.c
index dfe5b66..f06c8c4 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -553,8 +553,6 @@ static inline int __sock_sendmsg_nosec(struct kiocb *iocb, struct socket *sock,
 {
 	struct sock_iocb *si = kiocb_to_siocb(iocb);
 
-	sock_update_classid(sock->sk);
-
 	si->sock = sock;
 	si->scm = NULL;
 	si->msg = msg;
@@ -717,8 +715,6 @@ static inline int __sock_recvmsg_nosec(struct kiocb *iocb, struct socket *sock,
 {
 	struct sock_iocb *si = kiocb_to_siocb(iocb);
 
-	sock_update_classid(sock->sk);
-
 	si->sock = sock;
 	si->scm = NULL;
 	si->msg = msg;
@@ -829,8 +825,6 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
 	if (unlikely(!sock->ops->splice_read))
 		return -EINVAL;
 
-	sock_update_classid(sock->sk);
-
 	return sock->ops->splice_read(sock, ppos, pipe, len, flags);
 }
 
@@ -3353,8 +3347,6 @@ EXPORT_SYMBOL(kernel_setsockopt);
 int kernel_sendpage(struct socket *sock, struct page *page, int offset,
 		    size_t size, int flags)
 {
-	sock_update_classid(sock->sk);
-
 	if (sock->ops->sendpage)
 		return sock->ops->sendpage(sock, page, offset, size, flags);
 
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v3 2/5] cgroup: rework update socket logic

[Daniel Wagner]

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

Update net_cls do do the same as net_prio does:

commit 406a3c638ce8b17d9704052c07955490f732c2b8
Author: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date:   Fri Jul 20 10:39:25 2012 +0000

    net: netprio_cgroup: rework update socket logic

    Instead of updating the sk_cgrp_prioidx struct field on every send
    this only updates the field when a task is moved via cgroup
    infrastructure.

    This allows sockets that may be used by a kernel worker thread
    to be managed. For example in the iscsi case today a user can
    put iscsid in a netprio cgroup and control traffic will be sent
    with the correct sk_cgrp_prioidx value set but as soon as data
    is sent the kernel worker thread isssues a send and sk_cgrp_prioidx
    is updated with the kernel worker threads value which is the
    default case.

    It seems more correct to only update the field when the user
    explicitly sets it via control group infrastructure. This allows
    the users to manage sockets that may be used with other threads.

John already send an updated version for the attach function which
does not rely on the ugly scanf code, which I am using for this patch
here.

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Kamezawa Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Jamal Hadi Salim <jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
Cc: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 include/net/cls_cgroup.h  | 10 ++--------
 net/core/netprio_cgroup.c |  2 +-
 net/core/sock.c           | 11 ++++++-----
 net/sched/cls_cgroup.c    | 38 ++++++++++++++++++++++++++++++++++++++
 net/socket.c              |  8 --------
 5 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index 5f49b69..fc05f43 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -24,16 +24,13 @@ struct cgroup_cls_state
 	u32 classid;
 };
 
-extern void sock_update_classid(struct sock *sk);
+extern void sock_update_classid(struct sock *sk, struct task_struct *task);
 
 #ifdef 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;
@@ -49,9 +46,6 @@ static inline u32 task_cls_classid(struct task_struct *p)
 	int id;
 	u32 classid = 0;
 
-	if (in_interrupt())
-		return 0;
-
 	rcu_read_lock();
 	id = rcu_dereference_index_check(net_cls_subsys_id,
 					 rcu_read_lock_held());
@@ -64,7 +58,7 @@ static inline u32 task_cls_classid(struct task_struct *p)
 }
 #endif
 #else
-static inline void sock_update_classid(struct sock *sk)
+static inline void sock_update_classid(struct sock *sk, struct task_struct *task)
 {
 }
 
diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index ed0c043..98478aa 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -274,7 +274,7 @@ out_free_devname:
 	return ret;
 }
 
-void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
+static void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
 {
 	struct task_struct *p;
 	char *tmp = kzalloc(sizeof(char) * PATH_MAX, GFP_KERNEL);
diff --git a/net/core/sock.c b/net/core/sock.c
index 8f67ced..e08df6b 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1223,13 +1223,14 @@ static void sk_prot_free(struct proto *prot, struct sock *sk)
 }
 
 #ifdef CONFIG_CGROUPS
-void sock_update_classid(struct sock *sk)
+void sock_update_classid(struct sock *sk, struct task_struct *task)
 {
 	u32 classid;
 
-	rcu_read_lock();  /* doing current task, which cannot vanish. */
-	classid = task_cls_classid(current);
-	rcu_read_unlock();
+	if (in_interrupt())
+		return;
+
+	classid = task_cls_classid(task);
 	if (classid && classid != sk->sk_classid)
 		sk->sk_classid = classid;
 }
@@ -1269,7 +1270,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
 		sock_net_set(sk, get_net(net));
 		atomic_set(&sk->sk_wmem_alloc, 1);
 
-		sock_update_classid(sk);
+		sock_update_classid(sk, current);
 		sock_update_netprioidx(sk, current);
 	}
 
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 7743ea8..3535771 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -17,6 +17,7 @@
 #include <linux/skbuff.h>
 #include <linux/cgroup.h>
 #include <linux/rcupdate.h>
+#include <linux/fdtable.h>
 #include <net/rtnetlink.h>
 #include <net/pkt_cls.h>
 #include <net/sock.h>
@@ -53,6 +54,42 @@ static void cgrp_destroy(struct cgroup *cgrp)
 	kfree(cgrp_cls_state(cgrp));
 }
 
+static void cgrp_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
+{
+	struct task_struct *p;
+
+	cgroup_taskset_for_each(p, cgrp, tset) {
+		unsigned int fd;
+		struct fdtable *fdt;
+		struct files_struct *files;
+
+		task_lock(p);
+		files = p->files;
+		if (!files) {
+			task_unlock(p);
+			continue;
+		}
+
+		spin_lock(&files->file_lock);
+		fdt = files_fdtable(files);
+		for (fd = 0; fd < fdt->max_fds; fd++) {
+			struct file *file;
+			struct socket *sock;
+			int err;
+
+			file = fcheck_files(files, fd);
+			if (!file)
+				continue;
+
+			sock = sock_from_file(file, &err);
+			if (sock)
+				sock_update_netprioidx(sock->sk, p);
+		}
+		spin_unlock(&files->file_lock);
+		task_unlock(p);
+	}
+}
+
 static u64 read_classid(struct cgroup *cgrp, struct cftype *cft)
 {
 	return cgrp_cls_state(cgrp)->classid;
@@ -77,6 +114,7 @@ struct cgroup_subsys net_cls_subsys = {
 	.name		= "net_cls",
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
+	.attach		= cgrp_attach,
 #ifdef CONFIG_NET_CLS_CGROUP
 	.subsys_id	= net_cls_subsys_id,
 #endif
diff --git a/net/socket.c b/net/socket.c
index dfe5b66..f06c8c4 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -553,8 +553,6 @@ static inline int __sock_sendmsg_nosec(struct kiocb *iocb, struct socket *sock,
 {
 	struct sock_iocb *si = kiocb_to_siocb(iocb);
 
-	sock_update_classid(sock->sk);
-
 	si->sock = sock;
 	si->scm = NULL;
 	si->msg = msg;
@@ -717,8 +715,6 @@ static inline int __sock_recvmsg_nosec(struct kiocb *iocb, struct socket *sock,
 {
 	struct sock_iocb *si = kiocb_to_siocb(iocb);
 
-	sock_update_classid(sock->sk);
-
 	si->sock = sock;
 	si->scm = NULL;
 	si->msg = msg;
@@ -829,8 +825,6 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
 	if (unlikely(!sock->ops->splice_read))
 		return -EINVAL;
 
-	sock_update_classid(sock->sk);
-
 	return sock->ops->splice_read(sock, ppos, pipe, len, flags);
 }
 
@@ -3353,8 +3347,6 @@ EXPORT_SYMBOL(kernel_setsockopt);
 int kernel_sendpage(struct socket *sock, struct page *page, int offset,
 		    size_t size, int flags)
 {
-	sock_update_classid(sock->sk);
-
 	if (sock->ops->sendpage)
 		return sock->ops->sendpage(sock, page, offset, size, flags);
 
-- 
1.7.12.rc1.16.g05a20c8

[PATCH v3 3/6] cgroup: Update classid for fd pass in SCM_RIGHTS datagramm

[Daniel Wagner]

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

A socket fd passed in a SCM_RIGHTS datagram was not getting
updated with the new tasks cgrp classid. This leaves IO on
the socket tagged with the old tasks classid.

To fix this add a check in the scm recvmsg path to update the
sock cgrp classid with the new tasks value.

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
Cc: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Tim Chen <tim.c.chen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 net/core/scm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/core/scm.c b/net/core/scm.c
index 8f6ccfd..221080f 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -35,7 +35,7 @@
 #include <net/sock.h>
 #include <net/compat.h>
 #include <net/scm.h>
-
+#include <net/cls_cgroup.h>
 
 /*
  *	Only allow a user to send credentials, that they could set with
@@ -249,6 +249,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 	struct file **fp = scm->fp->fp;
 	int __user *cmfptr;
 	int err = 0, i;
+	__u32 classid = task_cls_classid(current);
 
 	if (MSG_CMSG_COMPAT & msg->msg_flags) {
 		scm_detach_fds_compat(msg, scm);
@@ -265,6 +266,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 	for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i<fdmax;
 	     i++, cmfptr++)
 	{
+		struct socket *sock;
 		int new_fd;
 		err = security_file_receive(fp[i]);
 		if (err)
@@ -282,6 +284,9 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 		/* Bump the usage count and install the file. */
 		get_file(fp[i]);
 		fd_install(new_fd, fp[i]);
+		sock = sock_from_file(fp[i], &err);
+		if (sock)
+			sock->sk->sk_classid = classid;
 	}
 
 	if (i > 0)
-- 
1.7.12.rc1.16.g05a20c8

Re: [PATCH v3 0/6] cgroup cls & netprio 'cleanups'

[Neil Horman]

On Tue, Aug 14, 2012 at 03:02:16PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> 
> Hi,
> 
> Sorry for the delay on updating this series. The usual
> excuse apply here.
> 
> I saw that John is busy improving net_prio so I took the
> liberty to port his changes to net_cls (#1-3). Patch #3 will
> collide with John's unapplied patches. I am happy
> to rebase this series if needed. 
> 
> Patch #4 and #5 improve the readability with using
> IS_MODULE/BUILTIN macros. This patches prepare the last
> patch.
> 
> Patch #6 removes support for assigning subsystem IDs during
> runtime. As it turns out this is not really needed. By doing
> so we are able to free some unused memory.
> 
> The patches are against net-next.
> 
> cheers,
> daniel
> 
These aren't so much 'cleanups' as feature enhancements and fixes for the first
pass of those enhancements (at least in the case of the net_prio cgroup).  I've
nothing against them, but since we're still going through some churn on the
net_prio variant, it may be best to wait until thats settled before moving them
over to net_cls.
Neil

Re: [PATCH v3 0/6] cgroup cls & netprio 'cleanups'

[Daniel Wagner]

Hi Neil,

On 14.08.2012 15:10, Neil Horman wrote:
> On Tue, Aug 14, 2012 at 03:02:16PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>>
>> Hi,
>>
>> Sorry for the delay on updating this series. The usual
>> excuse apply here.
>>
>> I saw that John is busy improving net_prio so I took the
>> liberty to port his changes to net_cls (#1-3). Patch #3 will
>> collide with John's unapplied patches. I am happy
>> to rebase this series if needed.
>>
>> Patch #4 and #5 improve the readability with using
>> IS_MODULE/BUILTIN macros. This patches prepare the last
>> patch.
>>
>> Patch #6 removes support for assigning subsystem IDs during
>> runtime. As it turns out this is not really needed. By doing
>> so we are able to free some unused memory.
>>
>> The patches are against net-next.
>>
>> cheers,
>> daniel
>>
> These aren't so much 'cleanups' as feature enhancements and fixes for the first
> pass of those enhancements (at least in the case of the net_prio cgroup).

Sorry about that. I wanted to keep the series title, so that someone 
looking up older versions find it.

> I've nothing against them, but since we're still going through some churn on the
> net_prio variant, it may be best to wait until thats settled before moving them
> over to net_cls.

Sure, I can update this series when the net_prio controller changes have 
settled down.

I just wonder if it wouldn't make sense to merge them together. 
Obviously, that will break the user space which is not a good thing but 
having a controller per socket option is not good either.

cheers,
daniel

Re: [PATCH v3 0/6] cgroup cls & netprio 'cleanups'

[Neil Horman]

On Tue, Aug 14, 2012 at 03:25:50PM +0200, Daniel Wagner wrote:
> Hi Neil,
> 
> On 14.08.2012 15:10, Neil Horman wrote:
> >On Tue, Aug 14, 2012 at 03:02:16PM +0200, Daniel Wagner wrote:
> >>From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> >>
> >>Hi,
> >>
> >>Sorry for the delay on updating this series. The usual
> >>excuse apply here.
> >>
> >>I saw that John is busy improving net_prio so I took the
> >>liberty to port his changes to net_cls (#1-3). Patch #3 will
> >>collide with John's unapplied patches. I am happy
> >>to rebase this series if needed.
> >>
> >>Patch #4 and #5 improve the readability with using
> >>IS_MODULE/BUILTIN macros. This patches prepare the last
> >>patch.
> >>
> >>Patch #6 removes support for assigning subsystem IDs during
> >>runtime. As it turns out this is not really needed. By doing
> >>so we are able to free some unused memory.
> >>
> >>The patches are against net-next.
> >>
> >>cheers,
> >>daniel
> >>
> >These aren't so much 'cleanups' as feature enhancements and fixes for the first
> >pass of those enhancements (at least in the case of the net_prio cgroup).
> 
> Sorry about that. I wanted to keep the series title, so that someone
> looking up older versions find it.
> 
That makes sense, but it would be best until we acked the the version going into
net_prio, otherwise we maybe tracking regressions in two places rather than one.

> >I've nothing against them, but since we're still going through some churn on the
> >net_prio variant, it may be best to wait until thats settled before moving them
> >over to net_cls.
> 
> Sure, I can update this series when the net_prio controller changes
> have settled down.
> 
Thank you, I think thats a good idea.

> I just wonder if it wouldn't make sense to merge them together.
> Obviously, that will break the user space which is not a good thing
> but having a controller per socket option is not good either.
> 
This has been discussed (although perhaps not on list) before.  I don't think
we're going to see lots of cgroups for socket options.  most of them have proc
tunables, priroity and classification dont.  We could merge the two controllers,
but as you said it breaks users space which is a non-starter.  It also doesn't
really buy us anything, as people want to be able to set priority and
classification independently, so we either use two controllers, or one
controller with twice as many cgroup instances (one for each combination of
priroity/class that an admin wants).  

I'll ping you when the net_prio stuff settles out.
Regards
Neil
 
> cheers,
> daniel
> 
> --
> 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 v3 0/6] cgroup cls & netprio 'cleanups'

[Daniel Wagner]

Hi Neil,

On 14.08.2012 15:30, Neil Horman wrote:
> On Tue, Aug 14, 2012 at 03:25:50PM +0200, Daniel Wagner wrote:
>> On 14.08.2012 15:10, Neil Horman wrote:
>>> On Tue, Aug 14, 2012 at 03:02:16PM +0200, Daniel Wagner wrote:
>>>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>>>>
>>>> Hi,
>>>>
>>>> Sorry for the delay on updating this series. The usual
>>>> excuse apply here.
>>>>
>>>> I saw that John is busy improving net_prio so I took the
>>>> liberty to port his changes to net_cls (#1-3). Patch #3 will
>>>> collide with John's unapplied patches. I am happy
>>>> to rebase this series if needed.
>>>>
>>>> Patch #4 and #5 improve the readability with using
>>>> IS_MODULE/BUILTIN macros. This patches prepare the last
>>>> patch.
>>>>
>>>> Patch #6 removes support for assigning subsystem IDs during
>>>> runtime. As it turns out this is not really needed. By doing
>>>> so we are able to free some unused memory.
>>>>
>>>> The patches are against net-next.
>>>>
>>>> cheers,
>>>> daniel
>>>>
>>> These aren't so much 'cleanups' as feature enhancements and fixes for the first
>>> pass of those enhancements (at least in the case of the net_prio cgroup).
>>
>> Sorry about that. I wanted to keep the series title, so that someone
>> looking up older versions find it.
>>
> That makes sense, but it would be best until we acked the the version going into
> net_prio, otherwise we maybe tracking regressions in two places rather than one.

I agree, let's wait until net_prio gets stable.
 
>>> I've nothing against them, but since we're still going through some churn on the
>>> net_prio variant, it may be best to wait until thats settled before moving them
>>> over to net_cls.
>>
>> Sure, I can update this series when the net_prio controller changes
>> have settled down.
>>
> Thank you, I think thats a good idea.
> 
>> I just wonder if it wouldn't make sense to merge them together.
>> Obviously, that will break the user space which is not a good thing
>> but having a controller per socket option is not good either.
>>
> This has been discussed (although perhaps not on list) before.  I don't think
> we're going to see lots of cgroups for socket options.  most of them have proc
> tunables, priroity and classification dont.

Well, I'll would like to able to set SO_MARK via a controller [1][2]. The use
case is that I'd like to set the routing table per application. As it turns
out the kernel has almost all bits and pieces to get this working. The only
missing thing is setting SO_MARK without touching the application. For which
the net cgroup controllers seem to be the perfect match.

> We could merge the two controllers,
> but as you said it breaks users space which is a non-starter.  It also doesn't
> really buy us anything, as people want to be able to set priority and
> classification independently, so we either use two controllers, or one
> controller with twice as many cgroup instances (one for each combination of
> priroity/class that an admin wants).

Okay, I see your point. What is your standpoint concerning SO_MARK? Following
your logic, we would need another controller for this which is what I rather
have avoided. But I am more than happy to send patches for this :)

> I'll ping you when the net_prio stuff settles out.

Great, thanks.

cheers,
daniel

[1] http://lists.connman.net/pipermail/connman/2012-August/010716.html
[2] http://lists.connman.net/pipermail/connman/2012-August/010715.html

Re: [PATCH v3 0/6] cgroup cls & netprio 'cleanups'

[Neil Horman]

On Tue, Aug 14, 2012 at 03:46:49PM +0200, Daniel Wagner wrote:
> Hi Neil,
> 
> On 14.08.2012 15:30, Neil Horman wrote:
> > On Tue, Aug 14, 2012 at 03:25:50PM +0200, Daniel Wagner wrote:
> >> On 14.08.2012 15:10, Neil Horman wrote:
> >>> On Tue, Aug 14, 2012 at 03:02:16PM +0200, Daniel Wagner wrote:
> >>>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> >>>>
> >>>> Hi,
> >>>>
> >>>> Sorry for the delay on updating this series. The usual
> >>>> excuse apply here.
> >>>>
> >>>> I saw that John is busy improving net_prio so I took the
> >>>> liberty to port his changes to net_cls (#1-3). Patch #3 will
> >>>> collide with John's unapplied patches. I am happy
> >>>> to rebase this series if needed.
> >>>>
> >>>> Patch #4 and #5 improve the readability with using
> >>>> IS_MODULE/BUILTIN macros. This patches prepare the last
> >>>> patch.
> >>>>
> >>>> Patch #6 removes support for assigning subsystem IDs during
> >>>> runtime. As it turns out this is not really needed. By doing
> >>>> so we are able to free some unused memory.
> >>>>
> >>>> The patches are against net-next.
> >>>>
> >>>> cheers,
> >>>> daniel
> >>>>
> >>> These aren't so much 'cleanups' as feature enhancements and fixes for the first
> >>> pass of those enhancements (at least in the case of the net_prio cgroup).
> >>
> >> Sorry about that. I wanted to keep the series title, so that someone
> >> looking up older versions find it.
> >>
> > That makes sense, but it would be best until we acked the the version going into
> > net_prio, otherwise we maybe tracking regressions in two places rather than one.
> 
> I agree, let's wait until net_prio gets stable.
>  
> >>> I've nothing against them, but since we're still going through some churn on the
> >>> net_prio variant, it may be best to wait until thats settled before moving them
> >>> over to net_cls.
> >>
> >> Sure, I can update this series when the net_prio controller changes
> >> have settled down.
> >>
> > Thank you, I think thats a good idea.
> > 
> >> I just wonder if it wouldn't make sense to merge them together.
> >> Obviously, that will break the user space which is not a good thing
> >> but having a controller per socket option is not good either.
> >>
> > This has been discussed (although perhaps not on list) before.  I don't think
> > we're going to see lots of cgroups for socket options.  most of them have proc
> > tunables, priroity and classification dont.
> 
> Well, I'll would like to able to set SO_MARK via a controller [1][2]. The use
> case is that I'd like to set the routing table per application. As it turns
> out the kernel has almost all bits and pieces to get this working. The only
> missing thing is setting SO_MARK without touching the application. For which
> the net cgroup controllers seem to be the perfect match.
> 
Ok, I stand corrected, there are other things that might be good for using with
cgroups in the networking space.

> > We could merge the two controllers,
> > but as you said it breaks users space which is a non-starter.  It also doesn't
> > really buy us anything, as people want to be able to set priority and
> > classification independently, so we either use two controllers, or one
> > controller with twice as many cgroup instances (one for each combination of
> > priroity/class that an admin wants).
> 
> Okay, I see your point. What is your standpoint concerning SO_MARK? Following
> your logic, we would need another controller for this which is what I rather
> have avoided. But I am more than happy to send patches for this :)
> 
Others may well have differing opinions, but my feeling is separate controllers
for separate functions.  As noted, while you could merge all the controllers, it
doesn't buy you much, and convolutes the meaning of a particular cgroup instance
(i.e. with a merged controller, a given cgroup instance owuld enforce
clasification A, priority B and mark value C, while another could enforce class
D priority B and mark C.  With separate controllers, admins can independently
select which class/priority and mark value to apply orthogonally.

So I'd vote for separate controllers.

> > I'll ping you when the net_prio stuff settles out.
> 
> Great, thanks.
> 
> cheers,
> daniel
> 
> [1] http://lists.connman.net/pipermail/connman/2012-August/010716.html
> [2] http://lists.connman.net/pipermail/connman/2012-August/010715.html
> 
> --
> 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
>