Message-ID: <2eb276f1-0d81-f207-4cf9-e6586c5e18c8@bytedance.com>
Date:   Wed, 22 Feb 2023 15:32:00 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.7.2
Subject: Re: [PATCH 2/5] mm: vmscan: make memcg slab shrink lockless
To:     Kirill Tkhai <tkhai@ya.ru>
Cc:     sultan@kerneltoast.com, dave@stgolabs.net,
        penguin-kernel@I-love.SAKURA.ne.jp, paulmck@kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Shakeel Butt <shakeelb@google.com>,
        Michal Hocko <mhocko@kernel.org>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Muchun Song <muchun.song@linux.dev>,
        David Hildenbrand <david@redhat.com>,
        Yang Shi <shy828301@gmail.com>
References: <20230220091637.64865-1-zhengqi.arch@bytedance.com>
 <20230220091637.64865-3-zhengqi.arch@bytedance.com>
 <8733cb3c-7a6a-33c4-b84e-4fb981dae765@ya.ru>
Content-Language: en-US
From:   Qi Zheng <zhengqi.arch@bytedance.com>
In-Reply-To: <8733cb3c-7a6a-33c4-b84e-4fb981dae765@ya.ru>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2023/2/22 05:28, Kirill Tkhai wrote:
> On 20.02.2023 12:16, Qi Zheng wrote:
<...>
>>   
>>   void reparent_shrinker_deferred(struct mem_cgroup *memcg)
>>   {
>> -	int i, nid;
>> +	int i, nid, srcu_idx;
>>   	long nr;
>>   	struct mem_cgroup *parent;
>>   	struct shrinker_info *child_info, *parent_info;
>> @@ -429,16 +443,16 @@ void reparent_shrinker_deferred(struct mem_cgroup *memcg)
>>   		parent = root_mem_cgroup;
>>   
>>   	/* Prevent from concurrent shrinker_info expand */
>> -	down_read(&shrinker_rwsem);
>> +	srcu_idx = srcu_read_lock(&shrinker_srcu);
> 
> Don't we still have to be protected against parallel expand_one_shrinker_info()?
> 
> It looks like parent->nodeinfo[nid]->shrinker_info pointer may be changed in expand*
> right after we've dereferenced it here.

Hi Kirill,

Oh, indeed. We may wrongly reparent the child's nr_deferred to the old
parent's nr_deferred (it is about to be freed by call_srcu).

The reparent_shrinker_deferred() will only be called on the offline
path (not a hotspot path), so we may be able to use shrinker_mutex
introduced later for protection. What do you think?

Thanks,
Qi

> 
>>   	for_each_node(nid) {
>> -		child_info = shrinker_info_protected(memcg, nid);
>> -		parent_info = shrinker_info_protected(parent, nid);
>> +		child_info = shrinker_info_srcu(memcg, nid);
>> +		parent_info = shrinker_info_srcu(parent, nid);
>>   		for (i = 0; i < shrinker_nr_max; i++) {
>>   			nr = atomic_long_read(&child_info->nr_deferred[i]);
>>   			atomic_long_add(nr, &parent_info->nr_deferred[i]);
>>   		}
>>   	}
>> -	up_read(&shrinker_rwsem);
>> +	srcu_read_unlock(&shrinker_srcu, srcu_idx);
>>   }
>>   
>>   static bool cgroup_reclaim(struct scan_control *sc)
>> @@ -891,15 +905,14 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
>>   {
>>   	struct shrinker_info *info;
>>   	unsigned long ret, freed = 0;
>> +	int srcu_idx;
>>   	int i;
>>   
>>   	if (!mem_cgroup_online(memcg))
>>   		return 0;
>>   
>> -	if (!down_read_trylock(&shrinker_rwsem))
>> -		return 0;
>> -
>> -	info = shrinker_info_protected(memcg, nid);
>> +	srcu_idx = srcu_read_lock(&shrinker_srcu);
>> +	info = shrinker_info_srcu(memcg, nid);
>>   	if (unlikely(!info))
>>   		goto unlock;
>>   
>> @@ -949,14 +962,9 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
>>   				set_shrinker_bit(memcg, nid, i);
>>   		}
>>   		freed += ret;
>> -
>> -		if (rwsem_is_contended(&shrinker_rwsem)) {
>> -			freed = freed ? : 1;
>> -			break;
>> -		}
>>   	}
>>   unlock:
>> -	up_read(&shrinker_rwsem);
>> +	srcu_read_unlock(&shrinker_srcu, srcu_idx);
>>   	return freed;
>>   }
>>   #else /* CONFIG_MEMCG */
> 

-- 
Thanks,
Qi