memcg: force charge kmem counter too

PATCH In review Shakeel Butt 4 participants 11 messages May 25, 2018 – May 31, 2018

Subsystems (3)

Memory management cgroup memcg

lore

Share & tools

lore

Shakeel Butt

author

2018-05-25 18:55 UTC #0 lore raw

PATCH 1/1 memcg: force charge kmem counter too

Based on several conditions the kernel can decide to force charge an allocation for a memcg i.e. overcharge memcg->memory and memcg->memsw counters. Do the same for memcg->kmem counter too. In cgroup-v1, this bug can cause a __GFP_NOFAIL kmem allocation fail if an explicit limit on kmem counter is set and reached.

Signed-off-by Shakeel Butt <[email protected]>

---
 mm/memcontrol.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

mm/memcontrol.c +19 -2

index ab5673dbfc4e..0a88f824c550 100644
@@ -1893,6 +1893,18 @@ void mem_cgroup_handle_over_high(void)
 	current->memcg_nr_pages_over_high = 0;
 }
 
+/*
+ * Based on try_charge() force charge conditions.
+ */
+static inline bool should_force_charge(gfp_t gfp_mask)
+{
+	return (unlikely(tsk_is_oom_victim(current) ||
+			 fatal_signal_pending(current) ||
+			 current->flags & PF_EXITING ||
+			 current->flags & PF_MEMALLOC ||
+			 gfp_mask & __GFP_NOFAIL));
+}
+
 static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
 		      unsigned int nr_pages)
 {

@@ -2008,6 +2020,8 @@ static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	 * The allocation either can't fail or will lead to more memory
 	 * being freed very soon.  Allow memory usage go over the limit
 	 * temporarily by force charging it.
+	 *
+	 * NOTE: Please keep the should_force_charge() conditions in sync.
 	 */
 	page_counter_charge(&memcg->memory, nr_pages);
 	if (do_memsw_account())

@@ -2331,8 +2345,11 @@ int memcg_kmem_charge_memcg(struct page *page, gfp_t gfp, int order,
 
 	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) &&
 	    !page_counter_try_charge(&memcg->kmem, nr_pages, &counter)) {
-		cancel_charge(memcg, nr_pages);
-		return -ENOMEM;
+		if (!should_force_charge(gfp)) {
+			cancel_charge(memcg, nr_pages);
+			return -ENOMEM;
+		}
+		page_counter_charge(&memcg->kmem, nr_pages);
 	}
 
 	page->mem_cgroup = memcg;

Signature · 1 line

2.17.0.921.gf22659ad46-goog

Vladimir Davydov

2018-05-26 18:51 UTC #1 lore raw

Quoted 5 lines · Shakeel Butt

Based on several conditions the kernel can decide to force charge an allocation for a memcg i.e. overcharge memcg->memory and memcg->memsw counters. Do the same for memcg->kmem counter too. In cgroup-v1, this bug can cause a __GFP_NOFAIL kmem allocation fail if an explicit limit on kmem counter is set and reached.

memory.kmem.limit is broken and unlikely to ever be fixed as this knob was deprecated in cgroup-v2. The fact that hitting the limit doesn't trigger reclaim can result in unexpected behavior from user's pov, like getting ENOMEM while listing a directory. Bypassing the limit for NOFAIL allocations isn't going to fix those problem. So I'd suggest to avoid setting memory.kmem.limit instead of trying to fix it or, even better, switch to cgroup-v2.

Quoted 52 lines

Signed-off-by Shakeel Butt <[email protected]>

---
 mm/memcontrol.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

mm/memcontrol.c +19 -2

index ab5673dbfc4e..0a88f824c550 100644
@@ -1893,6 +1893,18 @@ void mem_cgroup_handle_over_high(void)
 	current->memcg_nr_pages_over_high = 0;
 }
 
+/*
+ * Based on try_charge() force charge conditions.
+ */
+static inline bool should_force_charge(gfp_t gfp_mask)
+{
+	return (unlikely(tsk_is_oom_victim(current) ||
+			 fatal_signal_pending(current) ||
+			 current->flags & PF_EXITING ||
+			 current->flags & PF_MEMALLOC ||
+			 gfp_mask & __GFP_NOFAIL));
+}
+
 static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
 		      unsigned int nr_pages)
 {

@@ -2008,6 +2020,8 @@ static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	 * The allocation either can't fail or will lead to more memory
 	 * being freed very soon.  Allow memory usage go over the limit
 	 * temporarily by force charging it.
+	 *
+	 * NOTE: Please keep the should_force_charge() conditions in sync.
 	 */
 	page_counter_charge(&memcg->memory, nr_pages);
 	if (do_memsw_account())

@@ -2331,8 +2345,11 @@ int memcg_kmem_charge_memcg(struct page *page, gfp_t gfp, int order,
 
 	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) &&
 	    !page_counter_try_charge(&memcg->kmem, nr_pages, &counter)) {
-		cancel_charge(memcg, nr_pages);
-		return -ENOMEM;
+		if (!should_force_charge(gfp)) {
+			cancel_charge(memcg, nr_pages);
+			return -ENOMEM;
+		}
+		page_counter_charge(&memcg->kmem, nr_pages);
 	}
 
 	page->mem_cgroup = memcg;

Shakeel Butt

author

2018-05-26 22:37 UTC #2 lore raw

Quoted 7 lines · Vladimir Davydov

Quoted 5 lines · Shakeel Butt
Based on several conditions the kernel can decide to force charge an allocation for a memcg i.e. overcharge memcg->memory and memcg->memsw counters. Do the same for memcg->kmem counter too. In cgroup-v1, this bug can cause a __GFP_NOFAIL kmem allocation fail if an explicit limit on kmem counter is set and reached.

Quoted 5 lines

memory.kmem.limit is broken and unlikely to ever be fixed as this knob was deprecated in cgroup-v2. The fact that hitting the limit doesn't trigger reclaim can result in unexpected behavior from user's pov, like getting ENOMEM while listing a directory. Bypassing the limit for NOFAIL allocations isn't going to fix those problem.

I understand that fixing NOFAIL will not fix all other issues but it still is better than current situation. IMHO we should keep fixing kmem bit by bit.

One crazy idea is to just break it completely by force charging all the time.

Michal Hocko

maintainer

2018-05-28 09:11 UTC #3 lore raw

Quoted 15 lines · Shakeel Butt

Quoted 7 lines · Vladimir Davydov
Quoted 5 lines · Shakeel Butt
Based on several conditions the kernel can decide to force charge an allocation for a memcg i.e. overcharge memcg->memory and memcg->memsw counters. Do the same for memcg->kmem counter too. In cgroup-v1, this bug can cause a __GFP_NOFAIL kmem allocation fail if an explicit limit on kmem counter is set and reached.
Quoted 5 lines
memory.kmem.limit is broken and unlikely to ever be fixed as this knob was deprecated in cgroup-v2. The fact that hitting the limit doesn't trigger reclaim can result in unexpected behavior from user's pov, like getting ENOMEM while listing a directory. Bypassing the limit for NOFAIL allocations isn't going to fix those problem.

Quoted 5 lines

I understand that fixing NOFAIL will not fix all other issues but it still is better than current situation. IMHO we should keep fixing kmem bit by bit.
One crazy idea is to just break it completely by force charging all the time.

What is the limit good for then? Accounting?

Signature · 2 lines

Michal Hocko
SUSE Labs

Shakeel Butt

author

2018-05-28 17:23 UTC #4 lore raw

Quoted 16 lines · Michal Hocko

Quoted 15 lines · Shakeel Butt
Quoted 12 lines · Vladimir Davydov
Quoted 5 lines · Shakeel Butt
Based on several conditions the kernel can decide to force charge an allocation for a memcg i.e. overcharge memcg->memory and memcg->memsw counters. Do the same for memcg->kmem counter too. In cgroup-v1, this bug can cause a __GFP_NOFAIL kmem allocation fail if an explicit limit on kmem counter is set and reached.
memory.kmem.limit is broken and unlikely to ever be fixed as this knob was deprecated in cgroup-v2. The fact that hitting the limit doesn't trigger reclaim can result in unexpected behavior from user's pov, like getting ENOMEM while listing a directory. Bypassing the limit for NOFAIL allocations isn't going to fix those problem.

Quoted 8 lines

Quoted 5 lines
I understand that fixing NOFAIL will not fix all other issues but it still is better than current situation. IMHO we should keep fixing kmem bit by bit.
One crazy idea is to just break it completely by force charging all the time.
What is the limit good for then? Accounting?

Unlike tcpmem, the kmem accounting is enabled by default. No need to set the limit to enable accounting.

I think my crazy idea was just wrong and without much thought. Though is there a precedence where the broken feature is not fixed because an alternative is available?

Michal Hocko

maintainer

2018-05-29 08:31 UTC #5 lore raw

Quoted 3 lines · Shakeel Butt

On Mon, May 28, 2018 at 2:11 AM, Michal Hocko <[email protected]> wrote:
Though is there a precedence where the broken feature is not fixed because an alternative is available?

Well, I can see how breaking GFP_NOFAIL semantic is problematic, on the other hand we keep saying that kmem accounting in v1 is hard usable and strongly discourage people from using it. Sure we can add the code which handles _this_ particular case but that wouldn't make the whole thing more usable I strongly suspect. Maybe I am wrong and you can provide some specific examples. Is GFP_NOFAIL that common to matter?

In any case we should balance between the code maintainability here. Adding more cruft into the allocator path is not free.

Signature · 2 lines

Michal Hocko
SUSE Labs

Shakeel Butt

author

2018-05-30 18:14 UTC #6 lore raw

Quoted 12 lines · Michal Hocko

Quoted 3 lines · Shakeel Butt
On Mon, May 28, 2018 at 2:11 AM, Michal Hocko <[email protected]> wrote:
Though is there a precedence where the broken feature is not fixed because an alternative is available?
Well, I can see how breaking GFP_NOFAIL semantic is problematic, on the other hand we keep saying that kmem accounting in v1 is hard usable and strongly discourage people from using it. Sure we can add the code which handles _this_ particular case but that wouldn't make the whole thing more usable I strongly suspect. Maybe I am wrong and you can provide some specific examples. Is GFP_NOFAIL that common to matter?

Quoted 3 lines

In any case we should balance between the code maintainability here. Adding more cruft into the allocator path is not free.

We do not use kmem limits internally and this is something I found through code inspection. If this patch is increasing the cost of code maintainability I am fine with dropping it but at least there should a comment saying that kmem limits are broken and no need fix.

Shakeel

Minchan Kim

2018-05-31 06:01 UTC #7 lore raw

Quoted 13 lines · Shakeel Butt

Quoted 12 lines · Michal Hocko
Quoted 3 lines · Shakeel Butt
On Mon, May 28, 2018 at 2:11 AM, Michal Hocko <[email protected]> wrote:
Though is there a precedence where the broken feature is not fixed because an alternative is available?
Well, I can see how breaking GFP_NOFAIL semantic is problematic, on the other hand we keep saying that kmem accounting in v1 is hard usable and strongly discourage people from using it. Sure we can add the code which handles _this_ particular case but that wouldn't make the whole thing more usable I strongly suspect. Maybe I am wrong and you can provide some specific examples. Is GFP_NOFAIL that common to matter?

Quoted 8 lines

Quoted 3 lines
In any case we should balance between the code maintainability here. Adding more cruft into the allocator path is not free.
We do not use kmem limits internally and this is something I found through code inspection. If this patch is increasing the cost of code maintainability I am fine with dropping it but at least there should a comment saying that kmem limits are broken and no need fix.

I agree.

Even, I didn't know kmem is strongly discouraged until now. Then, why is it enabled by default on cgroup v1?

Let's turn if off with comment "It's broken so do not use/fix. Instead, please move to cgroup v2".

Michal Hocko

maintainer

2018-05-31 06:56 UTC #8 lore raw

Quoted 23 lines · Minchan Kim

Quoted 13 lines · Shakeel Butt
Quoted 12 lines · Michal Hocko
Quoted 3 lines · Shakeel Butt
On Mon, May 28, 2018 at 2:11 AM, Michal Hocko <[email protected]> wrote:
Though is there a precedence where the broken feature is not fixed because an alternative is available?
Well, I can see how breaking GFP_NOFAIL semantic is problematic, on the other hand we keep saying that kmem accounting in v1 is hard usable and strongly discourage people from using it. Sure we can add the code which handles _this_ particular case but that wouldn't make the whole thing more usable I strongly suspect. Maybe I am wrong and you can provide some specific examples. Is GFP_NOFAIL that common to matter?
Quoted 8 lines
Quoted 3 lines
In any case we should balance between the code maintainability here. Adding more cruft into the allocator path is not free.
We do not use kmem limits internally and this is something I found through code inspection. If this patch is increasing the cost of code maintainability I am fine with dropping it but at least there should a comment saying that kmem limits are broken and no need fix.

Quoted 4 lines

I agree.
Even, I didn't know kmem is strongly discouraged until now. Then, why is it enabled by default on cgroup v1?

You have to set a non-zero limit to make it active IIRC. The code is compiled in because v2 enables it by default.

Quoted 2 lines

Let's turn if off with comment "It's broken so do not use/fix. Instead, please move to cgroup v2".

Signature · 2 lines

Michal Hocko
SUSE Labs

Minchan Kim

2018-05-31 08:23 UTC #9 lore raw

Quoted 24 lines · Michal Hocko

Quoted 23 lines · Minchan Kim
Quoted 21 lines · Shakeel Butt
Quoted 12 lines · Michal Hocko
Quoted 3 lines · Shakeel Butt
On Mon, May 28, 2018 at 2:11 AM, Michal Hocko <[email protected]> wrote:
Though is there a precedence where the broken feature is not fixed because an alternative is available?
Well, I can see how breaking GFP_NOFAIL semantic is problematic, on the other hand we keep saying that kmem accounting in v1 is hard usable and strongly discourage people from using it. Sure we can add the code which handles _this_ particular case but that wouldn't make the whole thing more usable I strongly suspect. Maybe I am wrong and you can provide some specific examples. Is GFP_NOFAIL that common to matter?
Quoted 3 lines
In any case we should balance between the code maintainability here. Adding more cruft into the allocator path is not free.
We do not use kmem limits internally and this is something I found through code inspection. If this patch is increasing the cost of code maintainability I am fine with dropping it but at least there should a comment saying that kmem limits are broken and no need fix.

Quoted 6 lines

Quoted 4 lines
I agree.
Even, I didn't know kmem is strongly discouraged until now. Then, why is it enabled by default on cgroup v1?
You have to set a non-zero limit to make it active IIRC. The code is

Maybe, no. I didn't set anything. IOW, it was a default without any setting and I hit this as you can remember.
http://lkml.kernel.org/r/<[email protected]>
We don't need to allocate memory for stuff even maintainers discourage.

Quoted 8 lines

compiled in because v2 enables it by default.
Quoted 2 lines
Let's turn if off with comment "It's broken so do not use/fix. Instead, please move to cgroup v2".
Signature · 2 lines
Michal Hocko
SUSE Labs

Michal Hocko

maintainer

2018-05-31 08:51 UTC #10 lore raw

Quoted 30 lines · Minchan Kim

Quoted 29 lines · Michal Hocko
Quoted 27 lines · Minchan Kim
Quoted 13 lines · Shakeel Butt
Quoted 12 lines · Michal Hocko
Quoted 3 lines · Shakeel Butt
On Mon, May 28, 2018 at 2:11 AM, Michal Hocko <[email protected]> wrote:
Though is there a precedence where the broken feature is not fixed because an alternative is available?
Well, I can see how breaking GFP_NOFAIL semantic is problematic, on the other hand we keep saying that kmem accounting in v1 is hard usable and strongly discourage people from using it. Sure we can add the code which handles _this_ particular case but that wouldn't make the whole thing more usable I strongly suspect. Maybe I am wrong and you can provide some specific examples. Is GFP_NOFAIL that common to matter?
Quoted 8 lines
Quoted 3 lines
In any case we should balance between the code maintainability here. Adding more cruft into the allocator path is not free.
We do not use kmem limits internally and this is something I found through code inspection. If this patch is increasing the cost of code maintainability I am fine with dropping it but at least there should a comment saying that kmem limits are broken and no need fix.
I agree.
Even, I didn't know kmem is strongly discouraged until now. Then, why is it enabled by default on cgroup v1?

Quoted 6 lines

Quoted 1 line
You have to set a non-zero limit to make it active IIRC. The code is
Maybe, no. I didn't set anything. IOW, it was a default without any setting and I hit this as you can remember.
http://lkml.kernel.org/r/<[email protected]>
We don't need to allocate memory for stuff even maintainers discourage.

Well, you can disable the whole kmem accounting by cgroup.memory=nokmem kernel parameter. I wasn't very happy when we removed the config option which would have put all the tracking aside even when no limit is set but the argument was that the ifdefery was growing into an unmaintainable mess and I have to agree with that. So here we are...

Signature · 2 lines

Michal Hocko
SUSE Labs