Re: [PATCH for-7.5 3/5] memcg: use high_work for remote charges

[Michal Hocko]

On Sat 10-10-26 14:09:55, Shakeel Butt wrote:
> 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.

Hmm, you made me look at other places in the charge path where we are
assuming current->memcg == memcg. We have reclaim recursion and oom
checks. The former makes sense, for the latter it is likely not
harmful.

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

Makes sense.

> Suggested-by: Tejun Heo <tj@xxxxxxxxxx>
> Signed-off-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>

Acked-by: Michal Hocko <mhocko@xxxxxxxx>
Thanks!

> ---
>  mm/memcontrol.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8116fce6ab96..3456960cbe68 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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;
>  
> -- 
> 2.53.0-Meta

-- 
Michal Hocko
SUSE Labs