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=-7.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no 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 50FCAC83017 for ; Tue, 1 Dec 2020 17:20:37 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 92BAE206E0 for ; Tue, 1 Dec 2020 17:20:36 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="iZ9PBj8O" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 92BAE206E0 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 073666B005D; Tue, 1 Dec 2020 12:20:36 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 024AE8D0002; Tue, 1 Dec 2020 12:20:35 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E07878D0001; Tue, 1 Dec 2020 12:20:35 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0110.hostedemail.com [216.40.44.110]) by kanga.kvack.org (Postfix) with ESMTP id C9CA66B005D for ; Tue, 1 Dec 2020 12:20:35 -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 82F4C181AEF23 for ; Tue, 1 Dec 2020 17:20:35 +0000 (UTC) X-FDA: 77545377630.05.twist85_06177f6273ac Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin05.hostedemail.com (Postfix) with ESMTP id 6F5CD1826B6BF for ; Tue, 1 Dec 2020 17:20:35 +0000 (UTC) X-HE-Tag: twist85_06177f6273ac X-Filterd-Recvd-Size: 10180 Received: from mail-ej1-f67.google.com (mail-ej1-f67.google.com [209.85.218.67]) by imf26.hostedemail.com (Postfix) with ESMTP for ; Tue, 1 Dec 2020 17:20:34 +0000 (UTC) Received: by mail-ej1-f67.google.com with SMTP id lt17so5740393ejb.3 for ; Tue, 01 Dec 2020 09:20:34 -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=MTaZh6qlC8fMdWEL3sssg+oqlnsdSx1UbYzoEkEYxpc=; b=iZ9PBj8OrO9axqQj9THLEa9tudhLlyOEDMd7OcQ8Yr3Ui8/Y+VpnEwWZ+ym7P73dCd M2+N7aH5hhWNrPDeKtOD5UP/YvCboyDJ98ogdGit8AjwQeqqupFONC01G/5/Nfkrm8xV LYi3s21IvnchYtCswfCCgAlTTLycYXKRwk0aD2oVhihLNnr90l8GvYwfieV82D/Bb8cs kepqreIyDPi0JCeRvFaNBHuUoD9j+jDOQy+MTvuDNPtw2MID6Qjag5FRBN898gLXtUUE /2uSCbLYhk/69pVWZM7KdMznhCYUa5X7tPlAN3PCEyXVpaZifBFzM9cMNWF2b64K7gLt VhQQ== 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=MTaZh6qlC8fMdWEL3sssg+oqlnsdSx1UbYzoEkEYxpc=; b=ax8q8JzLdM24+02jH6hpIrmZmJAjZrpUKR2WcIzganZl+L865o0u0An3V4/L3v2i9n +URhQFGpikxYCAF0m7hwN+5D7pwCBTIhWPUfUN2Zm08FtRTRdOn5yXMPEdPXxFlGBAv6 oWJU0tWCHIkTOd6cp6BwtK4n3smtkHaWhl4XMntCrlxdxrogHwZBWA0dFaxjJUjfRkwM LCnY49Q/K6yCbA4t+pzp8Rw+P+KjvZAJthIXHuotg7o0IaYh8lnyQBq8kadPThpWjCGB sIQKTnZJTVl1XOmm5Qtpl+8BnbeYP0J6J1P1EdsN8pxs0dF9aa2CWaJPRSUJ2dJY1XXd wYcw== X-Gm-Message-State: AOAM530uPqRn5GFT1PvIsVS1I5QGszaT19DTxrRO5h40eyt0lbeaI9ol pgWgzrt0wzsaIXs7pCVjlQEai5WjV7JHQCrWmd8= X-Google-Smtp-Source: ABdhPJyX343fuQFvay2XOLPeBFYpa63dwr3R4ZUlIIJjGT0gzDRcuvKcvP6Zv58hmAxTMWidVK7z47KXN8RJjP/kljI= X-Received: by 2002:a17:907:20a4:: with SMTP id pw4mr2969468ejb.499.1606843233520; Tue, 01 Dec 2020 09:20:33 -0800 (PST) MIME-Version: 1.0 References: <20201130184514.551950-1-shy828301@gmail.com> <20201130200936.GA1354703@carbon.DHCP.thefacebook.com> In-Reply-To: From: Yang Shi Date: Tue, 1 Dec 2020 09:20:21 -0800 Message-ID: Subject: Re: [PATCH] mm: list_lru: hold nlru lock to avoid reading transient negative nr_items To: Kirill Tkhai Cc: Roman Gushchin , Vladimir Davydov , 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 9:17 AM Kirill Tkhai wrote: > > On 01.12.2020 20:15, Yang Shi wrote: > > On Tue, Dec 1, 2020 at 2:25 AM Kirill Tkhai wrote: > >> > >> 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. > > > > You mean something like the below (diff based on Roman's proposal)? > > > > diff --git a/mm/list_lru.c b/mm/list_lru.c > > index 127c2cf9f831..fe230081690b 100644 > > --- a/mm/list_lru.c > > +++ b/mm/list_lru.c > > @@ -545,10 +545,12 @@ 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); > > - dst->nr_items += src->nr_items; > > - if (src->nr_items) > > + > > + if (src->nr_items) { > > + dst->nr_items += src->nr_items; > > memcg_set_shrinker_bit(dst_memcg, nid, lru_shrinker_id(lru)); > > - src->nr_items = 0; > > + src->nr_items = 0; > > + } > > > > spin_unlock_irq(&nlru->lock); > > Yes. Thanks for confirming. Will solve all the comments in v2.