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

From: Kirill Tkhai
Date: Tue Mar 27 2018 - 05:41:31 EST


On 27.03.2018 12:30, Li,Rongqing wrote:
>
>
>> -----邮件原件-----
>> 发件人: Vladimir Davydov [mailto:vdavydov.dev@xxxxxxxxx]
>> 发送时间: 2018年3月27日 17:09
>> 收件人: Michal Hocko <mhocko@xxxxxxxxxx>
>> 抄送: Li,Rongqing <lirongqing@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
>> linux-mm@xxxxxxxxx; Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>;
>> Johannes Weiner <hannes@xxxxxxxxxxx>; Dave Chinner
>> <david@xxxxxxxxxxxxx>; Kirill Tkhai <ktkhai@xxxxxxxxxxxxx>
>> 主题: Re: [PATCH] mm/list_lru: replace spinlock with RCU in
>> __list_lru_count_one
>>
>> [Cc Kirill]
>>
>> AFAIU this has already been fixed in exactly the same fashion by Kirill
>> (mmotm commit 8e7d1201ec71 "mm: make counting of
>> list_lru_one::nr_items lockless"). Kirill is working on further optimizations
>> right now, see
>>
>>
>
> Ok, thanks

Thanks Vladimir, for CCing me.

Rong, if your are interested I may start to add you to CC on further iterations
of https://marc.info/?i=152163840790.21546.980703278415599202.stgit%40localhost.localdomain
since there are many people which meet such the problem.

Kirill

>
>> https://lkml.kernel.org/r/152163840790.21546.980703278415599202.stgit
>> @localhost.localdomain
>>
>> On Tue, Mar 27, 2018 at 10:15:46AM +0200, Michal Hocko wrote:
>>> [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@xxxxxxxxx>
>>>> ---
>>>> 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
>>>