Re: [PATCH] mm/list_lru: replace spinlock with RCU in __list_lru_count_one

[Michal Hocko <mhocko@kernel.org>]

[CC Dave]

On Tue 27-03-18 15:59:04, Li RongQing wrote:
> when reclaim memory, shink_slab will take lots of time even if
> no memory is reclaimed, since list_lru_count_one called by it
> needs to take a spinlock
>
> try to optimize it by replacing spinlock with RCU in
> __list_lru_count_one

Isn't the RCU overkill here? Why cannot we simply do an optimistic
lockless check for nr_items? It would be racy but does it actually
matter? We should be able to tolerate occasional 0 to non-zero and vice
versa transitions AFAICS.

> 
>     $dd if=aaa  of=bbb  bs=1k count=3886080
>     $rm -f bbb
>     $time echo 100000000 >/cgroup/memory/test/memory.limit_in_bytes
> 
> Before: 0m0.415s ===> after: 0m0.395s
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  include/linux/list_lru.h |  2 ++
>  mm/list_lru.c            | 69 ++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
> index bb8129a3474d..ae472538038e 100644
> --- a/include/linux/list_lru.h
> +++ b/include/linux/list_lru.h
> @@ -29,6 +29,7 @@ struct list_lru_one {
>  	struct list_head	list;
>  	/* may become negative during memcg reparenting */
>  	long			nr_items;
> +	struct rcu_head		rcu;
>  };
>  
>  struct list_lru_memcg {
> @@ -46,6 +47,7 @@ struct list_lru_node {
>  	struct list_lru_memcg	*memcg_lrus;
>  #endif
>  	long nr_items;
> +	struct rcu_head		rcu;
>  } ____cacheline_aligned_in_smp;
>  
>  struct list_lru {
> diff --git a/mm/list_lru.c b/mm/list_lru.c
> index fd41e969ede5..4c58ed861729 100644
> --- a/mm/list_lru.c
> +++ b/mm/list_lru.c
> @@ -52,13 +52,13 @@ static inline bool list_lru_memcg_aware(struct list_lru *lru)
>  static inline struct list_lru_one *
>  list_lru_from_memcg_idx(struct list_lru_node *nlru, int idx)
>  {
> -	/*
> -	 * The lock protects the array of per cgroup lists from relocation
> -	 * (see memcg_update_list_lru_node).
> -	 */
> -	lockdep_assert_held(&nlru->lock);
> -	if (nlru->memcg_lrus && idx >= 0)
> -		return nlru->memcg_lrus->lru[idx];
> +	struct list_lru_memcg *tmp;
> +
> +	WARN_ON_ONCE(!rcu_read_lock_held());
> +
> +	tmp = rcu_dereference(nlru->memcg_lrus);
> +	if (tmp && idx >= 0)
> +		return rcu_dereference(tmp->lru[idx]);
>  
>  	return &nlru->lru;
>  }
> @@ -113,14 +113,17 @@ bool list_lru_add(struct list_lru *lru, struct list_head *item)
>  	struct list_lru_one *l;
>  
>  	spin_lock(&nlru->lock);
> +	rcu_read_lock();
>  	if (list_empty(item)) {
>  		l = list_lru_from_kmem(nlru, item);
>  		list_add_tail(item, &l->list);
>  		l->nr_items++;
>  		nlru->nr_items++;
> +		rcu_read_unlock();
>  		spin_unlock(&nlru->lock);
>  		return true;
>  	}
> +	rcu_read_unlock();
>  	spin_unlock(&nlru->lock);
>  	return false;
>  }
> @@ -133,14 +136,17 @@ bool list_lru_del(struct list_lru *lru, struct list_head *item)
>  	struct list_lru_one *l;
>  
>  	spin_lock(&nlru->lock);
> +	rcu_read_lock();
>  	if (!list_empty(item)) {
>  		l = list_lru_from_kmem(nlru, item);
>  		list_del_init(item);
>  		l->nr_items--;
>  		nlru->nr_items--;
> +		rcu_read_unlock();
>  		spin_unlock(&nlru->lock);
>  		return true;
>  	}
> +	rcu_read_unlock();
>  	spin_unlock(&nlru->lock);
>  	return false;
>  }
> @@ -166,12 +172,13 @@ static unsigned long __list_lru_count_one(struct list_lru *lru,
>  {
>  	struct list_lru_node *nlru = &lru->node[nid];
>  	struct list_lru_one *l;
> -	unsigned long count;
> +	unsigned long count = 0;
>  
> -	spin_lock(&nlru->lock);
> +	rcu_read_lock();
>  	l = list_lru_from_memcg_idx(nlru, memcg_idx);
> -	count = l->nr_items;
> -	spin_unlock(&nlru->lock);
> +	if (l)
> +		count = l->nr_items;
> +	rcu_read_unlock();
>  
>  	return count;
>  }
> @@ -204,6 +211,7 @@ __list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
>  	unsigned long isolated = 0;
>  
>  	spin_lock(&nlru->lock);
> +	rcu_read_lock();
>  	l = list_lru_from_memcg_idx(nlru, memcg_idx);
>  restart:
>  	list_for_each_safe(item, n, &l->list) {
> @@ -250,6 +258,7 @@ __list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
>  		}
>  	}
>  
> +	rcu_read_unlock();
>  	spin_unlock(&nlru->lock);
>  	return isolated;
>  }
> @@ -296,9 +305,14 @@ static void __memcg_destroy_list_lru_node(struct list_lru_memcg *memcg_lrus,
>  					  int begin, int end)
>  {
>  	int i;
> +	struct list_lru_one *tmp;
>  
> -	for (i = begin; i < end; i++)
> -		kfree(memcg_lrus->lru[i]);
> +	for (i = begin; i < end; i++) {
> +		tmp = memcg_lrus->lru[i];
> +		rcu_assign_pointer(memcg_lrus->lru[i], NULL);
> +		if (tmp)
> +			kfree_rcu(tmp, rcu);
> +	}
>  }
>  
>  static int __memcg_init_list_lru_node(struct list_lru_memcg *memcg_lrus,
> @@ -314,7 +328,7 @@ static int __memcg_init_list_lru_node(struct list_lru_memcg *memcg_lrus,
>  			goto fail;
>  
>  		init_one_lru(l);
> -		memcg_lrus->lru[i] = l;
> +		rcu_assign_pointer(memcg_lrus->lru[i], l);
>  	}
>  	return 0;
>  fail:
> @@ -325,25 +339,37 @@ static int __memcg_init_list_lru_node(struct list_lru_memcg *memcg_lrus,
>  static int memcg_init_list_lru_node(struct list_lru_node *nlru)
>  {
>  	int size = memcg_nr_cache_ids;
> +	struct list_lru_memcg *tmp;
>  
> -	nlru->memcg_lrus = kvmalloc(size * sizeof(void *), GFP_KERNEL);
> -	if (!nlru->memcg_lrus)
> +	tmp = kvmalloc(size * sizeof(void *), GFP_KERNEL);
> +	if (!tmp)
>  		return -ENOMEM;
>  
> -	if (__memcg_init_list_lru_node(nlru->memcg_lrus, 0, size)) {
> -		kvfree(nlru->memcg_lrus);
> +	if (__memcg_init_list_lru_node(tmp, 0, size)) {
> +		kvfree(tmp);
>  		return -ENOMEM;
>  	}
>  
> +	rcu_assign_pointer(nlru->memcg_lrus, tmp);
> +
>  	return 0;
>  }
>  
> -static void memcg_destroy_list_lru_node(struct list_lru_node *nlru)
> +static void memcg_destroy_list_lru_node_rcu(struct rcu_head *rcu)
>  {
> +	struct list_lru_node *nlru;
> +
> +	nlru = container_of(rcu, struct list_lru_node, rcu);
> +
>  	__memcg_destroy_list_lru_node(nlru->memcg_lrus, 0, memcg_nr_cache_ids);
>  	kvfree(nlru->memcg_lrus);
>  }
>  
> +static void memcg_destroy_list_lru_node(struct list_lru_node *nlru)
> +{
> +	call_rcu(&nlru->rcu, memcg_destroy_list_lru_node_rcu);
> +}
> +
>  static int memcg_update_list_lru_node(struct list_lru_node *nlru,
>  				      int old_size, int new_size)
>  {
> @@ -371,9 +397,10 @@ static int memcg_update_list_lru_node(struct list_lru_node *nlru,
>  	 * we have to use IRQ-safe primitives here to avoid deadlock.
>  	 */
>  	spin_lock_irq(&nlru->lock);
> -	nlru->memcg_lrus = new;
> +	rcu_assign_pointer(nlru->memcg_lrus, new);
>  	spin_unlock_irq(&nlru->lock);
>  
> +	synchronize_rcu();
>  	kvfree(old);
>  	return 0;
>  }
> @@ -487,6 +514,7 @@ static void memcg_drain_list_lru_node(struct list_lru_node *nlru,
>  	 * we have to use IRQ-safe primitives here to avoid deadlock.
>  	 */
>  	spin_lock_irq(&nlru->lock);
> +	rcu_read_lock();
>  
>  	src = list_lru_from_memcg_idx(nlru, src_idx);
>  	dst = list_lru_from_memcg_idx(nlru, dst_idx);
> @@ -495,6 +523,7 @@ static void memcg_drain_list_lru_node(struct list_lru_node *nlru,
>  	dst->nr_items += src->nr_items;
>  	src->nr_items = 0;
>  
> +	rcu_read_unlock();
>  	spin_unlock_irq(&nlru->lock);
>  }
>  
> -- 
> 2.11.0

-- 
Michal Hocko
SUSE Labs