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=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 3593CC64E7B for ; Tue, 1 Dec 2020 17:19:20 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8C04B206E0 for ; Tue, 1 Dec 2020 17:19:19 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="mHJRr5IQ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8C04B206E0 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 04E8A6B005D; Tue, 1 Dec 2020 12:19:19 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id F414C6B0068; Tue, 1 Dec 2020 12:19:18 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E7E7C8D0001; Tue, 1 Dec 2020 12:19:18 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0119.hostedemail.com [216.40.44.119]) by kanga.kvack.org (Postfix) with ESMTP id D21B16B005D for ; Tue, 1 Dec 2020 12:19:18 -0500 (EST) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 70B0533C4 for ; Tue, 1 Dec 2020 17:19:18 +0000 (UTC) X-FDA: 77545374396.30.soap49_461128f273ac Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin30.hostedemail.com (Postfix) with ESMTP id 3332B180B3AA7 for ; Tue, 1 Dec 2020 17:19:18 +0000 (UTC) X-HE-Tag: soap49_461128f273ac X-Filterd-Recvd-Size: 8982 Received: from mail-ed1-f65.google.com (mail-ed1-f65.google.com [209.85.208.65]) by imf08.hostedemail.com (Postfix) with ESMTP for ; Tue, 1 Dec 2020 17:19:17 +0000 (UTC) Received: by mail-ed1-f65.google.com with SMTP id q16so4362274edv.10 for ; Tue, 01 Dec 2020 09:19:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=LQKIIhxcjoKVEtwnQ+W/fhUOvKOLq2pB3RUeQtzqXE0=; b=mHJRr5IQ0LM2xeUQGg8kKZ4FHj8Yn4VIVKEcX3t0MtTupgfvFybzy3Rph+eEuHpjon rPote0nXx21oj8cJmXYSEHIwjLihOrFdZziNCckU5Agi0jt2ryCcBRmPZpxrnmaxR1vS F8AaUCJsQJWIhBOjahpFJ49UB/wv398rJbPufSwaGUjnU4FNSfkFFxlgRpttTbwZp1JP +MUQMUR5ToW0ImCOa96JISZY9+0PhIE1IFDJkIhBSO2gpu3kZ89gILYkghqm7wgzkWmg we5dc/k7I5r73vYF3lqnzLokNA+zJ5DhIa7h1sxMMes3Hv1+gRVvkxULSzam2t8G9MVh Ba1Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=LQKIIhxcjoKVEtwnQ+W/fhUOvKOLq2pB3RUeQtzqXE0=; b=fVbHAbrQLMbFvQSSglUf82/q8+IKdwitQRaGZwODl/wYEOPxWHP51clOhW2bJoL+Ai tuBLBp5Q2YlVhX/Oz63hYt1jPqbEKYmcTSfq2TBaJI71S52Q3iWvyPOnFGAG6vkkPm/k EY/TdwgYWldCm1bA8kYQGMcuWKLdn9V2f/+1Q1m8he5Y7aXEbBz6H5FhmQaUq73UrDpj /Gckd5EV+oQ/94bt+AwGU+m2vuGq6uIXt5PbvbvWSq6CEB23X/FM8oSgPoskCU/bC+8j t9EjheKqEhHL6ikSCMDhSluMWXPEsryKIg2qhHHdpnPxkgrhG2xca2c+wopnLMsGbHvH rdJA== X-Gm-Message-State: AOAM530L4ru6PzBQj/g+2a/VaU++VWR4vmoZ3U+7ZcOZLU8AX8o79pEt PI7WaX+TahUF5OcxJlwfd2M4mYuxKKzCLSWnS0cagDfrmBk= X-Google-Smtp-Source: ABdhPJwPxJwfKRbLCo1ScpWc5QcnUc5Bh0dSP8w2ecuzPNnP0wjC2azPfNOI/vc4wzbVVpi9wlWUn67wFNbpDGQPT6A= X-Received: by 2002:a05:6402:1c8a:: with SMTP id cy10mr4016563edb.151.1606843156527; Tue, 01 Dec 2020 09:19:16 -0800 (PST) MIME-Version: 1.0 References: <20201130184514.551950-1-shy828301@gmail.com> In-Reply-To: From: Yang Shi Date: Tue, 1 Dec 2020 09:19:04 -0800 Message-ID: Subject: Re: [PATCH] mm: list_lru: hold nlru lock to avoid reading transient negative nr_items To: Kirill Tkhai Cc: Vladimir Davydov , Roman Gushchin , Shakeel Butt , Andrew Morton , Linux MM , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" 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 Tue, Dec 1, 2020 at 2:09 AM Kirill Tkhai wrote: > > 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()). Yes, agreed. Will incarnate Roman's proposal in v2. > > 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. Sure. > > > 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; > > } > > > >