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

[Shakeel Butt]

On Wed, Oct 14, 2026 at 06:59:30PM +0200, Michal Hocko wrote:
> 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"?

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.

>  
> > > > 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@xxxxxxxxx>
> > > 
> > > 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

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

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