mm/memcg: Fix min/low usage in propagate_protected_usage()

There may be cgroup memory overcommitment, it will become even common in the future.

Let's enable kswapd to reclaim low-protected memory in case of memory pressure, to mitigate the global direct reclaim pressures which could cause jitters to the response time of lantency-sensitive groups.

Signed-off-by Xunlei Pang <[email protected]>

---
 mm/vmscan.c | 8 ++++++++
 1 file changed, 8 insertions(+)

mm/vmscan.c +8 -0

index 62ac0c488624..3d412eb91f73 100644
@@ -3531,6 +3531,7 @@ static int balance_pgdat(pg_data_t *pgdat, int order, int classzone_idx)
 
 	count_vm_event(PAGEOUTRUN);
 
+retry:
 	do {
 		unsigned long nr_reclaimed = sc.nr_reclaimed;
 		bool raise_priority = true;

@@ -3622,6 +3623,13 @@ static int balance_pgdat(pg_data_t *pgdat, int order, int classzone_idx)
 			sc.priority--;
 	} while (sc.priority >= 1);
 
+	if (!sc.nr_reclaimed && sc.memcg_low_skipped) {
+		sc.priority = DEF_PRIORITY;
+		sc.memcg_low_reclaim = 1;
+		sc.memcg_low_skipped = 0;
+		goto retry;
+	}
+
 	if (!sc.nr_reclaimed)
 		pgdat->kswapd_failures++;
 

Signature · 1 line

2.13.5 (Apple Git-94)

Quoted 7 lines · Xunlei Pang

There may be cgroup memory overcommitment, it will become even common in the future.

Let's enable kswapd to reclaim low-protected memory in case of memory pressure, to mitigate the global direct reclaim pressures which could cause jitters to the response time of lantency-sensitive groups.

Please be more descriptive about the problem you are trying to handle here. I haven't actually read the patch but let me emphasise that the low limit protection is important isolation tool. And allowing kswapd to reclaim protected memcgs is going to break the semantic as it has been introduced and designed.

Quoted 36 lines

Signed-off-by Xunlei Pang <[email protected]>

---
 mm/vmscan.c | 8 ++++++++
 1 file changed, 8 insertions(+)

mm/vmscan.c +8 -0

index 62ac0c488624..3d412eb91f73 100644
@@ -3531,6 +3531,7 @@ static int balance_pgdat(pg_data_t *pgdat, int order, int classzone_idx)
 
 	count_vm_event(PAGEOUTRUN);
 
+retry:
 	do {
 		unsigned long nr_reclaimed = sc.nr_reclaimed;
 		bool raise_priority = true;

@@ -3622,6 +3623,13 @@ static int balance_pgdat(pg_data_t *pgdat, int order, int classzone_idx)
 			sc.priority--;
 	} while (sc.priority >= 1);
 
+	if (!sc.nr_reclaimed && sc.memcg_low_skipped) {
+		sc.priority = DEF_PRIORITY;
+		sc.memcg_low_reclaim = 1;
+		sc.memcg_low_skipped = 0;
+		goto retry;
+	}
+
 	if (!sc.nr_reclaimed)
 		pgdat->kswapd_failures++;
 

Signature · 1 line

2.13.5 (Apple Git-94)

Quoted 9 lines · Michal Hocko

Quoted 7 lines · Xunlei Pang

There may be cgroup memory overcommitment, it will become even common in the future.

Let's enable kswapd to reclaim low-protected memory in case of memory pressure, to mitigate the global direct reclaim pressures which could cause jitters to the response time of lantency-sensitive groups.

Quoted 5 lines

Please be more descriptive about the problem you are trying to handle here. I haven't actually read the patch but let me emphasise that the low limit protection is important isolation tool. And allowing kswapd to reclaim protected memcgs is going to break the semantic as it has been introduced and designed.

We have two types of memcgs: online groups(important business) and offline groups(unimportant business). Online groups are all configured with MAX low protection, while offline groups are not at all protected(with default 0 low).

When offline groups are overcommitted, the global memory pressure suffers. This will cause the memory allocations from online groups constantly go to the slow global direct reclaim in order to reclaim online's page caches, as kswap is not able to reclaim low-protection memory. low is not hard limit, it's reasonable to be reclaimed by kswapd if there's no other reclaimable memory.

Quoted 21 lines · Xunlei Pang

Quoted 9 lines · Michal Hocko

Quoted 7 lines · Xunlei Pang

There may be cgroup memory overcommitment, it will become even common in the future.

Let's enable kswapd to reclaim low-protected memory in case of memory pressure, to mitigate the global direct reclaim pressures which could cause jitters to the response time of lantency-sensitive groups.

Quoted 5 lines

Please be more descriptive about the problem you are trying to handle here. I haven't actually read the patch but let me emphasise that the low limit protection is important isolation tool. And allowing kswapd to reclaim protected memcgs is going to break the semantic as it has been introduced and designed.

We have two types of memcgs: online groups(important business) and offline groups(unimportant business). Online groups are all configured with MAX low protection, while offline groups are not at all protected(with default 0 low).

Quoted 6 lines

When offline groups are overcommitted, the global memory pressure suffers. This will cause the memory allocations from online groups constantly go to the slow global direct reclaim in order to reclaim online's page caches, as kswap is not able to reclaim low-protection memory. low is not hard limit, it's reasonable to be reclaimed by kswapd if there's no other reclaimable memory.

I am sorry I still do not follow. What role do offline cgroups play. Those are certainly not low mem protected because mem_cgroup_css_offline will reset them to 0.

Quoted 29 lines · Michal Hocko

Quoted 27 lines · Xunlei Pang

Quoted 9 lines · Michal Hocko

Quoted 7 lines · Xunlei Pang

There may be cgroup memory overcommitment, it will become even common in the future.

Let's enable kswapd to reclaim low-protected memory in case of memory pressure, to mitigate the global direct reclaim pressures which could cause jitters to the response time of lantency-sensitive groups.

Quoted 5 lines

Please be more descriptive about the problem you are trying to handle here. I haven't actually read the patch but let me emphasise that the low limit protection is important isolation tool. And allowing kswapd to reclaim protected memcgs is going to break the semantic as it has been introduced and designed.

We have two types of memcgs: online groups(important business) and offline groups(unimportant business). Online groups are all configured with MAX low protection, while offline groups are not at all protected(with default 0 low).

When offline groups are overcommitted, the global memory pressure suffers. This will cause the memory allocations from online groups constantly go to the slow global direct reclaim in order to reclaim online's page caches, as kswap is not able to reclaim low-protection memory. low is not hard limit, it's reasonable to be reclaimed by kswapd if there's no other reclaimable memory.

Quoted 4 lines

I am sorry I still do not follow. What role do offline cgroups play. Those are certainly not low mem protected because mem_cgroup_css_offline will reset them to 0.

Oh, I meant "offline groups" to be "offline-business groups", memcgs refered to here are all "online state" from kernel's perspective.

Quoted 35 lines · Xunlei Pang

Quoted 33 lines · Michal Hocko

Quoted 21 lines · Xunlei Pang

Quoted 9 lines · Michal Hocko

Quoted 7 lines · Xunlei Pang

There may be cgroup memory overcommitment, it will become even common in the future.

Let's enable kswapd to reclaim low-protected memory in case of memory pressure, to mitigate the global direct reclaim pressures which could cause jitters to the response time of lantency-sensitive groups.

Quoted 5 lines

Please be more descriptive about the problem you are trying to handle here. I haven't actually read the patch but let me emphasise that the low limit protection is important isolation tool. And allowing kswapd to reclaim protected memcgs is going to break the semantic as it has been introduced and designed.

We have two types of memcgs: online groups(important business) and offline groups(unimportant business). Online groups are all configured with MAX low protection, while offline groups are not at all protected(with default 0 low).

Quoted 6 lines

When offline groups are overcommitted, the global memory pressure suffers. This will cause the memory allocations from online groups constantly go to the slow global direct reclaim in order to reclaim online's page caches, as kswap is not able to reclaim low-protection memory. low is not hard limit, it's reasonable to be reclaimed by kswapd if there's no other reclaimable memory.

I am sorry I still do not follow. What role do offline cgroups play. Those are certainly not low mem protected because mem_cgroup_css_offline will reset them to 0.

Quoted 2 lines

Oh, I meant "offline groups" to be "offline-business groups", memcgs refered to here are all "online state" from kernel's perspective.

What is offline-business group? Please try to explain the actual problem in much more details and do not let us guess.

When memcgs get reclaimed after its usage exceeds min, some usages below the min may also be reclaimed in the current implementation, the amount is considerably large during kswapd reclaim according to my ftrace results.

Quoted 4 lines · Xunlei Pang

When memcgs get reclaimed after its usage exceeds min, some usages below the min may also be reclaimed in the current implementation, the amount is considerably large during kswapd reclaim according to my ftrace results.

And here again. Describe the setup and the behavior please?

When usage exceeds min, min usage should be min other than 0. Apply the same for low.

Signed-off-by Xunlei Pang <[email protected]>

---
 mm/page_counter.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

mm/page_counter.c +2 -10

index de31470655f6..75d53f15f040 100644
@@ -23,11 +23,7 @@ static void propagate_protected_usage(struct page_counter *c,
 		return;
 
 	if (c->min || atomic_long_read(&c->min_usage)) {
-		if (usage <= c->min)
-			protected = usage;
-		else
-			protected = 0;
-
+		protected = min(usage, c->min);
 		old_protected = atomic_long_xchg(&c->min_usage, protected);
 		delta = protected - old_protected;
 		if (delta)

@@ -35,11 +31,7 @@ static void propagate_protected_usage(struct page_counter *c,
 	}
 
 	if (c->low || atomic_long_read(&c->low_usage)) {
-		if (usage <= c->low)
-			protected = usage;
-		else
-			protected = 0;
-
+		protected = min(usage, c->low);
 		old_protected = atomic_long_xchg(&c->low_usage, protected);
 		delta = protected - old_protected;
 		if (delta)

Signature · 1 line

2.13.5 (Apple Git-94)

Quoted 2 lines · Xunlei Pang

When usage exceeds min, min usage should be min other than 0. Apply the same for low.

Why? What is the actual problem.

Quoted 39 lines

Signed-off-by Xunlei Pang <[email protected]>

---
 mm/page_counter.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

mm/page_counter.c +2 -10

index de31470655f6..75d53f15f040 100644
@@ -23,11 +23,7 @@ static void propagate_protected_usage(struct page_counter *c,
 		return;
 
 	if (c->min || atomic_long_read(&c->min_usage)) {
-		if (usage <= c->min)
-			protected = usage;
-		else
-			protected = 0;
-
+		protected = min(usage, c->min);
 		old_protected = atomic_long_xchg(&c->min_usage, protected);
 		delta = protected - old_protected;
 		if (delta)

@@ -35,11 +31,7 @@ static void propagate_protected_usage(struct page_counter *c,
 	}
 
 	if (c->low || atomic_long_read(&c->low_usage)) {
-		if (usage <= c->low)
-			protected = usage;
-		else
-			protected = 0;
-
+		protected = min(usage, c->low);
 		old_protected = atomic_long_xchg(&c->low_usage, protected);
 		delta = protected - old_protected;
 		if (delta)

Signature · 1 line

2.13.5 (Apple Git-94)

When usage exceeds min, min usage should be min other than 0. Apply the same for low.

Signed-off-by Xunlei Pang <[email protected]>

---
 mm/page_counter.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

mm/page_counter.c +0 -0

index de31470655f6..75d53f15f040 100644

Quoted 13 lines · Xunlei Pang

When usage exceeds min, min usage should be min other than 0. Apply the same for low.

Signed-off-by Xunlei Pang <[email protected]>

---
 mm/page_counter.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

mm/page_counter.c +0 -0

index de31470655f6..75d53f15f040 100644

mm/page_counter.c static void propagate_protected_usage(struct page_counter *c, · line 26

Quoted 9 lines

mm/page_counter.c +1 -5

@@ -23,11 +23,7 @@ static void propagate_protected_usage(struct page_counter *c,
 		return;
 
 	if (c->min || atomic_long_read(&c->min_usage)) {
-		if (usage <= c->min)
-			protected = usage;
-		else
-			protected = 0;
-
+		protected = min(usage, c->min);

This change makes sense in the combination with the patch 3, but not as a standlone "fix". It's not a bug, it's a required thing unless you start scanning proportionally to memory.low/min excess.

Please, reflect this in the commit message. Or, even better, merge it into the patch 3.

Also, please, make sure that cgroup kselftests are passing after your changes.

Hi Roman,

Quoted 28 lines · Roman Gushchin

Quoted 13 lines · Xunlei Pang

When usage exceeds min, min usage should be min other than 0. Apply the same for low.

Signed-off-by Xunlei Pang <[email protected]>

---
 mm/page_counter.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

mm/page_counter.c +0 -0

index de31470655f6..75d53f15f040 100644

mm/page_counter.c static void propagate_protected_usage(struct page_counter *c, · line 26

Quoted 9 lines

mm/page_counter.c +1 -5

@@ -23,11 +23,7 @@ static void propagate_protected_usage(struct page_counter *c,
 		return;
 
 	if (c->min || atomic_long_read(&c->min_usage)) {
-		if (usage <= c->min)
-			protected = usage;
-		else
-			protected = 0;
-
+		protected = min(usage, c->min);

This change makes sense in the combination with the patch 3, but not as a standlone "fix". It's not a bug, it's a required thing unless you start scanning proportionally to memory.low/min excess.

Quoted 2 lines

Please, reflect this in the commit message. Or, even better, merge it into the patch 3.

The more I looked the more I think it's a bug, but anyway I'm fine with merging it into patch 3 :-)