memcg: clean up memory.high enforcement

try_charge_memcg() does a lot. It charges the page counters, reclaims when a limit is hit, may call the OOM killer, and at the end it checks and enforces memory.high. Move the memory.high part into a new function, memcg_enforce_high(), so that try_charge_memcg() is shorter and the memory.high code is easier to read and change.

The code and its comments move as they are. The only difference is that the new function gets allow_spinning from gfp_mask itself.

No functional change.
---

Quoted 6 lines · Shakeel Butt

try_charge_memcg() does a lot. It charges the page counters, reclaims when a limit is hit, may call the OOM killer, and at the end it checks and enforces memory.high. Move the memory.high part into a new function, memcg_enforce_high(), so that try_charge_memcg() is shorter and the memory.high code is easier to read and change.

Quoted 5 lines

The code and its comments move as they are. The only difference is that the new function gets allow_spinning from gfp_mask itself.

No functional change.
---

Missing Signed-off-by: ?

Otherwise LGTM.

Reviewed-by Harry Yoo (Meta) <[email protected]>

try_charge_memcg() does a lot. It charges the page counters, reclaims when a limit is hit, may call the OOM killer, and at the end it checks and enforces memory.high. Move the memory.high part into a new function, memcg_enforce_high(), so that try_charge_memcg() is shorter and the memory.high code is easier to read and change.

The code and its comments move as they are. The only difference is that the new function gets allow_spinning from gfp_mask itself.

No functional change.

---
 mm/memcontrol.c | 125 ++++++++++++++++++++++++++----------------------
 1 file changed, 69 insertions(+), 56 deletions(-)

mm/memcontrol.c +69 -56

index aad0498a7bd6..20c5dfe1a894 100644
@@ -2696,6 +2696,74 @@ void __mem_cgroup_handle_over_high(gfp_t gfp_mask)
 	css_put(&memcg->css);
 }
 
+/*
+ * Called after @nr_pages were charged to @memcg. If @memcg or one of its
+ * ancestors is over memory.high or memory.swap.high, start reclaim or slow
+ * down the task.
+ */
+static void memcg_enforce_high(struct mem_cgroup *memcg, unsigned int nr_pages,
+			       gfp_t gfp_mask)
+{
+	bool allow_spinning = gfpflags_allow_spinning(gfp_mask);
+
+	/*
+	 * If the hierarchy is above the normal consumption range, schedule
+	 * reclaim on returning to userland.  We can perform reclaim here
+	 * if __GFP_RECLAIM but let's always punt for simplicity and so that
+	 * GFP_KERNEL can consistently be used during reclaim.  @memcg is
+	 * not recorded as it most likely matches current's and won't
+	 * change in the meantime.  As high limit is checked again before
+	 * reclaim, the cost of mismatch is negligible.
+	 */
+	do {
+		bool mem_high, swap_high;
+
+		mem_high = page_counter_read(&memcg->memory) >
+			READ_ONCE(memcg->memory.high);
+		swap_high = page_counter_read(&memcg->swap) >
+			READ_ONCE(memcg->swap.high);
+
+		/* Don't bother a random interrupted task */
+		if (!in_task()) {
+			if (mem_high) {
+				if (allow_spinning)
+					schedule_work(&memcg->high_work);
+				else
+					irq_work_queue(&memcg->high_irq_work);
+				break;
+			}
+			continue;
+		}
+
+		if (mem_high || swap_high) {
+			/*
+			 * The allocating tasks in this cgroup will need to do
+			 * reclaim or be throttled to prevent further growth
+			 * of the memory or swap footprints.
+			 *
+			 * Target some best-effort fairness between the tasks,
+			 * and distribute reclaim work and delay penalties
+			 * based on how much each task is actually allocating.
+			 */
+			current->memcg_nr_pages_over_high += nr_pages;
+			set_notify_resume(current);
+			break;
+		}
+	} while ((memcg = parent_mem_cgroup(memcg)));
+
+	/*
+	 * Reclaim is set up above to be called from the userland
+	 * return path. But also attempt synchronous reclaim to avoid
+	 * excessive overrun while the task is still inside the
+	 * kernel. If this is successful, the return path will see it
+	 * when it rechecks the overage and simply bail out.
+	 */
+	if (current->memcg_nr_pages_over_high > MEMCG_CHARGE_BATCH &&
+	    !(current->flags & PF_MEMALLOC) &&
+	    gfpflags_allow_blocking(gfp_mask))
+		__mem_cgroup_handle_over_high(gfp_mask);
+}
+
 static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 			    unsigned int nr_pages)
 {

@@ -2853,62 +2921,7 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	if (batch > nr_pages)
 		refill_stock(memcg, batch - nr_pages);
 
-	/*
-	 * If the hierarchy is above the normal consumption range, schedule
-	 * reclaim on returning to userland.  We can perform reclaim here
-	 * if __GFP_RECLAIM but let's always punt for simplicity and so that
-	 * GFP_KERNEL can consistently be used during reclaim.  @memcg is
-	 * not recorded as it most likely matches current's and won't
-	 * change in the meantime.  As high limit is checked again before
-	 * reclaim, the cost of mismatch is negligible.
-	 */
-	do {
-		bool mem_high, swap_high;
-
-		mem_high = page_counter_read(&memcg->memory) >
-			READ_ONCE(memcg->memory.high);
-		swap_high = page_counter_read(&memcg->swap) >
-			READ_ONCE(memcg->swap.high);
-
-		/* Don't bother a random interrupted task */
-		if (!in_task()) {
-			if (mem_high) {
-				if (allow_spinning)
-					schedule_work(&memcg->high_work);
-				else
-					irq_work_queue(&memcg->high_irq_work);
-				break;
-			}
-			continue;
-		}
-
-		if (mem_high || swap_high) {
-			/*
-			 * The allocating tasks in this cgroup will need to do
-			 * reclaim or be throttled to prevent further growth
-			 * of the memory or swap footprints.
-			 *
-			 * Target some best-effort fairness between the tasks,
-			 * and distribute reclaim work and delay penalties
-			 * based on how much each task is actually allocating.
-			 */
-			current->memcg_nr_pages_over_high += batch;
-			set_notify_resume(current);
-			break;
-		}
-	} while ((memcg = parent_mem_cgroup(memcg)));
-
-	/*
-	 * Reclaim is set up above to be called from the userland
-	 * return path. But also attempt synchronous reclaim to avoid
-	 * excessive overrun while the task is still inside the
-	 * kernel. If this is successful, the return path will see it
-	 * when it rechecks the overage and simply bail out.
-	 */
-	if (current->memcg_nr_pages_over_high > MEMCG_CHARGE_BATCH &&
-	    !(current->flags & PF_MEMALLOC) &&
-	    gfpflags_allow_blocking(gfp_mask))
-		__mem_cgroup_handle_over_high(gfp_mask);
+	memcg_enforce_high(memcg, batch, gfp_mask);
 	return ret;
 }
 

Signature · 1 line

2.53.0-Meta

Kernel threads never return to userspace, so deferred memory.high reclaim does not run. Reclaim and throttling during a charge can also delay work they do for other tasks.

Treat kernel threads like interrupts. Queue high_work for the first cgroup over memory.high and skip reclaim and throttling in the charging thread.

User workers created by create_io_thread() and vhost_task_create() keep their current behavior. Vhost kernel threads use the new path, even when they borrow the owner's mm.

The worker requests a fixed batch of pages each time. A fast allocator can keep usage well above memory.high. Kernel threads also skip memory.swap.high throttling and its high-event reporting. The memory.max and memory.swap.max checks stay the same.

Suggested-by Tejun Heo <[email protected]>

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

---
 mm/memcontrol.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

mm/memcontrol.c +11 -2

index 20c5dfe1a894..8116fce6ab96 100644
@@ -2705,6 +2705,7 @@ static void memcg_enforce_high(struct mem_cgroup *memcg, unsigned int nr_pages,
 			       gfp_t gfp_mask)
 {
 	bool allow_spinning = gfpflags_allow_spinning(gfp_mask);
+	bool use_worker = !in_task() || (current->flags & PF_KTHREAD);
 
 	/*
 	 * If the hierarchy is above the normal consumption range, schedule

@@ -2723,8 +2724,12 @@ static void memcg_enforce_high(struct mem_cgroup *memcg, unsigned int nr_pages,
 		swap_high = page_counter_read(&memcg->swap) >
 			READ_ONCE(memcg->swap.high);
 
-		/* Don't bother a random interrupted task */
-		if (!in_task()) {
+		/*
+		 * Don't make an unrelated interrupted task handle this charge.
+		 * Kernel threads may be working for other tasks, so let a worker
+		 * reclaim instead.
+		 */
+		if (use_worker) {
 			if (mem_high) {
 				if (allow_spinning)
 					schedule_work(&memcg->high_work);

@@ -2751,6 +2756,10 @@ static void memcg_enforce_high(struct mem_cgroup *memcg, unsigned int nr_pages,
 		}
 	} while ((memcg = parent_mem_cgroup(memcg)));
 
+	/* Interrupts and kernel threads stop here. */
+	if (use_worker)
+		return;
+
 	/*
 	 * Reclaim is set up above to be called from the userland
 	 * return path. But also attempt synchronous reclaim to avoid

Signature · 1 line

2.53.0-Meta

Some charges go to a cgroup outside the current mm's cgroup and its ancestors. The high handler uses current->mm, so it cannot handle these overages. Filesystem notifications and remote page faults can take this path.

Use mm_match_cgroup() before checking the high limits. If it does not match, use high_work instead of recording task overage or doing synchronous reclaim and throttling. Charges to the same cgroup or an ancestor keep their current handling. Tasks without an mm also use the worker.

Like kernel threads, remote chargers skip memory.swap.high throttling and its high-event reporting, and a fast allocator can outrun the worker. The memory.max and memory.swap.max checks stay the same.

Suggested-by Tejun Heo <[email protected]>

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

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

mm/memcontrol.c +4 -2

index 8116fce6ab96..3456960cbe68 100644
@@ -2705,7 +2705,8 @@ static void memcg_enforce_high(struct mem_cgroup *memcg, unsigned int nr_pages,
 			       gfp_t gfp_mask)
 {
 	bool allow_spinning = gfpflags_allow_spinning(gfp_mask);
-	bool use_worker = !in_task() || (current->flags & PF_KTHREAD);
+	bool use_worker = !in_task() || (current->flags & PF_KTHREAD) ||
+		!current->mm || !mm_match_cgroup(current->mm, memcg);
 
 	/*
 	 * If the hierarchy is above the normal consumption range, schedule

@@ -2728,6 +2729,7 @@ static void memcg_enforce_high(struct mem_cgroup *memcg, unsigned int nr_pages,
 		 * Don't make an unrelated interrupted task handle this charge.
 		 * Kernel threads may be working for other tasks, so let a worker
 		 * reclaim instead.
+		 * Remote charges need reclaim from their charged hierarchy.
 		 */
 		if (use_worker) {
 			if (mem_high) {

@@ -2756,7 +2758,7 @@ static void memcg_enforce_high(struct mem_cgroup *memcg, unsigned int nr_pages,
 		}
 	} while ((memcg = parent_mem_cgroup(memcg)));
 
-	/* Interrupts and kernel threads stop here. */
+	/* Interrupts, kernel threads, and remote chargers stop here. */
 	if (use_worker)
 		return;
 

Signature · 1 line

2.53.0-Meta

Kernel threads and remote chargers use high_work when memory.high is exceeded. Their allocations during reclaim can also charge memory, for example when zswap stores a page.

Without a check, the high_work worker can queue itself from its own reclaim. The pending bit is cleared before high_work runs, so each run can queue another run.

Skip memory.high enforcement for charges from task context with PF_MEMALLOC set when they would use high_work. This covers kernel threads and remote charges. The memory is still charged. Interrupt charges keep their current path.

In a VM with incompressible zswap data, the worker queued itself 163 times without this check.

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

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

mm/memcontrol.c +8 -0

index 3456960cbe68..d1e022e6572f 100644
@@ -2708,6 +2708,14 @@ static void memcg_enforce_high(struct mem_cgroup *memcg, unsigned int nr_pages,
 	bool use_worker = !in_task() || (current->flags & PF_KTHREAD) ||
 		!current->mm || !mm_match_cgroup(current->mm, memcg);
 
+	/*
+	 * Reclaim can charge memory, for example when zswap stores a page.
+	 * Don't queue high_work from a reclaiming task, or the
+	 * high_work worker could keep queuing itself.
+	 */
+	if (in_task() && use_worker && (current->flags & PF_MEMALLOC))
+		return;
+
 	/*
 	 * If the hierarchy is above the normal consumption range, schedule
 	 * reclaim on returning to userland.  We can perform reclaim here

Signature · 1 line

2.53.0-Meta

Quoted 7 lines · Shakeel Butt

Kernel threads and remote chargers use high_work when memory.high is exceeded. Their allocations during reclaim can also charge memory, for example when zswap stores a page.

Without a check, the high_work worker can queue itself from its own reclaim. The pending bit is cleared before high_work runs, so each run can queue another run.

What does that mean really? Does this delays the actual run?

Quoted 6 lines

Skip memory.high enforcement for charges from task context with PF_MEMALLOC set when they would use high_work. This covers kernel threads and remote charges. The memory is still charged. Interrupt charges keep their current path.

Quoted 4 lines

In a VM with incompressible zswap data, the worker queued itself 163 times without this check.

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

The patch itself looks OK. I am wondering whether we can unify the check with the existing one which forces the allocation through in main charging path.

Quoted 9 lines · Michal Hocko

Quoted 7 lines · Shakeel Butt

Kernel threads and remote chargers use high_work when memory.high is exceeded. Their allocations during reclaim can also charge memory, for example when zswap stores a page.

Without a check, the high_work worker can queue itself from its own reclaim. The pending bit is cleared before high_work runs, so each run can queue another run.

Quoted 1 line

What does that mean really? Does this delays the actual run?

It will not delay the actual charge request. This mainly means that we keep enqueing the high reclaim work even from high reclaim context and can potentially keep wasting CPU if high reclaim is not very successful.

Quoted 8 lines

Quoted 8 lines

Skip memory.high enforcement for charges from task context with PF_MEMALLOC set when they would use high_work. This covers kernel threads and remote charges. The memory is still charged. Interrupt charges keep their current path.

In a VM with incompressible zswap data, the worker queued itself 163 times without this check.

Quoted 5 lines

Quoted 1 line

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

The patch itself looks OK. I am wondering whether we can unify the check with the existing one which forces the allocation through in main charging path.

The force charge (and ENOMEM) paths bypass the memory.high enforcement altogether.

Only successful charges (done_restock label) goes through memory.high enforcement. So, even if we move this check in try_charge_memcg, it will still be a separate check before calling high enforcement function. I think this check is very high_work specific and better to be in the high enforcement function.

Quoted 10 lines · Shakeel Butt

Quoted 9 lines · Michal Hocko

Quoted 7 lines · Shakeel Butt

Kernel threads and remote chargers use high_work when memory.high is exceeded. Their allocations during reclaim can also charge memory, for example when zswap stores a page.

Without a check, the high_work worker can queue itself from its own reclaim. The pending bit is cleared before high_work runs, so each run can queue another run.

Quoted 5 lines

Quoted 1 line

What does that mean really? Does this delays the actual run?

It will not delay the actual charge request. This mainly means that we keep enqueing the high reclaim work even from high reclaim context and can potentially keep wasting CPU if high reclaim is not very successful.

Hmm, so is there any practical problem if we do that? What exactly do you mean by "keep wasting CPU"?

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Skip memory.high enforcement for charges from task context with PF_MEMALLOC set when they would use high_work. This covers kernel threads and remote charges. The memory is still charged. Interrupt charges keep their current path.

In a VM with incompressible zswap data, the worker queued itself 163 times without this check.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

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

The patch itself looks OK. I am wondering whether we can unify the check with the existing one which forces the allocation through in main charging path.

The force charge (and ENOMEM) paths bypass the memory.high enforcement altogether.

Exactly. Isn't that exactl what we want to achieve here as well?

Quoted 5 lines

Only successful charges (done_restock label) goes through memory.high enforcement. So, even if we move this check in try_charge_memcg, it will still be a separate check before calling high enforcement function. I think this check is very high_work specific and better to be in the high enforcement function.

I meant something like the following

mm/memcontrol.c +11 -18

index 856a7d07586c..832b40ee5b6f 100644
@@ -2653,7 +2653,6 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	bool passed_oom = false;
 	unsigned int reclaim_options;
 	bool drained = false;
-	bool raised_max_event = false;
 	unsigned long pflags;
 	bool allow_spinning = gfpflags_allow_spinning(gfp_mask);
 

@@ -2661,6 +2660,15 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	if (consume_stock(memcg, nr_pages))
 		return 0;
 
+	/*
+	 * Prevent unbounded recursion when reclaim operations need to
+	 * allocate memory. This might exceed the limits temporarily,
+	 * but we prefer facilitating memory reclaim and getting back
+	 * under the limit over triggering OOM kills in these cases.
+	 */
+	if (unlikely(current->flags & PF_MEMALLOC))
+		goto force;
+
 	if (!allow_spinning)
 		/* Avoid the refill and flush of the older stock */
 		batch = nr_pages;

@@ -2683,15 +2691,6 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 		goto retry;
 	}
 
-	/*
-	 * Prevent unbounded recursion when reclaim operations need to
-	 * allocate memory. This might exceed the limits temporarily,
-	 * but we prefer facilitating memory reclaim and getting back
-	 * under the limit over triggering OOM kills in these cases.
-	 */
-	if (unlikely(current->flags & PF_MEMALLOC))
-		goto force;
-
 	if (unlikely(task_in_memcg_oom(current)))
 		goto nomem;
 

@@ -2712,7 +2711,6 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 		goto nomem;
 
 	__memcg_memory_event(mem_over_limit, MEMCG_MAX, allow_spinning);
-	raised_max_event = true;
 
 	psi_memstall_enter(&pflags);
 	nr_reclaimed = try_to_free_mem_cgroup_pages(mem_over_limit, nr_pages,

@@ -2773,13 +2771,6 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	if (!(gfp_mask & (__GFP_NOFAIL | __GFP_HIGH)))
 		return -ENOMEM;
 force:
-	/*
-	 * If the allocation has to be enforced, don't forget to raise
-	 * a MEMCG_MAX event.
-	 */
-	if (!raised_max_event)
-		__memcg_memory_event(mem_over_limit, MEMCG_MAX, allow_spinning);
-
 	/*
 	 * The allocation either can't fail or will lead to more memory
 	 * being freed very soon.  Allow memory usage go over the limit

@@ -2789,6 +2780,8 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	if (do_memsw_account())
 		page_counter_charge(&memcg->memsw, nr_pages);
 
+	if (mem_cgroup_margin(memcg))
+		__memcg_memory_event(memcg, MEMCG_MAX, allow_spinning);
 	return 0;
 
 done_restock:

Signature · 3 lines

-- 
Michal Hocko
SUSE Labs

Quoted 17 lines · Michal Hocko

Quoted 15 lines · Shakeel Butt

Quoted 10 lines · Michal Hocko

Quoted 7 lines · Shakeel Butt

Kernel threads and remote chargers use high_work when memory.high is exceeded. Their allocations during reclaim can also charge memory, for example when zswap stores a page.

Without a check, the high_work worker can queue itself from its own reclaim. The pending bit is cleared before high_work runs, so each run can queue another run.

What does that mean really? Does this delays the actual run?

It will not delay the actual charge request. This mainly means that we keep enqueing the high reclaim work even from high reclaim context and can potentially keep wasting CPU if high reclaim is not very successful.

Quoted 2 lines

Hmm, so is there any practical problem if we do that? What exactly do you mean by "keep wasting CPU"?

More specifically if high_work kworker keeps enqueing high_work, most probably the memcg does not have reclaimable memory and keep retrying it wastes CPU.

Quoted 25 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Skip memory.high enforcement for charges from task context with PF_MEMALLOC set when they would use high_work. This covers kernel threads and remote charges. The memory is still charged. Interrupt charges keep their current path.

In a VM with incompressible zswap data, the worker queued itself 163 times without this check.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

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

The patch itself looks OK. I am wondering whether we can unify the check with the existing one which forces the allocation through in main charging path.

The force charge (and ENOMEM) paths bypass the memory.high enforcement altogether.

Exactly. Isn't that exactl what we want to achieve here as well?

Quoted 5 lines

Only successful charges (done_restock label) goes through memory.high enforcement. So, even if we move this check in try_charge_memcg, it will still be a separate check before calling high enforcement function. I think this check is very high_work specific and better to be in the high enforcement function.

Quoted 1 line

I meant something like the following

Oh ok this makes sense. Do you want to send a patch or should I add this in this series replacing the 4th patch?

Quoted 18 lines · Shakeel Butt

Quoted 17 lines · Michal Hocko

Quoted 15 lines · Shakeel Butt

Quoted 10 lines · Michal Hocko

Quoted 7 lines · Shakeel Butt

Kernel threads and remote chargers use high_work when memory.high is exceeded. Their allocations during reclaim can also charge memory, for example when zswap stores a page.

Without a check, the high_work worker can queue itself from its own reclaim. The pending bit is cleared before high_work runs, so each run can queue another run.

What does that mean really? Does this delays the actual run?

It will not delay the actual charge request. This mainly means that we keep enqueing the high reclaim work even from high reclaim context and can potentially keep wasting CPU if high reclaim is not very successful.

Quoted 5 lines

Quoted 2 lines

Hmm, so is there any practical problem if we do that? What exactly do you mean by "keep wasting CPU"?

More specifically if high_work kworker keeps enqueing high_work, most probably the memcg does not have reclaimable memory and keep retrying it wastes CPU.

Yes but this is not specific to PF_MEMALLOC context, or do I miss something?

Quoted 26 lines

Quoted 26 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Skip memory.high enforcement for charges from task context with PF_MEMALLOC set when they would use high_work. This covers kernel threads and remote charges. The memory is still charged. Interrupt charges keep their current path.

In a VM with incompressible zswap data, the worker queued itself 163 times without this check.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

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

The patch itself looks OK. I am wondering whether we can unify the check with the existing one which forces the allocation through in main charging path.

The force charge (and ENOMEM) paths bypass the memory.high enforcement altogether.

Exactly. Isn't that exactl what we want to achieve here as well?

Quoted 5 lines

Only successful charges (done_restock label) goes through memory.high enforcement. So, even if we move this check in try_charge_memcg, it will still be a separate check before calling high enforcement function. I think this check is very high_work specific and better to be in the high enforcement function.

Quoted 4 lines

Quoted 1 line

I meant something like the following

Oh ok this makes sense. Do you want to send a patch or should I add this in this series replacing the 4th patch?

Feel free to take it. I haven't even tried to compile this. Just wanted to show what I have in mind.

mm/memcontrol.c +5 -2

-	/* Interrupts, kernel threads, and remote chargers stop here. */
-	if (use_worker)
+	/*
+	 * Use a worker for interrupts, kernel threads, and tasks whose mm
+	 * does not match the charged hierarchy.
+	 */
+	if (use_high_work) {

mm/memcontrol.c +10 -5

+		/*
+		 * Count the full batch so reclaim work and delay penalties scale
+		 * with the pages charged by this task.
+		 */
+		current->memcg_nr_pages_over_high += nr_pages;
+		set_notify_resume(current);
+	}
 
 	/*
-	 * Reclaim is set up above to be called from the userland
-	 * return path. But also attempt synchronous reclaim to avoid
-	 * excessive overrun while the task is still inside the
-	 * kernel. If this is successful, the return path will see it
-	 * when it rechecks the overage and simply bail out.
+	 * Earlier charges may have left high overage pending even if this charge
+	 * found no new overage. Handle it now if this charge can block, to limit
+	 * overrun while the task remains in the kernel.
 	 */
 	if (current->memcg_nr_pages_over_high > MEMCG_CHARGE_BATCH &&
 	    !(current->flags & PF_MEMALLOC) &&

Signature · 1 line

2.53.0-Meta

On Sat 10-10-26 14:09:57, Shakeel Butt wrote:
[...]

Quoted 12 lines

mm/memcontrol.c +10 -2

-	/* Interrupts, kernel threads, and remote chargers stop here. */
-	if (use_worker)
+	/*
+	 * Use a worker for interrupts, kernel threads, and tasks whose mm
+	 * does not match the charged hierarchy.
+	 */
+	if (use_high_work) {
+		if (!iter)
+			return;
+		if (allow_spinning)
+			schedule_work(&iter->high_work);
+		else

mm/memcontrol.c void __mem_cgroup_handle_over_high(gfp_t gfp_mask) · line 2752

Quoted 5 lines

mm/memcontrol.c +4 -0

+			irq_work_queue(&iter->high_irq_work);
 		return;
+	}
+
+	if (iter) {

Would it be much simpler to bail out on iter==NULL (i.e. nothing over high limit) right after the loop?

Quoted 22 lines

mm/memcontrol.c +10 -5

+		/*
+		 * Count the full batch so reclaim work and delay penalties scale
+		 * with the pages charged by this task.
+		 */
+		current->memcg_nr_pages_over_high += nr_pages;
+		set_notify_resume(current);
+	}
 
 	/*
-	 * Reclaim is set up above to be called from the userland
-	 * return path. But also attempt synchronous reclaim to avoid
-	 * excessive overrun while the task is still inside the
-	 * kernel. If this is successful, the return path will see it
-	 * when it rechecks the overage and simply bail out.
+	 * Earlier charges may have left high overage pending even if this charge
+	 * found no new overage. Handle it now if this charge can block, to limit
+	 * overrun while the task remains in the kernel.
 	 */
 	if (current->memcg_nr_pages_over_high > MEMCG_CHARGE_BATCH &&
 	    !(current->flags & PF_MEMALLOC) &&

Signature · 1 line

2.53.0-Meta

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 14 lines · Michal Hocko

On Sat 10-10-26 14:09:57, Shakeel Butt wrote:
[...]

Quoted 12 lines

mm/memcontrol.c +10 -2

-	/* Interrupts, kernel threads, and remote chargers stop here. */
-	if (use_worker)
+	/*
+	 * Use a worker for interrupts, kernel threads, and tasks whose mm
+	 * does not match the charged hierarchy.
+	 */
+	if (use_high_work) {
+		if (!iter)
+			return;
+		if (allow_spinning)
+			schedule_work(&iter->high_work);
+		else

Quoted 9 lines

mm/memcontrol.c void __mem_cgroup_handle_over_high(gfp_t gfp_mask) · line 2752

Quoted 5 lines

mm/memcontrol.c +4 -0

+			irq_work_queue(&iter->high_irq_work);
 		return;
+	}
+
+	if (iter) {

Would it be much simpler to bail out on iter==NULL (i.e. nothing over high limit) right after the loop?

I want this patch to not change any behavior. If we return here when iter is NULL, we will skip a potential call to __mem_cgroup_handle_over_high() when current->memcg_nr_pages_over_high > MEMCG_CHARGE_BATCH. Though I agree that is very subtle and most probably unexpected. If we decide to remove it, we should do it in a separate patch.