From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AD224C64E7A for ; Tue, 1 Dec 2020 10:09:12 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 00036206F9 for ; Tue, 1 Dec 2020 10:09:11 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 00036206F9 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=virtuozzo.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 7B18C6B006C; Tue, 1 Dec 2020 05:09:11 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 739C58D0002; Tue, 1 Dec 2020 05:09:11 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6022A8D0001; Tue, 1 Dec 2020 05:09:11 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0175.hostedemail.com [216.40.44.175]) by kanga.kvack.org (Postfix) with ESMTP id 4752D6B006C for ; Tue, 1 Dec 2020 05:09:11 -0500 (EST) Received: from smtpin05.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 0C56A181AEF0B for ; Tue, 1 Dec 2020 10:09:11 +0000 (UTC) X-FDA: 77544290502.05.stick08_3f0c16d273a9 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin05.hostedemail.com (Postfix) with ESMTP id E8DDD18015405 for ; Tue, 1 Dec 2020 10:09:10 +0000 (UTC) X-HE-Tag: stick08_3f0c16d273a9 X-Filterd-Recvd-Size: 6794 Received: from relay3.sw.ru (relay.sw.ru [185.231.240.75]) by imf21.hostedemail.com (Postfix) with ESMTP for ; Tue, 1 Dec 2020 10:09:10 +0000 (UTC) Received: from [192.168.15.152] by relay3.sw.ru with esmtp (Exim 4.94) (envelope-from ) id 1kk2ae-00BFRE-NK; Tue, 01 Dec 2020 13:08:56 +0300 Subject: Re: [PATCH] mm: list_lru: hold nlru lock to avoid reading transient negative nr_items To: Yang Shi , vdavydov.dev@gmail.com, guro@fb.com, shakeelb@google.com, akpm@linux-foundation.org Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20201130184514.551950-1-shy828301@gmail.com> From: Kirill Tkhai Message-ID: Date: Tue, 1 Dec 2020 13:09:04 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: <20201130184514.551950-1-shy828301@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 30.11.2020 21:45, Yang Shi wrote: > When investigating a slab cache bloat problem, significant amount of > negative dentry cache was seen, but confusingly they neither got shrunk > by reclaimer (the host has very tight memory) nor be shrunk by dropping > cache. The vmcore shows there are over 14M negative dentry objects on lru, > but tracing result shows they were even not scanned at all. The further > investigation shows the memcg's vfs shrinker_map bit is not set. So the > reclaimer or dropping cache just skip calling vfs shrinker. So we have > to reboot the hosts to get the memory back. > > I didn't manage to come up with a reproducer in test environment, and the > problem can't be reproduced after rebooting. But it seems there is race > between shrinker map bit clear and reparenting by code inspection. The > hypothesis is elaborated as below. > > The memcg hierarchy on our production environment looks like: > root > / \ > system user > > The main workloads are running under user slice's children, and it creates > and removes memcg frequently. So reparenting happens very often under user > slice, but no task is under user slice directly. > > So with the frequent reparenting and tight memory pressure, the below > hypothetical race condition may happen: > > CPU A CPU B CPU C > reparent > dst->nr_items == 0 > shrinker: > total_objects == 0 > add src->nr_items to dst > set_bit > retrun SHRINK_EMPTY > clear_bit > list_lru_del() > reparent again > dst->nr_items may go negative > due to current list_lru_del() > on CPU C > The second run of shrinker: > read nr_items without any > synchronization, so it may > see intermediate negative > nr_items then total_objects > may return 0 conincidently > > keep the bit cleared > dst->nr_items != 0 > skip set_bit > add scr->nr_item to dst Good catch, Yang. Thanks for investigating this. But I agree with Roman it's better to fix that in rare-called place (memcg_drain_list_lru_node()), than in hot place (list_lru_count_one()). Also, I'd added to description of new patch a reference to memcg_offline_kmem(), because this is the place, where child->kmemcg_id is rewritten, and this is the reason of lru's nr_items may become negative. > After this point dst->nr_item may never go zero, so reparenting will not > set shrinker_map bit anymore. And since there is no task under user > slice directly, so no new object will be added to its lru to set the > shrinker map bit either. That bit is kept cleared forever. > > How does list_lru_del() race with reparenting? It is because > reparenting replaces childen's kmemcg_id to parent's without protecting > from nlru->lock, so list_lru_del() may see parent's kmemcg_id but > actually deleting items from child's lru, but dec'ing parent's nr_items, > so the parent's nr_items may go negative as commit > 2788cf0c401c268b4819c5407493a8769b7007aa ("memcg: reparent list_lrus and > free kmemcg_id on css offline") says. > > Can we move kmemcg_id replacement after reparenting? No, because the > race with list_lru_del() may result in negative src->nr_items, but it > will never be fixed. So the shrinker may never return SHRINK_EMPTY then > keep the shrinker map bit set always. The shrinker will be always > called for nonsense. > > Can we synchronize list_lru_del() and reparenting? Yes, it could be > done. But it seems we need introduce a new lock or use nlru->lock. But > it sounds complicated to move kmemcg_id replacement code under nlru->lock. > And list_lru_del() may be called quite often to exacerbate some hot > path, i.e. dentry kill. > > So, it sounds acceptable to synchronize reading nr_items to avoid seeing > intermediate negative nr_items given the simplicity and it is typically > just called by shrinkers when counting the freeable objects. > > The patch is tested with some shrinker intensive workloads, no > noticeable regression is soptted. > > Cc: Vladimir Davydov > Cc: Kirill Tkhai > Cc: Roman Gushchin > Cc: Shakeel Butt > Signed-off-by: Yang Shi > --- > mm/list_lru.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/mm/list_lru.c b/mm/list_lru.c > index 5aa6e44bc2ae..5c128a7710ff 100644 > --- a/mm/list_lru.c > +++ b/mm/list_lru.c > @@ -178,10 +178,17 @@ unsigned long list_lru_count_one(struct list_lru *lru, > struct list_lru_one *l; > unsigned long count; > > - rcu_read_lock(); > + /* > + * Since list_lru_{add,del} may be called under an IRQ-safe lock, > + * we have to use IRQ-safe primitives here to avoid deadlock. > + * > + * Hold the lock to prevent from seeing transient negative > + * nr_items value. > + */ > + spin_lock_irq(&nlru->lock); > l = list_lru_from_memcg_idx(nlru, memcg_cache_id(memcg)); > count = READ_ONCE(l->nr_items); > - rcu_read_unlock(); > + spin_unlock_irq(&nlru->lock); > > return count; > } >