Skip to contentSkip to conversation

mm: memcontrol: reign in CONFIG space madness

RFC PATCH In review Johannes Weiner 3 participants 6 messages Dec 9, 2015 – Dec 10, 2015

Subsystems (3)
Memory management cgroup memcg
lore
Share & tools
lore

Johannes Weiner

author maintainer
#0 lore raw

RFC PATCH 1/1 mm: memcontrol: reign in CONFIG space madness

Hey guys,

there has been quite a bit of trouble that stems from dividing our CONFIG space and having to provide real code and dummy functions correctly in all possible combinations. This is amplified by having the legacy mode and the cgroup2 mode in the same file sharing code.

The socket memory and kmem accounting series is a nightmare in that respect, and I'm still in the process of sorting it out. But no matter what the outcome there is going to be, what do you think about getting rid of the CONFIG_MEMCG[_LEGACY]_KMEM and CONFIG_INET stuff?

Because they end up saving very little and it doesn't seem worth the trouble. CONFIG_MEMCG_LEGACY_KMEM basically allows not compiling the interface structures and the limit updating function. Everything else is included anyway because of cgroup2. And CONFIG_INET also only saves a page_counter and two words in struct mem_cgroup, as well as the tiny socket-specific charge and uncharge wrappers that nobody would call.

Would you be opposed to getting rid of them to simplify things?

include/linux/memcontrol.h +30 -34
index c6a5ed2..bf895c0 100644
@@ -233,9 +233,10 @@ struct mem_cgroup {
 	 */
 	struct mem_cgroup_stat_cpu __percpu *stat;
 
-#if defined(CONFIG_MEMCG_LEGACY_KMEM) && defined(CONFIG_INET)
+	unsigned long		socket_pressure;
+
+	/* Legacy tcp memory control */
 	struct cg_proto tcp_mem;
-#endif
 
         /* Index in the kmem_cache->memcg_params.memcg_caches array */
 	int kmemcg_id;

@@ -253,10 +254,6 @@ struct mem_cgroup {
 	struct wb_domain cgwb_domain;
 #endif
 
-#ifdef CONFIG_INET
-	unsigned long		socket_pressure;
-#endif
-
 	/* List of events which userspace want to receive */
 	struct list_head event_list;
 	spinlock_t event_list_lock;

@@ -615,6 +612,26 @@ static __always_inline void memcg_kmem_put_cache(struct kmem_cache *cachep)
 		__memcg_kmem_put_cache(cachep);
 }
 
+extern struct static_key_false memcg_sockets_enabled_key;
+#define mem_cgroup_sockets_enabled static_branch_unlikely(&memcg_sockets_enabled_key)
+
+struct sock;
+void sock_update_memcg(struct sock *sk);
+void sock_release_memcg(struct sock *sk);
+bool mem_cgroup_charge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages);
+void mem_cgroup_uncharge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages);
+
+static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
+{
+	if (memcg->tcp_mem.memory_pressure)
+		return true;
+	do {
+		if (time_before(jiffies, memcg->socket_pressure))
+			return true;
+	} while ((memcg = parent_mem_cgroup(memcg)));
+	return false;
+}
+
 #else /* CONFIG_MEMCG */
 struct mem_cgroup;
 

@@ -836,6 +853,13 @@ static inline void memcg_kmem_put_cache(struct kmem_cache *cachep)
 {
 }
 
+#define mem_cgroup_sockets_enabled 0
+
+static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
+{
+	return false;
+}
+
 #endif /* CONFIG_MEMCG */
 
 #ifdef CONFIG_CGROUP_WRITEBACK

@@ -863,32 +887,4 @@ static inline void mem_cgroup_wb_stats(struct bdi_writeback *wb,
 
 #endif	/* CONFIG_CGROUP_WRITEBACK */
 
-struct sock;
-void sock_update_memcg(struct sock *sk);
-void sock_release_memcg(struct sock *sk);
-bool mem_cgroup_charge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages);
-void mem_cgroup_uncharge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages);
-#if defined(CONFIG_MEMCG) && defined(CONFIG_INET)
-extern struct static_key_false memcg_sockets_enabled_key;
-#define mem_cgroup_sockets_enabled static_branch_unlikely(&memcg_sockets_enabled_key)
-static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
-{
-#ifdef CONFIG_MEMCG_LEGACY_KMEM
-	if (memcg->tcp_mem.memory_pressure)
-		return true;
-#endif
-	do {
-		if (time_before(jiffies, memcg->socket_pressure))
-			return true;
-	} while ((memcg = parent_mem_cgroup(memcg)));
-	return false;
-}
-#else
-#define mem_cgroup_sockets_enabled 0
-static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
-{
-	return false;
-}
-#endif
-
 #endif /* _LINUX_MEMCONTROL_H */
init/Kconfig +0 -16
index 05440ad..2a739b9 100644
@@ -1040,22 +1040,6 @@ config MEMCG_SWAP_ENABLED
 	  For those who want to have the feature enabled by default should
 	  select this option (if, for some reason, they need to disable it
 	  then swapaccount=0 does the trick).
-config MEMCG_LEGACY_KMEM
-	bool "Legacy Memory Resource Controller Kernel Memory accounting"
-	depends on MEMCG
-	depends on SLUB || SLAB
-	help
-	  The Kernel Memory extension for Memory Resource Controller can limit
-	  the amount of memory used by kernel objects in the system. Those are
-	  fundamentally different from the entities handled by the standard
-	  Memory Controller, which are page-based, and can be swapped. Users of
-	  the kmem extension can use it to guarantee that no group of processes
-	  will ever exhaust kernel resources alone.
-
-	  This option affects the ORIGINAL cgroup interface. The cgroup2 memory
-	  controller includes important in-kernel memory consumers per default.
-
-	  If you're using cgroup2, say N.
 
 config CGROUP_HUGETLB
 	bool "HugeTLB Resource Controller for Control Groups"
mm/memcontrol.c +3 -46

189 lines not on this page · raw message

#1 lore raw
Quoted 10 lines · Johannes Weiner

there has been quite a bit of trouble that stems from dividing our CONFIG space and having to provide real code and dummy functions correctly in all possible combinations. This is amplified by having the legacy mode and the cgroup2 mode in the same file sharing code.

The socket memory and kmem accounting series is a nightmare in that respect, and I'm still in the process of sorting it out. But no matter what the outcome there is going to be, what do you think about getting rid of the CONFIG_MEMCG[_LEGACY]_KMEM and CONFIG_INET stuff?

Quoted 8 lines

Because they end up saving very little and it doesn't seem worth the trouble. CONFIG_MEMCG_LEGACY_KMEM basically allows not compiling the interface structures and the limit updating function. Everything else is included anyway because of cgroup2. And CONFIG_INET also only saves a page_counter and two words in struct mem_cgroup, as well as the tiny socket-specific charge and uncharge wrappers that nobody would call.

Would you be opposed to getting rid of them to simplify things?

That's exactly what I was thinking about while cooking the patch which would get rid of tcp_memcontrol.c, but I was afraid I would be turned down flat, so I dopped the idea :-)

So I'm all for this change. Actually, we already follow the trend when we define kmem and memsw counters even if MEMCG_KMEM/MEMCG_SWAP is disabled, and that's reasonable, because wrapping them in ifdefs would make the code look like hell.

Besides, !CONFIG_INET && CONFIG_MEMCG looks exotic. I doubt such a configuration exists in real life.

...

Quoted 10 lines
init/Kconfig +0 -6
@@ -1040,22 +1040,6 @@ config MEMCG_SWAP_ENABLED
 	  For those who want to have the feature enabled by default should
 	  select this option (if, for some reason, they need to disable it
 	  then swapaccount=0 does the trick).
-config MEMCG_LEGACY_KMEM
-	bool "Legacy Memory Resource Controller Kernel Memory accounting"
-	depends on MEMCG
-	depends on SLUB || SLAB
-	help
-	  The Kernel Memory extension for Memory Resource Controller can limit

init/Kconfig config MEMCG_SWAP_ENABLED · line 1058

Quoted 10 lines
init/Kconfig +0 -10
-	  the amount of memory used by kernel objects in the system. Those are
-	  fundamentally different from the entities handled by the standard
-	  Memory Controller, which are page-based, and can be swapped. Users of
-	  the kmem extension can use it to guarantee that no group of processes
-	  will ever exhaust kernel resources alone.
-
-	  This option affects the ORIGINAL cgroup interface. The cgroup2 memory
-	  controller includes important in-kernel memory consumers per default.
-
-	  If you're using cgroup2, say N.

Hmm, should we hide memory.kmem.* files if this option is disabled? Probably, but it won't do anything bad if we don't.

From a quick glance, the patch looks good to me.

Thanks,
Vladimir

maintainer
#2 lore raw
In reply to Johannes Weiner
Quoted 11 lines · Johannes Weiner

Hey guys,

there has been quite a bit of trouble that stems from dividing our CONFIG space and having to provide real code and dummy functions correctly in all possible combinations. This is amplified by having the legacy mode and the cgroup2 mode in the same file sharing code.

The socket memory and kmem accounting series is a nightmare in that respect, and I'm still in the process of sorting it out. But no matter what the outcome there is going to be, what do you think about getting rid of the CONFIG_MEMCG[_LEGACY]_KMEM and CONFIG_INET stuff?

The code size difference after your recent patches is indeed not that large but that is only because huge part of the kmem code is enabled by default now. I have raised this in the reply to the respective patch. This is ~8K of the code 1K for data. I do understand your reasoning about the complications but this is quite a lot of code. CONFIG_INET ifdefs are probably pointless - they do not add really much and most configs will have it by default. The core for KMEM seems to be a different thing to me. Maybe we can reorganize the code to make the maintenance easier and still allow to enable KMEM accounting separately for kernel size savy users?

[...]

Signature · 8 lines
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author maintainer
#3 lore raw
Quoted 13 lines · Michal Hocko
Quoted 11 lines · Johannes Weiner

Hey guys,

there has been quite a bit of trouble that stems from dividing our CONFIG space and having to provide real code and dummy functions correctly in all possible combinations. This is amplified by having the legacy mode and the cgroup2 mode in the same file sharing code.

The socket memory and kmem accounting series is a nightmare in that respect, and I'm still in the process of sorting it out. But no matter what the outcome there is going to be, what do you think about getting rid of the CONFIG_MEMCG[_LEGACY]_KMEM and CONFIG_INET stuff?

Quoted 10 lines

The code size difference after your recent patches is indeed not that large but that is only because huge part of the kmem code is enabled by default now. I have raised this in the reply to the respective patch. This is ~8K of the code 1K for data. I do understand your reasoning about the complications but this is quite a lot of code. CONFIG_INET ifdefs are probably pointless - they do not add really much and most configs will have it by default. The core for KMEM seems to be a different thing to me. Maybe we can reorganize the code to make the maintenance easier and still allow to enable KMEM accounting separately for kernel size savy users?

Look, if kernel size savvy users care THAT much about TWO pages then they must absolutely LOVE me for having eliminated page_cgroup and saving them THOUSANDS of pages, and deleted hundreds of lines of code and static data in memcontrol.c ever since I started working on it.

Yet this has been the only point you have been bringing up this entire
time: the cost I'm putting on users with all this in both memory and cpu cycles. When I have just made all hotpaths and accounting in memcg completely lockless. And when cgroup2 is going to be a FRACTION of the original memcg code, data size, and runtime cost, even INCLUDING the entirety of the kmem accounting.

There is no perspective to your criticism.

So let's just say I'm going to cash some of that credit I built up in order to get to v2 as fast as possible, without having to spend days engineering a solution to save two damn pages in legacy code, okay?

And if you DO care so much about cost for legacy users beyond this, I think it's time for you to put your money where your mouth is and start sending patches that save those users memory and cpu cycles, instead of constantly demanding this from people who work on making this whole thing much leaner, faster, and cleaner for EVERYBODY.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

maintainer
#4 lore raw
Quoted 25 lines · Johannes Weiner
Quoted 13 lines · Michal Hocko
Quoted 11 lines · Johannes Weiner

Hey guys,

there has been quite a bit of trouble that stems from dividing our CONFIG space and having to provide real code and dummy functions correctly in all possible combinations. This is amplified by having the legacy mode and the cgroup2 mode in the same file sharing code.

The socket memory and kmem accounting series is a nightmare in that respect, and I'm still in the process of sorting it out. But no matter what the outcome there is going to be, what do you think about getting rid of the CONFIG_MEMCG[_LEGACY]_KMEM and CONFIG_INET stuff?

Quoted 10 lines

The code size difference after your recent patches is indeed not that large but that is only because huge part of the kmem code is enabled by default now. I have raised this in the reply to the respective patch. This is ~8K of the code 1K for data. I do understand your reasoning about the complications but this is quite a lot of code. CONFIG_INET ifdefs are probably pointless - they do not add really much and most configs will have it by default. The core for KMEM seems to be a different thing to me. Maybe we can reorganize the code to make the maintenance easier and still allow to enable KMEM accounting separately for kernel size savy users?

Quoted 4 lines

Look, if kernel size savvy users care THAT much about TWO pages then they must absolutely LOVE me for having eliminated page_cgroup and saving them THOUSANDS of pages, and deleted hundreds of lines of code and static data in memcontrol.c ever since I started working on it.

They surely do! And I appreciate that very much as well!

Quoted 3 lines

Yet this has been the only point you have been bringing up this entire
time: the cost I'm putting on users with all this in both memory and cpu cycles.

This is quite an unfair statement, don't you think? I have been reviewing all those changes as deeply as I could and many of them were highly non trivial so it took quite some time. I've raised concerns I had on the way. That doesn't compare to the time you have spent on that of course but I think that reducing all my review feedback to a single thing is really unfair.

Quoted 6 lines

When I have just made all hotpaths and accounting in memcg completely lockless. And when cgroup2 is going to be a FRACTION of the original memcg code, data size, and runtime cost, even INCLUDING the entirety of the kmem accounting.

There is no perspective to your criticism.

This is what we call a review process. Raise concerns and deal with them. My review hasn't implied this would be a show stopper or block those change to get merged. I was merely asking whether we can keep the code size with a _reasonable_ maintenance burden. If the answer is no then I can live with that even when I might not like that fact. That has been reflected by a lack of my acked-by.

Quoted 3 lines

So let's just say I'm going to cash some of that credit I built up in order to get to v2 as fast as possible, without having to spend days engineering a solution to save two damn pages in legacy code, okay?

You sound as if you had to overrule a nack which sounds like over reacting because this is not the case.

Quoted 5 lines

And if you DO care so much about cost for legacy users beyond this, I think it's time for you to put your money where your mouth is and start sending patches that save those users memory and cpu cycles, instead of constantly demanding this from people who work on making this whole thing much leaner, faster, and cleaner for EVERYBODY.

Signature · 2 lines
Michal Hocko
SUSE Labs
author maintainer
#5 lore raw
Quoted 6 lines · Michal Hocko

This is what we call a review process. Raise concerns and deal with them. My review hasn't implied this would be a show stopper or block those change to get merged. I was merely asking whether we can keep the code size with a _reasonable_ maintenance burden. If the answer is no then I can live with that even when I might not like that fact. That has been reflected by a lack of my acked-by.

Everything we do bears a cost, our entire work is making tradeoffs (with a few exceptions where code is just dumb). So when you bring up cost, you have to weigh it against what you're trading off. There is simply no value in saying "this costs X" and nothing else. It's meaningless on its own. Unless X is so unreasonably large that there MUST be another way of doing it, and investing the time is worth it.

8K is not unreasonably large given the history and overall trend of the memcg code. And the only possible tradeoff is to make even more CONFIG options and encourage balkanization of cgroup users, which is hardly a reasonable route to go down. So what exactly ARE you saying when you post `size' results that you don't consider show stoppers? That you don't like increasing the kernel size? Do you think I do?

Pointing out the unexpected (bugs, excessive cost, design problems) is part of a healthy review process. Proposing a different tradeoff, supported by a cost/benefit analysis in both directions is useful.

What's happening here is definitely not a constructive review process.

Quoted 2 lines

You sound as if you had to overrule a nack which sounds like over reacting because this is not the case.

Issues raised are usually considered showstoppers unless it's clear from the way they're raised that we can live with them.

And I'm tired of having the same discussion over and over.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>