[PATCH v2 0/5] cgroup cls & netprio 'cleanups'

[PATCH v2 0/5] cgroup cls & netprio 'cleanups'

[Daniel Wagner]

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

Hi,

While looking a bit at the cls and netprio controller I found a
few things which I think are 'not optimal'.

The first patch makes sock_update_netprioidx() a bit smatter, so that
only on changed value the sk->sk_cgroup_prioidx is written and not
all the time.

The next two patches are changing the built. In case someone 
builds a kernel with cgroup support but disabled cls or netprio
controller, there was still code added.

And the last two patches change the cls and netprio source files in
that way, that the use IS_BUILTIN and IS_MODULE as it used in the
header files.

The patches are against net-next.

cheers,
daniel

Daniel Wagner (5):
  cgroup: Only update sk_cgrp_prioidx on change
  cgroup: Do not add sock_update_classid() when CONFIG_NET_CLS_CGROUP=0
  cgroup: Do not add sock_update_netprioidx() when
    CONFIG_NETPRIO_CGROUP=0
  cgroup: Make builtin vs. module build ifdefs consistent
  cgroup: Make builtin vs. module build ifdefs consistent

 include/net/cls_cgroup.h     | 33 +++++++++++++++++++++++----------
 include/net/netprio_cgroup.h | 31 ++++++++++++++-----------------
 include/net/sock.h           |  8 --------
 net/core/netprio_cgroup.c    |  6 +++---
 net/core/sock.c              | 14 +++++++++++---
 net/sched/cls_cgroup.c       |  6 +++---
 6 files changed, 54 insertions(+), 44 deletions(-)

-- 
1.7.11.1.165.g299666c

[PATCH v2 1/5] cgroup: Only update sk_cgrp_prioidx on change

[Daniel Wagner]

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

Do not make the cache line dirty when nothing has changed.
sock_update_classid() does exactly the same thing.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Cc: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Glauber Costa <glommer@parallels.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
 net/core/sock.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 929bdcc..bbab10d 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1182,10 +1182,14 @@ EXPORT_SYMBOL(sock_update_classid);
 
 void sock_update_netprioidx(struct sock *sk)
 {
+	u32 prioidx;
+
 	if (in_interrupt())
 		return;
 
-	sk->sk_cgrp_prioidx = task_netprioidx(current);
+	prioidx = task_netprioidx(current);
+	if (prioidx != sk->sk_cgrp_prioidx)
+		sk->sk_cgrp_prioidx = prioidx;
 }
 EXPORT_SYMBOL_GPL(sock_update_netprioidx);
 #endif
-- 
1.7.11.1.165.g299666c

[PATCH v2 3/5] cgroup: Do not add sock_update_netprioidx() when CONFIG_NETPRIO_CGROUP=0

[Daniel Wagner]

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

With the configuration CONFIG_CGROUPS=y and CONFIG_NETPRIO_CGROUP=0
sock_update_netprioidx() was still added and introduced unnecessary load.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Glauber Costa <glommer@parallels.com>
Cc: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
 include/net/netprio_cgroup.h | 31 ++++++++++++++-----------------
 net/core/sock.c              |  4 +++-
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/include/net/netprio_cgroup.h b/include/net/netprio_cgroup.h
index d58fdec..d14dfbe 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,24 @@ 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);
 
+#else
+
+#define sock_update_netprioidx(sk)
+
+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 +60,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 +80,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)
-#endif
-
 #endif  /* _NET_CLS_CGROUP_H */
diff --git a/net/core/sock.c b/net/core/sock.c
index 52c4a2a..94e0100 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -276,7 +276,7 @@ EXPORT_SYMBOL(sysctl_optmem_max);
 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
@@ -1182,6 +1182,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)
 {
 	u32 prioidx;
@@ -1195,6 +1196,7 @@ void sock_update_netprioidx(struct sock *sk)
 }
 EXPORT_SYMBOL_GPL(sock_update_netprioidx);
 #endif
+#endif
 
 /**
  *	sk_alloc - All socket objects are allocated here
-- 
1.7.11.1.165.g299666c

[PATCH v2 4/5] cgroup: Make builtin vs. module build ifdefs consistent

[Daniel Wagner]

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

The header file is using IS_MODULE and IS_BUILTIN macros. So let's
use them in the source file too.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
 net/sched/cls_cgroup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 7743ea8..4bfb8f7 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -77,7 +77,7 @@ struct cgroup_subsys net_cls_subsys = {
 	.name		= "net_cls",
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
-#ifdef CONFIG_NET_CLS_CGROUP
+#if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
 	.subsys_id	= net_cls_subsys_id,
 #endif
 	.base_cftypes	= ss_files,
@@ -283,7 +283,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;
@@ -301,7 +301,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.11.1.165.g299666c

[PATCH v2 2/5] cgroup: Do not add sock_update_classid() when CONFIG_NET_CLS_CGROUP=0

[Daniel Wagner]

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

With the configuration CONFIG_CGROUPS=y and CONFIG_NET_CLS_CGROUP=0
sock_update_classid() was still added and introduced unnecessary load.

Also remove cls cgroup related definition form sock.h. The only user
is socket.h which includes cls_cgroup.h direclty.

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: Kamezawa Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
---
 include/net/cls_cgroup.h | 33 +++++++++++++++++++++++----------
 include/net/sock.h       |  8 --------
 net/core/sock.c          |  4 +++-
 3 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index a4dc5b0..d2a8cfa 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -17,14 +17,29 @@
 #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;
 	u32 classid;
 };
 
-#ifdef CONFIG_NET_CLS_CGROUP
+extern void sock_update_classid(struct sock *sk);
+
+#else
+
+#define sock_update_classid(sk)
+
+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;
@@ -39,7 +54,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 +77,7 @@ 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_GROUP */
+
 #endif  /* _NET_CLS_CGROUP_H */
diff --git a/include/net/sock.h b/include/net/sock.h
index dcb54a0..6282230 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1451,14 +1451,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.
diff --git a/net/core/sock.c b/net/core/sock.c
index bbab10d..52c4a2a 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -272,7 +272,7 @@ int sysctl_optmem_max __read_mostly = sizeof(unsigned long)*(2*UIO_MAXIOV+512);
 EXPORT_SYMBOL(sysctl_optmem_max);
 
 #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
@@ -1168,6 +1168,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;
@@ -1179,6 +1180,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)
 {
-- 
1.7.11.1.165.g299666c

[PATCH v2 5/5] cgroup: Make builtin vs. module build ifdefs consistent

[Daniel Wagner]

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

The header file is using IS_MODULE and IS_BUILTIN macros. So let's
use them in the source file too.

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
---
 net/core/netprio_cgroup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index 3e953ea..2707948 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -252,7 +252,7 @@ struct cgroup_subsys net_prio_subsys = {
 	.name		= "net_prio",
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
-#ifdef CONFIG_NETPRIO_CGROUP
+#if IS_BUILTIN(CONFIG_NETPRIO_CGROUP)
 	.subsys_id	= net_prio_subsys_id,
 #endif
 	.base_cftypes	= ss_files,
@@ -292,7 +292,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
@@ -312,7 +312,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
-- 
1.7.11.1.165.g299666c

Re: [PATCH v2 0/5] cgroup cls & netprio 'cleanups'

[Daniel Wagner]

On 11.07.2012 11:35, Daniel Wagner wrote:
> The next two patches are changing the built. In case someone
> builds a kernel with cgroup support but disabled cls or netprio
> controller, there was still code added.
>
> And the last two patches change the cls and netprio source files in
> that way, that the use IS_BUILTIN and IS_MODULE as it used in the
> header files.

Forget patch #2 - #5.

I'll have a new version soon ready which changes the *_subsys_id part to 
always beeing a enum. This allows to remove the rather unpleasant module 
loading part with the ID assignment.

cheers,
daniel

Re: [PATCH v2 0/5] cgroup cls & netprio 'cleanups'

[David Miller]

From: Daniel Wagner <wagi@monom.org>
Date: Fri, 13 Jul 2012 15:53:37 +0200

> On 11.07.2012 11:35, Daniel Wagner wrote:
>> The next two patches are changing the built. In case someone
>> builds a kernel with cgroup support but disabled cls or netprio
>> controller, there was still code added.
>>
>> And the last two patches change the cls and netprio source files in
>> that way, that the use IS_BUILTIN and IS_MODULE as it used in the
>> header files.
> 
> Forget patch #2 - #5.
> 
> I'll have a new version soon ready which changes the *_subsys_id part
> to always beeing a enum. This allows to remove the rather unpleasant
> module loading part with the ID assignment.

Resubmit the entire set once the new versions of #2 - #5 are ready.