Re: [PATCH] mm/memcontrol.c: Remove duplicate inclusion of sock.h file

On Wed 12-09-12 12:50:41, Glauber Costa wrote:
[...]

quoted

quoted

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 795e525..85ec9ff 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c

@@ -50,8 +50,12 @@
 #include <linux/cpu.h>
 #include <linux/oom.h>
 #include "internal.h"
+
+#ifdef CONFIG_MEMCG_KMEM
 #include <net/sock.h>
+#include <net/ip.h>
 #include <net/tcp_memcontrol.h>
+#endif
 
 #include <asm/uaccess.h>
 

@@ -326,7 +330,7 @@ struct mem_cgroup {
 	struct mem_cgroup_stat_cpu nocpu_base;
 	spinlock_t pcp_counter_lock;
 
-#ifdef CONFIG_INET
+#ifdef CONFIG_MEMCG_KMEM
 	struct tcp_memcontrol tcp_mem;
 #endif
 };

If you are changing this, why not test for both? This field will be
useless with inet disabled. I usually don't like conditional in
structures (note that the "kmem" res counter in my patchsets is not
conditional to KMEM!!), but since the decision was made to make this one
conditional, I think INET is a much better test. I am fine with both though.

 
 You are right of course. Updated patch bellow:
---
From 9c465974c27859b6230065436b16e5c86a717291 Mon Sep 17 00:00:00 2001
From: Michal Hocko <redacted>
Date: Tue, 11 Sep 2012 10:38:42 +0200
Subject: [PATCH] memcg: clean up networking headers file inclusion

Memory controller doesn't need anything from the networking stack unless
CONFIG_MEMCG_KMEM is selected.
Now we are including net/sock.h and net/tcp_memcontrol.h unconditionally
which is not necessary. Moreover struct mem_cgroup contains tcp_mem even
if CONFIG_MEMCG_KMEM is not selected which is not necessary.

Signed-off-by: Sachin Kamat <redacted>
Signed-off-by: Michal Hocko <redacted>
---
 mm/memcontrol.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 795e525..369b552 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c

@@ -50,8 +50,12 @@
 #include <linux/cpu.h>
 #include <linux/oom.h>
 #include "internal.h"
+
+#ifdef CONFIG_MEMCG_KMEM && CONFIG_INET
 #include <net/sock.h>
+#include <net/ip.h>
 #include <net/tcp_memcontrol.h>
+#endif
 
 #include <asm/uaccess.h>
 

@@ -326,7 +330,7 @@ struct mem_cgroup {
 	struct mem_cgroup_stat_cpu nocpu_base;
 	spinlock_t pcp_counter_lock;
 
-#ifdef CONFIG_INET
+#ifdef CONFIG_MEMCG_KMEM && CONFIG_INET
 	struct tcp_memcontrol tcp_mem;
 #endif
 };

@@ -413,8 +417,6 @@ struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *s)
 
 /* Writing them here to avoid exposing memcg's inner layout */
 #ifdef CONFIG_MEMCG_KMEM
-#include <net/sock.h>
-#include <net/ip.h>
 
 static bool mem_cgroup_is_root(struct mem_cgroup *memcg);
 void sock_update_memcg(struct sock *sk)

-- 
1.7.10.4


-- 
Michal Hocko
SUSE Labs