linux-kernel - Re: [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20201127200215.dc96a839cdd816361e7093e6@linux-foundation.org>
Date:   Fri, 27 Nov 2020 20:02:15 -0800
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Alex Shi <alex.shi@...ux.alibaba.com>
Cc:     Johannes Weiner <hannes@...xchg.org>,
        Shakeel Butt <shakeelb@...gle.com>,
        Roman Gushchin <guro@...com>,
        Lorenzo Stoakes <lstoakes@...il.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Alexander Duyck <alexander.h.duyck@...ux.intel.com>,
        Yafang Shao <laoar.shao@...il.com>,
        Wei Yang <richard.weiyang@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/memcg: bail out early when !memcg in
 mem_cgroup_lruvec

On Fri, 27 Nov 2020 11:08:35 +0800 Alex Shi <alex.shi@...ux.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;
	}