memcg: trim the per-cpu charge stock instead of draining it

PATCH Applied Shakeel Butt 2 participants 6 messages Aug 20 – Aug 21

Subsystems (3)

Memory management cgroup memcg

lore

Share & tools

lore

Shakeel Butt

author maintainer

2026-08-20 01:20 UTC #0 lore raw

PATCH v2 1/1 memcg: trim the per-cpu charge stock instead of draining it

Joy reported that an application generating a request/response traffic pattern spends 44.6% to 57.0% of CPU in the memcg charge/uncharge path for a range of message sizes, against 0.27% to 0.71% outside that range. Running from the root memcg, where socket memory accounting is skipped, recovers the performance.

Tracing the charge path showed that the application generates a pattern where the write syscall charges one page and the read syscall uncharges two pages on the same CPU. This hits a corner case in the memcg percpu stock code that thrashes the stock continuously.

In the memcg percpu stock code, MEMCG_CHARGE_BATCH (64) is both the high watermark and the emptying target, i.e. on a request to charge one page the kernel charges MEMCG_CHARGE_BATCH pages and caches (MEMCG_CHARGE_BATCH - 1) of them in the percpu stock. The following uncharge of 2 pages takes the cached count to (MEMCG_CHARGE_BATCH + 1), and refill_stock() then empties the cache completely. With such a pattern the percpu stock becomes completely ineffective.

Instead of a single boundary point for charges, use the technique the page allocator uses for its own percpu caches, which keeps the watermark and the emptying target apart: nr_pcp_free() frees between batch and high - batch pages, leaving at least pcp->batch on the list. Add a high watermark MEMCG_STOCK_HIGH and, once the cached count goes over it, return only the pages above MEMCG_STOCK_LOW. The watermarks are MEMCG_CHARGE_BATCH apart, so a page_counter update still covers a full batch.

For now, keep MEMCG_STOCK_HIGH same as MEMCG_CHARGE_BATCH and in future we will reevaluate if it makes sense to increase it.

Reported-by Joy Chaoyue Xiong <[email protected]>

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

---

Changes since v1:
http://lore.kernel.org/[email protected]
- Kept the upper limit of memcg stock same as before (Michal)

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

mm/memcontrol.c +19 -6

index 17da1f43b7d3..58e4d23cf5e0 100644
@@ -2048,6 +2048,15 @@ void mem_cgroup_print_oom_group(struct mem_cgroup *memcg)
  * nr_pages in a single cacheline. This may change in future.
  */
 #define NR_MEMCG_STOCK 7
+
+/*
+ * Watermarks for a charge stock slot, in the spirit of pcp->high and
+ * pcp->batch: MEMCG_STOCK_HIGH is the high watermark at which a slot is
+ * trimmed, and it is trimmed down to MEMCG_STOCK_LOW rather than emptied.
+ */
+#define MEMCG_STOCK_LOW		(MEMCG_CHARGE_BATCH / 2)
+#define MEMCG_STOCK_HIGH	(MEMCG_CHARGE_BATCH)
+
 #define FLUSHING_CACHED_CHARGE	0
 struct memcg_stock_pcp {
 	local_trylock_t lock;

@@ -2223,17 +2232,18 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
 {
 	struct memcg_stock_pcp *stock;
 	struct mem_cgroup *cached;
-	uint8_t stock_pages;
+	unsigned int stock_pages;
 	bool success = false;
 	int empty_slot = -1;
 	int i;
 
 	/*
-	 * For now limit MEMCG_CHARGE_BATCH to 127 and less. In future if we
-	 * decide to increase it more than 127 then we will need more careful
-	 * handling of nr_pages[] in struct memcg_stock_pcp.
+	 * nr_pages[] is a uint8_t and a slot's count is capped at
+	 * MEMCG_STOCK_HIGH. Raising MEMCG_CHARGE_BATCH beyond 127 would need
+	 * more careful handling of nr_pages[] in struct memcg_stock_pcp.
 	 */
 	BUILD_BUG_ON(MEMCG_CHARGE_BATCH > S8_MAX);
+	BUILD_BUG_ON(MEMCG_STOCK_HIGH > U8_MAX);
 
 	VM_WARN_ON_ONCE(mem_cgroup_is_root(memcg));

@@ -2254,9 +2264,12 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
 			empty_slot = i;
 		if (memcg == READ_ONCE(stock->cached[i])) {
 			stock_pages = READ_ONCE(stock->nr_pages[i]) + nr_pages;
+			if (stock_pages > MEMCG_STOCK_HIGH) {
+				memcg_uncharge(memcg,
+					       stock_pages - MEMCG_STOCK_LOW);
+				stock_pages = MEMCG_STOCK_LOW;
+			}
 			WRITE_ONCE(stock->nr_pages[i], stock_pages);
-			if (stock_pages > MEMCG_CHARGE_BATCH)
-				drain_stock(stock, i);
 			success = true;
 			break;
 		}

Signature · 1 line

2.53.0-Meta

Michal Hocko

Acked-by on v2 1/1

2026-08-20 07:06 UTC #1 lore raw

Quoted 28 lines · Shakeel Butt

Joy reported that an application generating a request/response traffic pattern spends 44.6% to 57.0% of CPU in the memcg charge/uncharge path for a range of message sizes, against 0.27% to 0.71% outside that range. Running from the root memcg, where socket memory accounting is skipped, recovers the performance.
Tracing the charge path showed that the application generates a pattern where the write syscall charges one page and the read syscall uncharges two pages on the same CPU. This hits a corner case in the memcg percpu stock code that thrashes the stock continuously.
In the memcg percpu stock code, MEMCG_CHARGE_BATCH (64) is both the high watermark and the emptying target, i.e. on a request to charge one page the kernel charges MEMCG_CHARGE_BATCH pages and caches (MEMCG_CHARGE_BATCH - 1) of them in the percpu stock. The following uncharge of 2 pages takes the cached count to (MEMCG_CHARGE_BATCH + 1), and refill_stock() then empties the cache completely. With such a pattern the percpu stock becomes completely ineffective.
Instead of a single boundary point for charges, use the technique the page allocator uses for its own percpu caches, which keeps the watermark and the emptying target apart: nr_pcp_free() frees between batch and high - batch pages, leaving at least pcp->batch on the list. Add a high watermark MEMCG_STOCK_HIGH and, once the cached count goes over it, return only the pages above MEMCG_STOCK_LOW. The watermarks are MEMCG_CHARGE_BATCH apart, so a page_counter update still covers a full batch.

Quoted 5 lines

For now, keep MEMCG_STOCK_HIGH same as MEMCG_CHARGE_BATCH and in future we will reevaluate if it makes sense to increase it.
Reported-by Joy Chaoyue Xiong <[email protected]>
Signed-off-by Shakeel Butt <[email protected]>

Acked-by Michal Hocko <[email protected]>

One minor nit

Quoted 13 lines

---
Changes since v1:
http://lore.kernel.org/[email protected]
- Kept the upper limit of memcg stock same as before (Michal)
 mm/memcontrol.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)
mm/memcontrol.c +0 -0
index 17da1f43b7d3..58e4d23cf5e0 100644

[...]

mm/memcontrol.c static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) · line 2270

Quoted 8 lines

mm/memcontrol.c +4 -0

@@ -2254,9 +2264,12 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
 			empty_slot = i;
 		if (memcg == READ_ONCE(stock->cached[i])) {
 			stock_pages = READ_ONCE(stock->nr_pages[i]) + nr_pages;
+			if (stock_pages > MEMCG_STOCK_HIGH) {
+				memcg_uncharge(memcg,
+					       stock_pages - MEMCG_STOCK_LOW);
+				stock_pages = MEMCG_STOCK_LOW;

I would find it easier to read to keep the update in sync with memcg_uncharge, i.e.
stock_pages = WRITE_ONCE(stock_pages - MEMCG_STOCK_LOW)

Quoted 9 lines

mm/memcontrol.c +1 -2

+			}
 			WRITE_ONCE(stock->nr_pages[i], stock_pages);
-			if (stock_pages > MEMCG_CHARGE_BATCH)
-				drain_stock(stock, i);
 			success = true;
 			break;
 		}

Signature · 1 line

2.53.0-Meta

Signature · 2 lines

Michal Hocko
SUSE Labs

Shakeel Butt

author maintainer

2026-08-20 16:07 UTC #2 lore raw

Quoted 29 lines · Michal Hocko

Quoted 28 lines · Shakeel Butt
Joy reported that an application generating a request/response traffic pattern spends 44.6% to 57.0% of CPU in the memcg charge/uncharge path for a range of message sizes, against 0.27% to 0.71% outside that range. Running from the root memcg, where socket memory accounting is skipped, recovers the performance.
Tracing the charge path showed that the application generates a pattern where the write syscall charges one page and the read syscall uncharges two pages on the same CPU. This hits a corner case in the memcg percpu stock code that thrashes the stock continuously.
In the memcg percpu stock code, MEMCG_CHARGE_BATCH (64) is both the high watermark and the emptying target, i.e. on a request to charge one page the kernel charges MEMCG_CHARGE_BATCH pages and caches (MEMCG_CHARGE_BATCH - 1) of them in the percpu stock. The following uncharge of 2 pages takes the cached count to (MEMCG_CHARGE_BATCH + 1), and refill_stock() then empties the cache completely. With such a pattern the percpu stock becomes completely ineffective.
Instead of a single boundary point for charges, use the technique the page allocator uses for its own percpu caches, which keeps the watermark and the emptying target apart: nr_pcp_free() frees between batch and high - batch pages, leaving at least pcp->batch on the list. Add a high watermark MEMCG_STOCK_HIGH and, once the cached count goes over it, return only the pages above MEMCG_STOCK_LOW. The watermarks are MEMCG_CHARGE_BATCH apart, so a page_counter update still covers a full batch.

Quoted 7 lines

Quoted 5 lines
For now, keep MEMCG_STOCK_HIGH same as MEMCG_CHARGE_BATCH and in future we will reevaluate if it makes sense to increase it.
Reported-by Joy Chaoyue Xiong <[email protected]>
Signed-off-by Shakeel Butt <[email protected]>
Acked-by Michal Hocko <[email protected]>

Thanks.

Quoted 16 lines

One minor nit
Quoted 13 lines
---
Changes since v1:
http://lore.kernel.org/[email protected]
- Kept the upper limit of memcg stock same as before (Michal)
 mm/memcontrol.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)
mm/memcontrol.c +0 -0
index 17da1f43b7d3..58e4d23cf5e0 100644
[...]

Quoted 12 lines

mm/memcontrol.c static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) · line 2270
Quoted 8 lines
mm/memcontrol.c +4 -0
@@ -2254,9 +2264,12 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
 			empty_slot = i;
 		if (memcg == READ_ONCE(stock->cached[i])) {
 			stock_pages = READ_ONCE(stock->nr_pages[i]) + nr_pages;
+			if (stock_pages > MEMCG_STOCK_HIGH) {
+				memcg_uncharge(memcg,
+					       stock_pages - MEMCG_STOCK_LOW);
+				stock_pages = MEMCG_STOCK_LOW;
I would find it easier to read to keep the update in sync with memcg_uncharge, i.e.
stock_pages = WRITE_ONCE(stock_pages - MEMCG_STOCK_LOW)

Sorry I am not sure I understand your suggestion. The WRITE_ONCE() is throwing me off.

Quoted 13 lines

Quoted 9 lines

mm/memcontrol.c +1 -2

+			}
 			WRITE_ONCE(stock->nr_pages[i], stock_pages);
-			if (stock_pages > MEMCG_CHARGE_BATCH)
-				drain_stock(stock, i);
 			success = true;
 			break;
 		}

Signature · 1 line

2.53.0-Meta

Signature · 2 lines

Michal Hocko
SUSE Labs

Michal Hocko

2026-08-20 18:44 UTC #3 lore raw

Quoted 15 lines · Shakeel Butt

Quoted 13 lines · Michal Hocko
mm/memcontrol.c static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) · line 2270
Quoted 8 lines · Shakeel Butt
mm/memcontrol.c +4 -0
@@ -2254,9 +2264,12 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
 			empty_slot = i;
 		if (memcg == READ_ONCE(stock->cached[i])) {
 			stock_pages = READ_ONCE(stock->nr_pages[i]) + nr_pages;
+			if (stock_pages > MEMCG_STOCK_HIGH) {
+				memcg_uncharge(memcg,
+					       stock_pages - MEMCG_STOCK_LOW);
+				stock_pages = MEMCG_STOCK_LOW;
I would find it easier to read to keep the update in sync with memcg_uncharge, i.e.
stock_pages = WRITE_ONCE(stock_pages - MEMCG_STOCK_LOW)

Quoted 2 lines

Sorry I am not sure I understand your suggestion. The WRITE_ONCE() is throwing me off.

Sorry, brainfarth on my end. I meant to say that stock_pages should be updated by the uncharged decrement rather than capping it at MEMCG_STOCK_LOW. So in fact I meant this

				stock_pages = stock_pages - MEMCG_STOCK_LOW;

but then shortcuted it to a nonsense.

Quoted 13 lines

Quoted 9 lines

mm/memcontrol.c +1 -2

+			}
 			WRITE_ONCE(stock->nr_pages[i], stock_pages);
-			if (stock_pages > MEMCG_CHARGE_BATCH)
-				drain_stock(stock, i);
 			success = true;
 			break;
 		}

Signature · 1 line

2.53.0-Meta

Signature · 2 lines

Michal Hocko
SUSE Labs

Signature · 2 lines

Michal Hocko
SUSE Labs

Shakeel Butt

author maintainer

2026-08-20 20:50 UTC #4 lore raw

Quoted 16 lines · Michal Hocko

Quoted 15 lines · Shakeel Butt
Quoted 13 lines · Michal Hocko
mm/memcontrol.c static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) · line 2270
Quoted 8 lines · Shakeel Butt
mm/memcontrol.c +4 -0
@@ -2254,9 +2264,12 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
 			empty_slot = i;
 		if (memcg == READ_ONCE(stock->cached[i])) {
 			stock_pages = READ_ONCE(stock->nr_pages[i]) + nr_pages;
+			if (stock_pages > MEMCG_STOCK_HIGH) {
+				memcg_uncharge(memcg,
+					       stock_pages - MEMCG_STOCK_LOW);
+				stock_pages = MEMCG_STOCK_LOW;
I would find it easier to read to keep the update in sync with memcg_uncharge, i.e.
stock_pages = WRITE_ONCE(stock_pages - MEMCG_STOCK_LOW)

Quoted 8 lines

Quoted 2 lines
Sorry I am not sure I understand your suggestion. The WRITE_ONCE() is throwing me off.
Sorry, brainfarth on my end. I meant to say that stock_pages should be updated by the uncharged decrement rather than capping it at MEMCG_STOCK_LOW. So in fact I meant this
				stock_pages = stock_pages - MEMCG_STOCK_LOW;
but then shortcuted it to a nonsense.

To fully understand your suggestion, let me walkthrough the code:

Let's suppose stock_pages = 65

// Original code does
memcg_uncharge(memcg, stock_pages-MEMCG_STOCK_LOW); // uncharge 33 pages
stock_pages = MEMCG_STOCK_LOW; // stock 32 pages

-----
// You want instead
memcg_uncharge(memcg, MEMCG_STOCK_LOW); // uncharge 32 pages
stock_pages = stock_pages-MEMCG_STOCK_LOW // stock 33 pages

Am I understanding you correctly?

Michal Hocko

2026-08-21 07:45 UTC #5 lore raw

Quoted 34 lines · Shakeel Butt

Quoted 16 lines · Michal Hocko
Quoted 15 lines · Shakeel Butt
Quoted 13 lines · Michal Hocko
mm/memcontrol.c static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) · line 2270
Quoted 8 lines · Shakeel Butt
mm/memcontrol.c +4 -0
@@ -2254,9 +2264,12 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
 			empty_slot = i;
 		if (memcg == READ_ONCE(stock->cached[i])) {
 			stock_pages = READ_ONCE(stock->nr_pages[i]) + nr_pages;
+			if (stock_pages > MEMCG_STOCK_HIGH) {
+				memcg_uncharge(memcg,
+					       stock_pages - MEMCG_STOCK_LOW);
+				stock_pages = MEMCG_STOCK_LOW;
I would find it easier to read to keep the update in sync with memcg_uncharge, i.e.
stock_pages = WRITE_ONCE(stock_pages - MEMCG_STOCK_LOW)
Quoted 8 lines
Quoted 2 lines
Sorry I am not sure I understand your suggestion. The WRITE_ONCE() is throwing me off.
Sorry, brainfarth on my end. I meant to say that stock_pages should be updated by the uncharged decrement rather than capping it at MEMCG_STOCK_LOW. So in fact I meant this
				stock_pages = stock_pages - MEMCG_STOCK_LOW;
but then shortcuted it to a nonsense.
To fully understand your suggestion, let me walkthrough the code:
Let's suppose stock_pages = 65
// Original code does
memcg_uncharge(memcg, stock_pages-MEMCG_STOCK_LOW); // uncharge 33 pages
stock_pages = MEMCG_STOCK_LOW; // stock 32 pages

Quoted 7 lines

-----
// You want instead
memcg_uncharge(memcg, MEMCG_STOCK_LOW); // uncharge 32 pages
stock_pages = stock_pages-MEMCG_STOCK_LOW // stock 33 pages
Am I understanding you correctly?

Nope you are right, I was clearly not thinking straight. Sorry about the noise.

Signature · 2 lines

Michal Hocko
SUSE Labs