[PATCH v2] memcg: trim the per-cpu charge stock instead of draining it

[PATCH v2] memcg: trim the per-cpu charge stock instead of draining it

[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.

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 <cxiong@meta.com>
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---

Changes since v1:
http://lore.kernel.org/20260817234651.666540-1-shakeel.butt@linux.dev
- Kept the upper limit of memcg stock same as before (Michal)

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

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 17da1f43b7d3..58e4d23cf5e0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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;
 		}
-- 
2.53.0-Meta

Re: [PATCH v2] memcg: trim the per-cpu charge stock instead of draining it

[Michal Hocko]

On Wed 19-08-26 18:20:10, Shakeel Butt wrote:
> 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 <cxiong@meta.com>
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>

Acked-by: Michal Hocko <mhocko@suse.com>
One minor nit

> ---
> 
> Changes since v1:
> http://lore.kernel.org/20260817234651.666540-1-shakeel.butt@linux.dev
> - Kept the upper limit of memcg stock same as before (Michal)
> 
>  mm/memcontrol.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 17da1f43b7d3..58e4d23cf5e0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
[...]
> @@ -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)
> +			}
>  			WRITE_ONCE(stock->nr_pages[i], stock_pages);
> -			if (stock_pages > MEMCG_CHARGE_BATCH)
> -				drain_stock(stock, i);
>  			success = true;
>  			break;
>  		}
> -- 
> 2.53.0-Meta

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: trim the per-cpu charge stock instead of draining it

[Shakeel Butt]

On Thu, Aug 20, 2026 at 09:06:18AM +0200, Michal Hocko wrote:
> On Wed 19-08-26 18:20:10, Shakeel Butt wrote:
> > 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 <cxiong@meta.com>
> > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> 
> Acked-by: Michal Hocko <mhocko@suse.com>

Thanks.

> One minor nit
> 
> > ---
> > 
> > Changes since v1:
> > http://lore.kernel.org/20260817234651.666540-1-shakeel.butt@linux.dev
> > - Kept the upper limit of memcg stock same as before (Michal)
> > 
> >  mm/memcontrol.c | 25 +++++++++++++++++++------
> >  1 file changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 17da1f43b7d3..58e4d23cf5e0 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> [...]
> > @@ -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.

> > +			}
> >  			WRITE_ONCE(stock->nr_pages[i], stock_pages);
> > -			if (stock_pages > MEMCG_CHARGE_BATCH)
> > -				drain_stock(stock, i);
> >  			success = true;
> >  			break;
> >  		}
> > -- 
> > 2.53.0-Meta
> 
> -- 
> Michal Hocko
> SUSE Labs

Re: [PATCH v2] memcg: trim the per-cpu charge stock instead of draining it

[Michal Hocko]

On Thu 20-08-26 09:06:29, Shakeel Butt wrote:
> On Thu, Aug 20, 2026 at 09:06:18AM +0200, Michal Hocko wrote:
> > On Wed 19-08-26 18:20:10, Shakeel Butt wrote:
> > > @@ -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.

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.

> > > +			}
> > >  			WRITE_ONCE(stock->nr_pages[i], stock_pages);
> > > -			if (stock_pages > MEMCG_CHARGE_BATCH)
> > > -				drain_stock(stock, i);
> > >  			success = true;
> > >  			break;
> > >  		}
> > > -- 
> > > 2.53.0-Meta
> > 
> > -- 
> > Michal Hocko
> > SUSE Labs

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: trim the per-cpu charge stock instead of draining it

[Shakeel Butt]

On Thu, Aug 20, 2026 at 08:44:34PM +0200, Michal Hocko wrote:
> On Thu 20-08-26 09:06:29, Shakeel Butt wrote:
> > On Thu, Aug 20, 2026 at 09:06:18AM +0200, Michal Hocko wrote:
> > > On Wed 19-08-26 18:20:10, Shakeel Butt wrote:
> > > > @@ -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.
> 
> 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?