Re: [PATCH for-7.5 4/5] memcg: avoid queuing high_work from reclaim

[Michal Hocko <mhocko@suse.com>]

On Wed 14-10-26 09:06:00, Shakeel Butt wrote:
> On Wed, Oct 14, 2026 at 01:39:06PM +0200, Michal Hocko wrote:
> > On Sat 10-10-26 14:09:56, Shakeel Butt wrote:
> > > 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.

Hmm, so is there any practical problem if we do that? What exactly do you
mean by "keep wasting CPU"?
 
> > > 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 <shakeel.butt@linux.dev>
> > 
> > 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?

> 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

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 856a7d07586c..832b40ee5b6f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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:

-- 
Michal Hocko
SUSE Labs