Re: linux-next: runtime warning in Linus' tree

[Roman Gushchin <guro@fb.com>]

On Thu, Aug 13, 2020 at 11:20:33AM -0400, Johannes Weiner wrote:
> On Thu, Aug 13, 2020 at 04:46:54PM +1000, Stephen Rothwell wrote:
> > [    0.055220][    T0] WARNING: CPU: 0 PID: 0 at mm/memcontrol.c:5220 mem_cgroup_css_alloc+0x350/0x904
> 
> > [The line numbers in the final linux next are 5226 and 5141 due to
> > later patches.]
> > 
> > Introduced (or exposed) by commit
> > 
> >   3e38e0aaca9e ("mm: memcg: charge memcg percpu memory to the parent cgroup")
> > 
> > This commit actually adds the WARN_ON, so it either adds the bug that
> > sets it off, or the bug already existed.
> > 
> > Unfotunately, the version of this patch in linux-next up tuntil today
> > is different.  :-(
> 
> Sorry, I made a last-minute request to include these checks in that
> patch to make the code a bit more robust, but they trigger a false
> positive here. Let's remove them.
> 
> ---
> 
> From de8ea7c96c056c3cbe7b93995029986a158fb9cd Mon Sep 17 00:00:00 2001
> From: Johannes Weiner <hannes@cmpxchg.org>
> Date: Thu, 13 Aug 2020 10:40:54 -0400
> Subject: [PATCH] mm: memcontrol: fix warning when allocating the root cgroup
> 
> Commit 3e38e0aaca9e ("mm: memcg: charge memcg percpu memory to the
> parent cgroup") adds memory tracking to the memcg kernel structures
> themselves to make cgroups liable for the memory they are consuming
> through the allocation of child groups (which can be significant).
> 
> This code is a bit awkward as it's spread out through several
> functions: The outermost function does memalloc_use_memcg(parent) to
> set up current->active_memcg, which designates which cgroup to charge,
> and the inner functions pass GFP_ACCOUNT to request charging for
> specific allocations. To make sure this dependency is satisfied at all
> times - to make sure we don't randomly charge whoever is calling the
> functions - the inner functions warn on !current->active_memcg.
> 
> However, this triggers a false warning when the root memcg itself is
> allocated. No parent exists in this case, and so current->active_memcg
> is rightfully NULL. It's a false positive, not indicative of a bug.
> 
> Delete the warnings for now, we can revisit this later.
> 
> Fixes: 3e38e0aaca9e ("mm: memcg: charge memcg percpu memory to the parent cgroup")
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Roman Gushchin <guro@fb.com>

Thanks!


> ---
>  mm/memcontrol.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d59fd9af6e63..9d87082e64aa 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5137,9 +5137,6 @@ static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
>  	if (!pn)
>  		return 1;
>  
> -	/* We charge the parent cgroup, never the current task */
> -	WARN_ON_ONCE(!current->active_memcg);
> -
>  	pn->lruvec_stat_local = alloc_percpu_gfp(struct lruvec_stat,
>  						 GFP_KERNEL_ACCOUNT);
>  	if (!pn->lruvec_stat_local) {
> @@ -5222,9 +5219,6 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
>  		goto fail;
>  	}
>  
> -	/* We charge the parent cgroup, never the current task */
> -	WARN_ON_ONCE(!current->active_memcg);
> -
>  	memcg->vmstats_local = alloc_percpu_gfp(struct memcg_vmstats_percpu,
>  						GFP_KERNEL_ACCOUNT);
>  	if (!memcg->vmstats_local)
> -- 
> 2.28.0
>