Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 27 Nov 2020 20:02:15 -0800
From:   Andrew Morton <akpm@linux-foundation.org>
To:     Alex Shi <alex.shi@linux.alibaba.com>
Cc:     Johannes Weiner <hannes@cmpxchg.org>,
        Shakeel Butt <shakeelb@google.com>,
        Roman Gushchin <guro@fb.com>,
        Lorenzo Stoakes <lstoakes@gmail.com>,
        Stephen Rothwell <sfr@canb.auug.org.au>,
        Alexander Duyck <alexander.h.duyck@linux.intel.com>,
        Yafang Shao <laoar.shao@gmail.com>,
        Wei Yang <richard.weiyang@gmail.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/memcg: bail out early when !memcg in
 mem_cgroup_lruvec
Message-Id: <20201127200215.dc96a839cdd816361e7093e6@linux-foundation.org>
In-Reply-To: <1606446515-36069-1-git-send-email-alex.shi@linux.alibaba.com>
References: <1606446515-36069-1-git-send-email-alex.shi@linux.alibaba.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 27 Nov 2020 11:08:35 +0800 Alex Shi <alex.shi@linux.alibaba.com> wrote:

> Sometime, we use NULL memcg in mem_cgroup_lruvec(memcg, pgdat)
> so we could get out early in the situation to avoid useless checking.
> 
> Also warning if both parameter are NULL.

Why do you think a warning is needed here?

> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -613,14 +613,13 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
>  	struct mem_cgroup_per_node *mz;
>  	struct lruvec *lruvec;
>  
> -	if (mem_cgroup_disabled()) {
> +	VM_WARN_ON_ONCE(!memcg && !pgdat);
> +
> +	if (mem_cgroup_disabled() || !memcg) {
>  		lruvec = &pgdat->__lruvec;
>  		goto out;
>  	}
>  
> -	if (!memcg)
> -		memcg = root_mem_cgroup;
> -

This change isn't obviously equivalent, is it?

>  	mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id);
>  	lruvec = &mz->lruvec;
>  out:

And the resulting code is awkward:

	if (mem_cgroup_disabled() || !memcg) {
		lruvec = &pgdat->__lruvec;
		goto out;
	}

	mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id);
	lruvec = &mz->lruvec;
out:


could be

	if (mem_cgroup_disabled() || !memcg) {
		lruvec = &pgdat->__lruvec;
	} else {
		mem_cgroup_per_node mz;

		mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id);
		lruvec = &mz->lruvec;
	}