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: <67aacbba-7049-bee8-0ad4-ab4db588c841@gmail.com>
Date:   Mon, 30 Nov 2020 22:44:11 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Alex Shi <alex.shi@...ux.alibaba.com>,
        Andrew Morton <akpm@...ux-foundation.org>
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,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec

29.11.2020 07:43, Alex Shi пишет:
> 
> 
> 在 2020/11/28 下午12:02, Andrew Morton 写道:
>> 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?
> 
> Uh, Consider there are no problem for long time, it could be saved.
> 
>>
>>> --- 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?
> 
> If !memcg, the root_mem_cgroup will still lead the lruvec to a pgdat
> same as parameter.
> 
>>
>>>  	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;
>> 	}
>>
> 
> Right. remove 'goto' is better for understander.
> 
> So, is the following patch ok?
> 
> From 225f29e03b40a7cbaeb4e3bb76f8efbcd7d648a2 Mon Sep 17 00:00:00 2001
> From: Alex Shi <alex.shi@...ux.alibaba.com>
> Date: Wed, 25 Nov 2020 14:06:33 +0800
> Subject: [PATCH v2] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec
> 
> Sometime, we use NULL memcg in mem_cgroup_lruvec(memcg, pgdat)
> so we could get out early in the situation to avoid useless checking.
> 
> Polished as Andrew Morton's suggestion.
> 
> Signed-off-by: Alex Shi <alex.shi@...ux.alibaba.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Johannes Weiner <hannes@...xchg.org>
> Cc: Shakeel Butt <shakeelb@...gle.com>
> Cc: Roman Gushchin <guro@...com>
> Cc: Lorenzo Stoakes <lstoakes@...il.com>
> Cc: Stephen Rothwell <sfr@...b.auug.org.au>
> Cc: Alexander Duyck <alexander.h.duyck@...ux.intel.com>
> Cc: Yafang Shao <laoar.shao@...il.com>
> Cc: Wei Yang <richard.weiyang@...il.com>
> Cc: linux-kernel@...r.kernel.org
> ---
>  include/linux/memcontrol.h | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 3e6a1df3bdb9..4ff2ffe2b73d 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -610,20 +610,17 @@ mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid)
>  static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
>  					       struct pglist_data *pgdat)
>  {
> -	struct mem_cgroup_per_node *mz;
>  	struct lruvec *lruvec;
>  
> -	if (mem_cgroup_disabled()) {
> +	if (mem_cgroup_disabled() || !memcg) {
>  		lruvec = &pgdat->__lruvec;
> -		goto out;
> -	}
> +	} else {
> +		struct mem_cgroup_per_node *mz;
>  
> -	if (!memcg)
> -		memcg = root_mem_cgroup;
> +		mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id);
> +		lruvec = &mz->lruvec;
> +	}
>  
> -	mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id);
> -	lruvec = &mz->lruvec;
> -out:
>  	/*
>  	 * Since a node can be onlined after the mem_cgroup was created,
>  	 * we have to be prepared to initialize lruvec->pgdat here;
> 

Hi,

This patch causes a hard lock on one of my ARM32 devices using today's
linux-next, please fix.