[Devel] [PATCH rh7 v2 09/10] tcache: Use ni->lock only for inserting and erasing from rbtree.

[Kirill Tkhai]

On 16.08.2017 17:12, Andrey Ryabinin wrote:
> 
> 
> On 08/16/2017 02:53 PM, Kirill Tkhai wrote:
>> This patch completes splitting of ni->lock into ni->lock and pni->lock.
>> Now, global ni->lock is used for inserting in tcache_nodeinfo::reclaim_tree,
>> which happen just on every 1024 inserting or erasing of pages.
>> For other LRU operations is used pni->lock, which is per-filesystem
>> (i.e., per-container), and does not affect other containers.
>>
>> Also, lock order is changed:
>>
>> 	spin_lock(&pni->lock);
>> 	spin_lock(&ni->lock);
>>
>> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
>> ---
>>  mm/tcache.c |   27 ++++++++++++++-------------
>>  1 file changed, 14 insertions(+), 13 deletions(-)
>>
> 
>>  static int tcache_create_pool(void)
>> @@ -1064,7 +1064,6 @@ tcache_lru_isolate(int nid, struct page **pages, int nr_to_isolate)
>>  	}
>>  	rcu_read_unlock();
>>  
>> -	spin_lock_irq(&ni->lock);
>>  	spin_lock(&pni->lock);
> 
> spin_lock_irq(&pni->lock) 
> 
>>  	nr_isolated = __tcache_lru_isolate(pni, pages, nr_to_isolate);
>>  
>> @@ -1073,17 +1072,19 @@ tcache_lru_isolate(int nid, struct page **pages, int nr_to_isolate)
>>  
>>  	atomic_long_sub(nr_isolated, &ni->nr_pages);
>>  
>> -	if (!RB_EMPTY_NODE(rbn)) {
>> -		rb_erase(rbn, &ni->reclaim_tree);
>> -		RB_CLEAR_NODE(rbn);
>> +	if (!RB_EMPTY_NODE(rbn) || !list_empty(&pni->lru)) {
>> +		spin_lock(&ni->lock);
> 
> 
> 
> 
>> +		if (!RB_EMPTY_NODE(rbn))
>> +			rb_erase(rbn, &ni->reclaim_tree);
>> +		if (!list_empty(&pni->lru))
>> +			__tcache_insert_reclaim_node(ni, pni);
>> +		else
>> +			RB_CLEAR_NODE(rbn);
> 
> Didn't get this part. Shouldn't we have something like this?
> 
> 		if (!RB_EMPTY_NODE(rbn)) {
> 			rb_erase(rbn, &ni->reclaim_tree);
> 			RB_CLEAR_NODE(rbn);
> 		}
> 		if (!list_empty(&pni->lru))
> 			__tcache_insert_reclaim_node(ni, pni);

RB_CLEAR_NODE sets __rb_parent_color in NULL, and __tcache_insert_reclaim_node()
sets it in !NULL again.

It's just unnecessary action.

> 
> 
> 
>> +		update_ni_rb_first(ni);
>> +		spin_unlock(&ni->lock);
>>  	}
>> -	if (!list_empty(&pni->lru))
>> -		__tcache_insert_reclaim_node(ni, pni);
>> -	update_ni_rb_first(ni);
>> -
>>  unlock:
>>  	spin_unlock(&pni->lock);
> 
> spin_unlock_irq(&pni->lock) 
> 
>> -	spin_unlock_irq(&ni->lock);
>>  	tcache_put_pool(pni->pool);
>>  out:
>>  	return nr_isolated;
>>