memcg: Don't call schedule_work when no spinning is allowed

PATCH Superseded David Stevens 6 participants 13 messages Aug 31 – Sep 4

Subsystems (3)

Memory management cgroup memcg

lore

Share & tools

lore

David Stevens

author

2026-08-31 23:44 UTC #0 lore raw

PATCH 1/1 memcg: Don't call schedule_work when no spinning is allowed

Memcg charging can be done from any context, but calling schedule_work() isn't safe from an NMI. If memory.high is breached from a context where spinning isn't allowed, use irq_work to schedule the reclaim work.

Fixes 3ac4638a734a ("memcg: make memcg_rstat_updated nmi safe")

Signed-off-by David Stevens <[email protected]>

---
 include/linux/memcontrol.h |  1 +
 mm/memcontrol.c            | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

include/linux/memcontrol.h +1 -0

index 8170bb8066a2..036d973ceca6 100644
@@ -219,6 +219,7 @@ struct mem_cgroup {
 	spinlock_t	 peaks_lock;
 
 	/* Range enforcement for interrupt charges */
+	struct irq_work high_irq_work;
 	struct work_struct high_work;
 
 #ifdef CONFIG_ZSWAP

mm/memcontrol.c +11 -1

index 6dc4888a90f3..5e2f749067cb 100644
@@ -2360,6 +2360,11 @@ static void high_work_func(struct work_struct *work)
 	reclaim_high(memcg, MEMCG_CHARGE_BATCH, GFP_KERNEL);
 }
 
+static void high_irq_work_func(struct irq_work *work)
+{
+	schedule_work(&container_of(work, struct mem_cgroup, high_irq_work)->high_work);
+}
+
 /*
  * Clamp the maximum sleep time per allocation batch to 2 seconds. This is
  * enough to still cause a significant slowdown in most cases, while still

@@ -2752,7 +2757,10 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 		/* Don't bother a random interrupted task */
 		if (!in_task()) {
 			if (mem_high) {
-				schedule_work(&memcg->high_work);
+				if (allow_spinning)
+					schedule_work(&memcg->high_work);
+				else
+					irq_work_queue(&memcg->high_irq_work);
 				break;
 			}
 			continue;

@@ -4129,6 +4137,7 @@ static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
 		goto fail;
 
 	INIT_WORK(&memcg->high_work, high_work_func);
+	init_irq_work(&memcg->high_irq_work, high_irq_work_func);
 	vmpressure_init(&memcg->vmpressure);
 	INIT_LIST_HEAD(&memcg->memory_peaks);
 	INIT_LIST_HEAD(&memcg->swap_peaks);

@@ -4337,6 +4346,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
 		static_branch_dec(&memcg_bpf_enabled_key);
 
 	vmpressure_cleanup(&memcg->vmpressure);
+	irq_work_sync(&memcg->high_irq_work);
 	cancel_work_sync(&memcg->high_work);
 	memcg1_remove_from_trees(memcg);
 	free_shrinker_info(memcg);

base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f

Signature · 1 line

2.55.0.897.gb25b4bd76c-goog

Andrew Morton

maintainer

2026-09-01 00:02 UTC #1 lore raw

Quoted 2 lines · On Mon, 31 Aug 2026 16:43:39 -0700 David Stevens <[email protected]> wrote:

Memcg charging can be done from any context, but calling schedule_work() isn't safe from an NMI.

Who does that. bpf, IIRC?

Quoted 4 lines

If memory.high is breached from a context where spinning isn't allowed, use irq_work to schedule the reclaim work.
Fixes 3ac4638a734a ("memcg: make memcg_rstat_updated nmi safe")

Can anything actually hit this? IOW, should we backport it?

Quoted 1 line

Signed-off-by David Stevens <[email protected]>

Shakeel Butt

maintainer

2026-09-01 00:10 UTC #2 lore raw

In reply to David Stevens

Quoted 6 lines · David Stevens

Memcg charging can be done from any context, but calling schedule_work() isn't safe from an NMI. If memory.high is breached from a context where spinning isn't allowed, use irq_work to schedule the reclaim work.
Fixes 3ac4638a734a ("memcg: make memcg_rstat_updated nmi safe")
Signed-off-by David Stevens <[email protected]>

Did you hit this issue or just code inspection? I assume this is the done_restock code path.

Quoted 10 lines

---
 include/linux/memcontrol.h |  1 +
 mm/memcontrol.c            | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

include/linux/memcontrol.h +0 -0

index 8170bb8066a2..036d973ceca6 100644

include/linux/memcontrol.h struct mem_cgroup { · line 222

Quoted 4 lines

include/linux/memcontrol.h +1 -0

@@ -219,6 +219,7 @@ struct mem_cgroup {
 	spinlock_t	 peaks_lock;
 
 	/* Range enforcement for interrupt charges */
+	struct irq_work high_irq_work;

Instead of adding more complexity, let's just return if we can not spin on done_restock path.

David Stevens

author

2026-09-01 01:05 UTC #3 lore raw

Quoted 11 lines · Shakeel Butt

Quoted 6 lines · David Stevens
Memcg charging can be done from any context, but calling schedule_work() isn't safe from an NMI. If memory.high is breached from a context where spinning isn't allowed, use irq_work to schedule the reclaim work.
Fixes 3ac4638a734a ("memcg: make memcg_rstat_updated nmi safe")
Signed-off-by David Stevens <[email protected]>
Did you hit this issue or just code inspection? I assume this is the done_restock code path.

I just found this via code inspection. I spent a little bit trying to trigger it for real, but the only way I managed was by writing a hacky driver absuing alloc_pages_nolock().

Quoted 12 lines

---
 include/linux/memcontrol.h |  1 +
 mm/memcontrol.c            | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

include/linux/memcontrol.h +0 -0

index 8170bb8066a2..036d973ceca6 100644
@@ -219,6 +219,7 @@ struct mem_cgroup {
      spinlock_t       peaks_lock;

Quoted 6 lines

include/linux/memcontrol.h struct mem_cgroup { · line 222
Quoted 2 lines
include/linux/memcontrol.h +1 -0
      /* Range enforcement for interrupt charges */
+     struct irq_work high_irq_work;
Instead of adding more complexity, let's just return if we can not spin on done_restock path.

There would be no guarantee that memcg reclaim would ever be triggered, which also would also stop MEMCG_HIGH events from being generated. Overall that seems a more serious than just dropping userspace notifications like is done for MEMCG_MAX.

That said, it is very much an edge case. I can send a patch with the simpler fix if dropping the events is preferred.

-David

Michal Hocko

2026-09-01 08:18 UTC #4 lore raw

Quoted 10 lines · David Stevens

Quoted 9 lines · Shakeel Butt
Quoted 6 lines · David Stevens
Memcg charging can be done from any context, but calling schedule_work() isn't safe from an NMI. If memory.high is breached from a context where spinning isn't allowed, use irq_work to schedule the reclaim work.
Fixes 3ac4638a734a ("memcg: make memcg_rstat_updated nmi safe")
Signed-off-by David Stevens <[email protected]>

Quoted 6 lines

Quoted 2 lines
Did you hit this issue or just code inspection? I assume this is the done_restock code path.
I just found this via code inspection. I spent a little bit trying to trigger it for real, but the only way I managed was by writing a hacky driver absuing alloc_pages_nolock().

Then this is not really a fix but rather a new feature.

Quoted 19 lines

Quoted 12 lines
Quoted 12 lines
---
 include/linux/memcontrol.h |  1 +
 mm/memcontrol.c            | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)
include/linux/memcontrol.h +0 -0
index 8170bb8066a2..036d973ceca6 100644
@@ -219,6 +219,7 @@ struct mem_cgroup {
      spinlock_t       peaks_lock;
 
Quoted 6 lines
include/linux/memcontrol.h struct mem_cgroup { · line 222
Quoted 2 lines
include/linux/memcontrol.h +1 -0
      /* Range enforcement for interrupt charges */
+     struct irq_work high_irq_work;
Instead of adding more complexity, let's just return if we can not spin on done_restock path.

Quoted 7 lines

There would be no guarantee that memcg reclaim would ever be triggered, which also would also stop MEMCG_HIGH events from being generated. Overall that seems a more serious than just dropping userspace notifications like is done for MEMCG_MAX.
That said, it is very much an edge case. I can send a patch with the simpler fix if dropping the events is preferred.

Yes, let's go simpler before making this more complex without any actual user.

Signature · 2 lines

Michal Hocko
SUSE Labs

David Stevens

author

2026-09-01 18:08 UTC #5 lore raw

Quoted 12 lines · Michal Hocko

Quoted 10 lines · David Stevens
Quoted 9 lines · Shakeel Butt
Quoted 6 lines · David Stevens
Memcg charging can be done from any context, but calling schedule_work() isn't safe from an NMI. If memory.high is breached from a context where spinning isn't allowed, use irq_work to schedule the reclaim work.
Fixes 3ac4638a734a ("memcg: make memcg_rstat_updated nmi safe")
Signed-off-by David Stevens <[email protected]>

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Did you hit this issue or just code inspection? I assume this is the done_restock code path.
I just found this via code inspection. I spent a little bit trying to trigger it for real, but the only way I managed was by writing a hacky driver absuing alloc_pages_nolock().
Then this is not really a fix but rather a new feature.

I spent a bit longer looking, and this can be hit via bpf_arena_alloc_pages().

-David

Johannes Weiner

maintainer

2026-09-01 14:25 UTC #6 lore raw

In reply to David Stevens

Quoted 36 lines · David Stevens

Quoted 11 lines · Shakeel Butt
Quoted 6 lines · David Stevens
Memcg charging can be done from any context, but calling schedule_work() isn't safe from an NMI. If memory.high is breached from a context where spinning isn't allowed, use irq_work to schedule the reclaim work.
Fixes 3ac4638a734a ("memcg: make memcg_rstat_updated nmi safe")
Signed-off-by David Stevens <[email protected]>
Did you hit this issue or just code inspection? I assume this is the done_restock code path.
I just found this via code inspection. I spent a little bit trying to trigger it for real, but the only way I managed was by writing a hacky driver absuing alloc_pages_nolock().
Quoted 12 lines
Quoted 12 lines
---
 include/linux/memcontrol.h |  1 +
 mm/memcontrol.c            | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)
include/linux/memcontrol.h +0 -0
index 8170bb8066a2..036d973ceca6 100644
@@ -219,6 +219,7 @@ struct mem_cgroup {
      spinlock_t       peaks_lock;
 
Quoted 6 lines
include/linux/memcontrol.h struct mem_cgroup { · line 222
Quoted 2 lines
include/linux/memcontrol.h +1 -0
      /* Range enforcement for interrupt charges */
+     struct irq_work high_irq_work;
Instead of adding more complexity, let's just return if we can not spin on done_restock path.

Quoted 4 lines

There would be no guarantee that memcg reclaim would ever be triggered, which also would also stop MEMCG_HIGH events from being generated. Overall that seems a more serious than just dropping userspace notifications like is done for MEMCG_MAX.

I'm leaning that way too. It's an indefinite error, and it's a freely programmable surface.

IMO, a few lines of relatively straight-forward, self-explanatory code is better than code that needs a comment and leaves a problem that somebody in the future might run into.

Michal Hocko

2026-09-01 15:42 UTC #7 lore raw

Quoted 1 line · Johannes Weiner

On Mon, Aug 31, 2026 at 06:04:57PM -0700, David Stevens wrote:

[...]

Quoted 7 lines

Quoted 4 lines
There would be no guarantee that memcg reclaim would ever be triggered, which also would also stop MEMCG_HIGH events from being generated. Overall that seems a more serious than just dropping userspace notifications like is done for MEMCG_MAX.
I'm leaning that way too. It's an indefinite error, and it's a freely programmable surface.

I am really curious about the indefinite error side of things. It has been my understanding that these NMI safe charges are a) rare and b) there is userspace running so eventually any discrepancies would resolve so the excess is temporary.

Signature · 2 lines

Michal Hocko
SUSE Labs

Johannes Weiner

maintainer

2026-09-01 20:59 UTC #8 lore raw

Quoted 8 lines · Michal Hocko

Quoted 1 line · Johannes Weiner
On Mon, Aug 31, 2026 at 06:04:57PM -0700, David Stevens wrote:
[...]
Quoted 5 lines
Quoted 4 lines
There would be no guarantee that memcg reclaim would ever be triggered, which also would also stop MEMCG_HIGH events from being generated. Overall that seems a more serious than just dropping userspace notifications like is done for MEMCG_MAX.

Quoted 7 lines

Quoted 2 lines
I'm leaning that way too. It's an indefinite error, and it's a freely programmable surface.
I am really curious about the indefinite error side of things. It has been my understanding that these NMI safe charges are a) rare and b) there is userspace running so eventually any discrepancies would resolve so the excess is temporary.

So I think the question is what limits the error in both space and time. When you say it's rare and userspace fixes it, it basically means the answer is: luck of the common case.

But that doesn't help the worst case that can be triggered.

Like I said, if we need to have code to handle that !allow_spinning case anyway, I'd rather just have a few lines of working code than a (lengthy) comment explaining the luck of the common case.

Michal Hocko

Acked-by

2026-09-04 11:26 UTC #9 lore raw

Quoted 12 lines · Johannes Weiner

Quoted 11 lines · Michal Hocko
Quoted 1 line · Johannes Weiner
On Mon, Aug 31, 2026 at 06:04:57PM -0700, David Stevens wrote:
[...]
Quoted 7 lines
Quoted 4 lines
There would be no guarantee that memcg reclaim would ever be triggered, which also would also stop MEMCG_HIGH events from being generated. Overall that seems a more serious than just dropping userspace notifications like is done for MEMCG_MAX.
I'm leaning that way too. It's an indefinite error, and it's a freely programmable surface.

Quoted 8 lines

Quoted 4 lines
I am really curious about the indefinite error side of things. It has been my understanding that these NMI safe charges are a) rare and b) there is userspace running so eventually any discrepancies would resolve so the excess is temporary.
So I think the question is what limits the error in both space and time. When you say it's rare and userspace fixes it, it basically means the answer is: luck of the common case.

Right. I was asking because so far we are trying these allocations as more or less trusted (we do allow them to breach the high limit without any pushback). If there are known scenarios where this could run away then we might need to re-evaluate that. Async reclaim might be just too late in those cases.

Anway...

Quoted 5 lines

But that doesn't help the worst case that can be triggered.
Like I said, if we need to have code to handle that !allow_spinning case anyway, I'd rather just have a few lines of working code than a (lengthy) comment explaining the luck of the common case.

Fair enough. A jump through irq work is not that bad from the complexity POV. So you've convinced me

Acked-by Michal Hocko <[email protected]>

Thanks!

Signature · 2 lines

Michal Hocko
SUSE Labs

Lorenzo Stoakes (ARM)

2026-09-04 16:25 UTC #10 lore raw

In reply to David Stevens

Quoted 6 lines · David Stevens

Memcg charging can be done from any context, but calling schedule_work() isn't safe from an NMI. If memory.high is breached from a context where spinning isn't allowed, use irq_work to schedule the reclaim work.
Fixes 3ac4638a734a ("memcg: make memcg_rstat_updated nmi safe")
Signed-off-by David Stevens <[email protected]>

This is in mm-unstable and breaks the build for me :)

In general - please don't rely on implicit header includes, if you're using functions from a header that's not included, add the include because your local config might happen to import it but another one might be broken.

$ make -j $(nproc) LLVM=1 mm/memcontrol.o
  DESCEND objtool
  CC      arch/x86/kernel/asm-offsets.s
  UPD     include/generated/asm-offsets.h
  CC      kernel/sched/rq-offsets.s
  UPD     include/generated/rq-offsets.h
  CALL    scripts/checksyscalls.sh
  CC      mm/memcontrol.o
mm/memcontrol.c:2859:6: error: call to undeclared function 'irq_work_queue'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
 2859 |                                         irq_work_queue(&memcg->high_irq_work);
      |                                         ^
mm/memcontrol.c:4234:2: error: call to undeclared function 'init_irq_work'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
 4234 |         init_irq_work(&memcg->high_irq_work, high_irq_work_func);
      |         ^
mm/memcontrol.c:4443:2: error: call to undeclared function 'irq_work_sync'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
 4443 |         irq_work_sync(&memcg->high_irq_work);
      |         ^
3 errors generated.
make[3]: *** [scripts/Makefile.build:290: mm/memcontrol.o] Error 1
make[2]: *** [scripts/Makefile.build:551: mm] Error 2
make[1]: *** [/home/lorenzo/kerndev/kernels/mm/Makefile:2229: .] Error 2
make: *** [Makefile:248: __sub-make] Error 2

mm/memcontrol.c

Quoted 21 lines

---
 include/linux/memcontrol.h |  1 +
 mm/memcontrol.c            | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

include/linux/memcontrol.h +1 -0

index 8170bb8066a2..036d973ceca6 100644
@@ -219,6 +219,7 @@ struct mem_cgroup {
 	spinlock_t	 peaks_lock;
 
 	/* Range enforcement for interrupt charges */
+	struct irq_work high_irq_work;
 	struct work_struct high_work;
 
 #ifdef CONFIG_ZSWAP

mm/memcontrol.c +0 -0

index 6dc4888a90f3..5e2f749067cb 100644

You need to add:

#include <linux/irq_work.h> in this file.

Quoted 41 lines

mm/memcontrol.c +11 -1

@@ -2360,6 +2360,11 @@ static void high_work_func(struct work_struct *work)
 	reclaim_high(memcg, MEMCG_CHARGE_BATCH, GFP_KERNEL);
 }
 
+static void high_irq_work_func(struct irq_work *work)
+{
+	schedule_work(&container_of(work, struct mem_cgroup, high_irq_work)->high_work);
+}
+
 /*
  * Clamp the maximum sleep time per allocation batch to 2 seconds. This is
  * enough to still cause a significant slowdown in most cases, while still

@@ -2752,7 +2757,10 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 		/* Don't bother a random interrupted task */
 		if (!in_task()) {
 			if (mem_high) {
-				schedule_work(&memcg->high_work);
+				if (allow_spinning)
+					schedule_work(&memcg->high_work);
+				else
+					irq_work_queue(&memcg->high_irq_work);
 				break;
 			}
 			continue;

@@ -4129,6 +4137,7 @@ static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
 		goto fail;
 
 	INIT_WORK(&memcg->high_work, high_work_func);
+	init_irq_work(&memcg->high_irq_work, high_irq_work_func);
 	vmpressure_init(&memcg->vmpressure);
 	INIT_LIST_HEAD(&memcg->memory_peaks);
 	INIT_LIST_HEAD(&memcg->swap_peaks);

@@ -4337,6 +4346,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
 		static_branch_dec(&memcg_bpf_enabled_key);
 
 	vmpressure_cleanup(&memcg->vmpressure);
+	irq_work_sync(&memcg->high_irq_work);
 	cancel_work_sync(&memcg->high_work);
 	memcg1_remove_from_trees(memcg);
 	free_shrinker_info(memcg);

Quoted 5 lines

base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f --
2.55.0.897.gb25b4bd76c-goog

--
Cheers, Lorenzo

David Stevens

author

2026-09-04 17:33 UTC #11 lore raw

Quoted 6 lines · Lorenzo Stoakes (ARM)

Quoted 4 lines · David Stevens
Memcg charging can be done from any context, but calling schedule_work() isn't safe from an NMI. If memory.high is breached from a context where spinning isn't allowed, use irq_work to schedule the reclaim work.

Quoted 8 lines

Quoted 2 lines
Fixes 3ac4638a734a ("memcg: make memcg_rstat_updated nmi safe")
Signed-off-by David Stevens <[email protected]>
This is in mm-unstable and breaks the build for me :)
In general - please don't rely on implicit header includes, if you're using functions from a header that's not included, add the include because your local config might happen to import it but another one might be broken.

Sorry about that, I'll be sure to be more careful in the future. I've sent out a v2 with the missing includes.

-David

Lorenzo Stoakes (ARM)

2026-09-04 17:59 UTC #12 lore raw

Quoted 10 lines · David Stevens

Quoted 9 lines · Lorenzo Stoakes (ARM)
Quoted 6 lines · David Stevens
Memcg charging can be done from any context, but calling schedule_work() isn't safe from an NMI. If memory.high is breached from a context where spinning isn't allowed, use irq_work to schedule the reclaim work.
Fixes 3ac4638a734a ("memcg: make memcg_rstat_updated nmi safe")
Signed-off-by David Stevens <[email protected]>

Quoted 8 lines

Quoted 5 lines
This is in mm-unstable and breaks the build for me :)
In general - please don't rely on implicit header includes, if you're using functions from a header that's not included, add the include because your local config might happen to import it but another one might be broken.
Sorry about that, I'll be sure to be more careful in the future. I've sent out a v2 with the missing includes.

Thanks!

The kernel headers are a real mess so it's entirely understandable :>) (I think Matthew is doing some work to improve things on this, or at least some of them).

Quoted 2 lines

-David

--
Cheers, Lorenzo