mm: rework memcg kernel stack accounting

I've noticed, that dying memory cgroups are often pinned in memory by a single pagecache page. Even under moderate memory pressure they sometimes stayed in such state for a long time. That looked strange.

My investigation showed that the problem is caused by applying the LRU pressure balancing math:

scan = div64_u64(scan * fraction[lru], denominator),

where

denominator = fraction[anon] + fraction[file] + 1.

Because fraction[lru] is always less than denominator, if the initial scan size is 1, the result is always 0.

This means the last page is not scanned and has no chances to be reclaimed.

Fix this by rounding up the result of the division.

In practice this change significantly improves the speed of dying cgroups reclaim.

...

include/linux/math64.h +0 -0

 #endif /* _LINUX_MATH64_H */

mm/vmscan.c +4 -2

index d649b242b989..2c67a0121c6d 100644
@@ -2446,9 +2446,11 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 			/*
 			 * Scan types proportional to swappiness and
 			 * their relative recent reclaim efficiency.
+			 * Make sure we don't miss the last page
+			 * because of a round-off error.
 			 */
-			scan = div64_u64(scan * fraction[file],
-					 denominator);
+			scan = DIV64_U64_ROUND_UP(scan * fraction[file],
+						  denominator);
 			break;
 		case SCAN_FILE:
 		case SCAN_ANON:

Quoted 33 lines · On Mon, 27 Aug 2018 09:26:21 -0700 Roman Gushchin <[email protected]> wrote:

I've noticed, that dying memory cgroups are often pinned in memory by a single pagecache page. Even under moderate memory pressure they sometimes stayed in such state for a long time. That looked strange.

My investigation showed that the problem is caused by applying the LRU pressure balancing math:

scan = div64_u64(scan * fraction[lru], denominator),

where

denominator = fraction[anon] + fraction[file] + 1.

Because fraction[lru] is always less than denominator, if the initial scan size is 1, the result is always 0.

This means the last page is not scanned and has no chances to be reclaimed.

Fix this by rounding up the result of the division.

In practice this change significantly improves the speed of dying cgroups reclaim.

...

include/linux/math64.h +0 -0

@@ -281,4 +281,6 @@ static inline u64 mul_u64_u32_div(u64 a, u32 mul, u32 divisor)
 }
 #endif /* mul_u64_u32_div */
 

include/linux/math64.h static inline u64 mul_u64_u32_div(u64 a, u32 mul, u32 divisor) · line 284

Quoted 1 line

include/linux/math64.h +1 -0

+#define DIV64_U64_ROUND_UP(ll, d)	div64_u64((ll) + (d) - 1, (d))

This macro references arg `d' more than once. That can cause problems if the passed expression has side-effects and is poor practice. Can we please redo this with a temporary?

Quoted 33 lines · On Mon, 27 Aug 2018 09:26:21 -0700 Roman Gushchin <[email protected]> wrote:

I've noticed, that dying memory cgroups are often pinned in memory by a single pagecache page. Even under moderate memory pressure they sometimes stayed in such state for a long time. That looked strange.

My investigation showed that the problem is caused by applying the LRU pressure balancing math:

scan = div64_u64(scan * fraction[lru], denominator),

where

denominator = fraction[anon] + fraction[file] + 1.

Because fraction[lru] is always less than denominator, if the initial scan size is 1, the result is always 0.

This means the last page is not scanned and has no chances to be reclaimed.

Fix this by rounding up the result of the division.

In practice this change significantly improves the speed of dying cgroups reclaim.

...

include/linux/math64.h +0 -0

@@ -281,4 +281,6 @@ static inline u64 mul_u64_u32_div(u64 a, u32 mul, u32 divisor)
 }
 #endif /* mul_u64_u32_div */
 

include/linux/math64.h static inline u64 mul_u64_u32_div(u64 a, u32 mul, u32 divisor) · line 284

Quoted 1 line

include/linux/math64.h +1 -0

+#define DIV64_U64_ROUND_UP(ll, d)	div64_u64((ll) + (d) - 1, (d))

This macro references arg `d' more than once. That can cause problems if the passed expression has side-effects and is poor practice. Can we please redo this with a temporary?

Quoted 1 line

From d8237d3df222e6c5a98a74baa04bc52edf8a3677 Mon Sep 17 00:00:00 2001

From: Roman Gushchin <[email protected]>
Date: Wed, 29 Aug 2018 14:14:48 -0700
Subject: [PATCH] math64: prevent double calculation of DIV64_U64_ROUND_UP()
arguments

Cause the DIV64_U64_ROUND_UP(ll, d) macro to cache the result of (d) expression in a local variable to avoid double calculation, which might bring unexpected side effects.

Signed-off-by Roman Gushchin <[email protected]>

---
 include/linux/math64.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

include/linux/math64.h +0 -0

index 94af3d9c73e7..bb2c84afb80c 100644

If CONFIG_VMAP_STACK is set, kernel stacks are allocated using __vmalloc_node_range() with __GFP_ACCOUNT. So kernel stack pages are charged against corresponding memory cgroups on allocation and uncharged on releasing them.

The problem is that we do cache kernel stacks in small per-cpu caches and do reuse them for new tasks, which can belong to different memory cgroups.

Each stack page still holds a reference to the original cgroup, so the cgroup can't be released until the vmap area is released.

To make this happen we need more than two subsequent exits without forks in between on the current cpu, which makes it very unlikely to happen. As a result, I saw a significant number of dying cgroups (in theory, up to 2 * number_of_cpu + number_of_tasks), which can't be released even by significant memory pressure.

As a cgroup structure can take a significant amount of memory (first of all, per-cpu data like memcg statistics), it leads to a noticeable waste of memory.

...

kernel/fork.c +0 -0

@@ -371,6 +382,35 @@ static void account_kernel_stack(struct task_struct *tsk, int account)
 	}
 }
 

kernel/fork.c +18 -0

+			/*
+			 * If memcg_kmem_charge() fails, page->mem_cgroup
+			 * pointer is NULL, and both memcg_kmem_uncharge()
+			 * and mod_memcg_page_state() in free_thread_stack()
+			 * will ignore this page. So it's safe.
+			 */
+			ret = memcg_kmem_charge(vm->pages[i], GFP_KERNEL, 0);
+			if (ret)
+				return ret;
+
+			mod_memcg_page_state(vm->pages[i],
+					     MEMCG_KERNEL_STACK_KB,
+					     PAGE_SIZE / 1024);
+		}
+	}
+#endif
+	return 0;
+}
 
...

Quoted 19 lines · On Mon, 27 Aug 2018 09:26:19 -0700 Roman Gushchin <[email protected]> wrote:

If CONFIG_VMAP_STACK is set, kernel stacks are allocated using __vmalloc_node_range() with __GFP_ACCOUNT. So kernel stack pages are charged against corresponding memory cgroups on allocation and uncharged on releasing them.

The problem is that we do cache kernel stacks in small per-cpu caches and do reuse them for new tasks, which can belong to different memory cgroups.

Each stack page still holds a reference to the original cgroup, so the cgroup can't be released until the vmap area is released.

To make this happen we need more than two subsequent exits without forks in between on the current cpu, which makes it very unlikely to happen. As a result, I saw a significant number of dying cgroups (in theory, up to 2 * number_of_cpu + number_of_tasks), which can't be released even by significant memory pressure.

Quoted 3 lines

As a cgroup structure can take a significant amount of memory (first of all, per-cpu data like memcg statistics), it leads to a noticeable waste of memory.

OK, but this doesn't describe how the patch addresses this issue?

Quoted 8 lines

...

(quoted diff) +0 -0

@@ -371,6 +382,35 @@ static void account_kernel_stack(struct task_struct *tsk, int account)
 	}
 }
 

Review static void account_kernel_stack(struct task_struct *tsk, int account) · line 394

Quoted 10 lines

(quoted diff) +10 -0

+static int memcg_charge_kernel_stack(struct task_struct *tsk)
+{
+#ifdef CONFIG_VMAP_STACK
+	struct vm_struct *vm = task_stack_vm_area(tsk);
+	int ret;
+
+	if (vm) {
+		int i;
+
+		for (i = 0; i < THREAD_SIZE / PAGE_SIZE; i++) {

Can we ever have THREAD_SIZE < PAGE_SIZE? 64k pages?

Quoted 20 lines

(quoted diff) +18 -0

+			/*
+			 * If memcg_kmem_charge() fails, page->mem_cgroup
+			 * pointer is NULL, and both memcg_kmem_uncharge()
+			 * and mod_memcg_page_state() in free_thread_stack()
+			 * will ignore this page. So it's safe.
+			 */
+			ret = memcg_kmem_charge(vm->pages[i], GFP_KERNEL, 0);
+			if (ret)
+				return ret;
+
+			mod_memcg_page_state(vm->pages[i],
+					     MEMCG_KERNEL_STACK_KB,
+					     PAGE_SIZE / 1024);
+		}
+	}
+#endif
+	return 0;
+}
 

Quoted 2 lines

...