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=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,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 8DBE2C64E7A for ; Tue, 1 Dec 2020 10:25:54 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D37552080A for ; Tue, 1 Dec 2020 10:25:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D37552080A 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 E50436B0068; Tue, 1 Dec 2020 05:25:52 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E01138D0002; Tue, 1 Dec 2020 05:25:52 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CEFCD8D0001; Tue, 1 Dec 2020 05:25:52 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0167.hostedemail.com [216.40.44.167]) by kanga.kvack.org (Postfix) with ESMTP id B69646B0068 for ; Tue, 1 Dec 2020 05:25:52 -0500 (EST) Received: from smtpin06.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 7778B180AD811 for ; Tue, 1 Dec 2020 10:25:52 +0000 (UTC) X-FDA: 77544332544.06.pear20_3e13ef4273a9 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin06.hostedemail.com (Postfix) with ESMTP id 543811003CE2E for ; Tue, 1 Dec 2020 10:25:52 +0000 (UTC) X-HE-Tag: pear20_3e13ef4273a9 X-Filterd-Recvd-Size: 6726 Received: from relay3.sw.ru (relay.sw.ru [185.231.240.75]) by imf01.hostedemail.com (Postfix) with ESMTP for ; Tue, 1 Dec 2020 10:25:51 +0000 (UTC) Received: from [192.168.15.152] by relay3.sw.ru with esmtp (Exim 4.94) (envelope-from ) id 1kk2qo-00BFaq-VP; Tue, 01 Dec 2020 13:25:39 +0300 Subject: Re: [PATCH] mm: list_lru: hold nlru lock to avoid reading transient negative nr_items To: Roman Gushchin , Yang Shi Cc: vdavydov.dev@gmail.com, shakeelb@google.com, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20201130184514.551950-1-shy828301@gmail.com> <20201130200936.GA1354703@carbon.DHCP.thefacebook.com> From: Kirill Tkhai Message-ID: Date: Tue, 1 Dec 2020 13:25:47 +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: <20201130200936.GA1354703@carbon.DHCP.thefacebook.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 23:09, Roman Gushchin wrote: > On Mon, Nov 30, 2020 at 10:45:14AM -0800, 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 >> >> 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. > > Hi Yang! > > It's really tricky, thank you for digging in! It's a perfect analysis! > > I wonder though, if it's better to just always set the shrinker bit on reparenting > if we do reparent some items? Then we'll avoid adding new synchronization > to the hot path. What do you think? > > -- > > @@ -534,7 +534,6 @@ static void memcg_drain_list_lru_node(struct list_lru *lru, int nid, > struct list_lru_node *nlru = &lru->node[nid]; > int dst_idx = dst_memcg->kmemcg_id; > struct list_lru_one *src, *dst; > - bool set; > > /* > * Since list_lru_{add,del} may be called under an IRQ-safe lock, > @@ -546,9 +545,8 @@ static void memcg_drain_list_lru_node(struct list_lru *lru, int nid, > dst = list_lru_from_memcg_idx(nlru, dst_idx); > > list_splice_init(&src->list, &dst->list); > - set = (!dst->nr_items && src->nr_items); > dst->nr_items += src->nr_items; > - if (set) > + if (src->nr_items) > memcg_set_shrinker_bit(dst_memcg, nid, lru_shrinker_id(lru)); > src->nr_items = 0; This looks like a good fix. To make a code more clear, we may also want to group neighbouring lines under the same "if" branch in Yang's v2 resend. Thanks, Kirill