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 8A53DC433E0 for ; Thu, 4 Feb 2021 17:33:02 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 18FBD64DA1 for ; Thu, 4 Feb 2021 17:33:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 18FBD64DA1 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 8048F6B0005; Thu, 4 Feb 2021 12:33:01 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 78DC76B0006; Thu, 4 Feb 2021 12:33:01 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 655C16B0070; Thu, 4 Feb 2021 12:33:01 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0029.hostedemail.com [216.40.44.29]) by kanga.kvack.org (Postfix) with ESMTP id 4B45A6B0005 for ; Thu, 4 Feb 2021 12:33:01 -0500 (EST) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 0A6A41EE6 for ; Thu, 4 Feb 2021 17:33:01 +0000 (UTC) X-FDA: 77781280962.13.pull64_3416982275dd Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin13.hostedemail.com (Postfix) with ESMTP id DD69318140B67 for ; Thu, 4 Feb 2021 17:33:00 +0000 (UTC) X-HE-Tag: pull64_3416982275dd X-Filterd-Recvd-Size: 7646 Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) by imf04.hostedemail.com (Postfix) with ESMTP for ; Thu, 4 Feb 2021 17:33:00 +0000 (UTC) Received: by mail-lf1-f50.google.com with SMTP id u25so5779447lfc.2 for ; Thu, 04 Feb 2021 09:33:00 -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=D9/da0TYqc2IuJFpJdibkLommX5XSKGKPIxcqiy6/RM=; b=jxK2pLrz7T9ANF2wqKTw7AKiJ7M0bYH1vM2tXWf0st/UY2Rz50ckQfB2Y+448RVr/L nME7/yCM2E7dYcUxLvVSNnM6Vjk513mgj8U70Dldayl76491r/G/A9Dh23SxOsLyXgJx Guo2P4wrf/AJrs6P97SVbPy1odjB4an45VuapaMlWVyd44lkoptaeaoML48Bs0/Wu/T3 zQ9rVePBSS5p1+JaPwCUBaAIdWMTLKlFE371UGwkFlWyux23z1klAJAH/mbnaN6iXEIA M0HdoEVCue+e0/30e+am4lqWGZweIchKO0te8189gBrR0rwe3bSP1v9SFql6qFfAdfIH luxA== 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=D9/da0TYqc2IuJFpJdibkLommX5XSKGKPIxcqiy6/RM=; b=X3rCQjXsj3SPRn9oFbcldCQxaLJjrqf3PatGlrza5Yio2y7tTb2FxS2tL/45HkVf88 br0C2lUaORcBjYnYvCrszblsYA0xsVgcBMPxnVGjn3Eg7YSn7fYGJRI/bO2TmF6GBORG aWgzqe8Hy+s9jFVfHWOS3JquffztaIjIzH8+dHm/h871vLRyP/LIwprBCg8SNJ6UyKl6 9hLDgSrIINV4piuZdcStaiq9a2W0faw4sr4zhHPFA/AF7dKv5DeC38FWrCnGVWmx1PnP s2aPS4LOp6wKoOdmomJg1XP+HPIbY+OeBbBkQho9Z7l7y+Ap16PLXEa+dxe/DmXsXuGs bH6Q== X-Gm-Message-State: AOAM530vvRzYT2Y8de7fZBAL3Ds/Lt9tQXXbo0jk8T1u6C5vKmlpksq/ 9Tn74haxmXmbc6etvVXl1RhSpj+Cp3k5xLUioGk= X-Google-Smtp-Source: ABdhPJxskkEcudNmLMXwFbaywk0AsuzD69Nc7ex/EdDYH309v5vJBCtwQYOtDY9HFfxdJeg3cOVUTdY5u+uCKQ7uncc= X-Received: by 2002:a05:6512:3772:: with SMTP id z18mr243698lft.620.1612459978869; Thu, 04 Feb 2021 09:32:58 -0800 (PST) MIME-Version: 1.0 References: <20210203172042.800474-1-shy828301@gmail.com> <20210203172042.800474-10-shy828301@gmail.com> <656865f5-bb56-4f4c-b88d-ec933a042b4c@virtuozzo.com> <5e335e4a-1556-e694-8f0b-192d924f99e5@virtuozzo.com> In-Reply-To: <5e335e4a-1556-e694-8f0b-192d924f99e5@virtuozzo.com> From: Yang Shi Date: Thu, 4 Feb 2021 09:32:46 -0800 Message-ID: Subject: Re: [v6 PATCH 09/11] mm: vmscan: don't need allocate shrinker->nr_deferred for memcg aware shrinkers To: Kirill Tkhai Cc: Roman Gushchin , Vlastimil Babka , Shakeel Butt , Dave Chinner , Johannes Weiner , Michal Hocko , Andrew Morton , Linux MM , Linux FS-devel Mailing List , 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 Thu, Feb 4, 2021 at 2:14 AM Kirill Tkhai wrote: > > On 04.02.2021 12:29, Kirill Tkhai wrote: > > On 03.02.2021 20:20, Yang Shi wrote: > >> Now nr_deferred is available on per memcg level for memcg aware shrinkers, so don't need > >> allocate shrinker->nr_deferred for such shrinkers anymore. > >> > >> The prealloc_memcg_shrinker() would return -ENOSYS if !CONFIG_MEMCG or memcg is disabled > >> by kernel command line, then shrinker's SHRINKER_MEMCG_AWARE flag would be cleared. > >> This makes the implementation of this patch simpler. > >> > >> Acked-by: Vlastimil Babka > >> Signed-off-by: Yang Shi > >> --- > >> mm/vmscan.c | 31 ++++++++++++++++--------------- > >> 1 file changed, 16 insertions(+), 15 deletions(-) > >> > >> diff --git a/mm/vmscan.c b/mm/vmscan.c > >> index 545422d2aeec..20a35d26ae12 100644 > >> --- a/mm/vmscan.c > >> +++ b/mm/vmscan.c > >> @@ -334,6 +334,9 @@ static int prealloc_memcg_shrinker(struct shrinker *shrinker) > >> { > >> int id, ret = -ENOMEM; > >> > >> + if (mem_cgroup_disabled()) > >> + return -ENOSYS; > >> + > >> down_write(&shrinker_rwsem); > >> /* This may call shrinker, so it must use down_read_trylock() */ > >> id = idr_alloc(&shrinker_idr, shrinker, 0, 0, GFP_KERNEL); > >> @@ -414,7 +417,7 @@ static bool writeback_throttling_sane(struct scan_control *sc) > >> #else > >> static int prealloc_memcg_shrinker(struct shrinker *shrinker) > >> { > >> - return 0; > >> + return -ENOSYS; > >> } > >> > >> static void unregister_memcg_shrinker(struct shrinker *shrinker) > >> @@ -525,8 +528,18 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone > >> */ > >> int prealloc_shrinker(struct shrinker *shrinker) > >> { > >> - unsigned int size = sizeof(*shrinker->nr_deferred); > >> + unsigned int size; > >> + int err; > >> + > >> + if (shrinker->flags & SHRINKER_MEMCG_AWARE) { > >> + err = prealloc_memcg_shrinker(shrinker); > >> + if (err != -ENOSYS) > >> + return err; > >> > >> + shrinker->flags &= ~SHRINKER_MEMCG_AWARE; > >> + } > >> + > >> + size = sizeof(*shrinker->nr_deferred); > >> if (shrinker->flags & SHRINKER_NUMA_AWARE) > >> size *= nr_node_ids; > > > > This may sound surprisingly, but IIRC do_shrink_slab() may be called on early boot > > *even before* root_mem_cgroup is allocated. AFAIR, I received syzcaller crash report > > because of this, when I was implementing shrinker_maps. > > > > This is a reason why we don't use shrinker_maps even in case of mem cgroup is not > > disabled: we iterate every shrinker of shrinker_list. See check in shrink_slab(): > > > > if (!mem_cgroup_disabled() && !mem_cgroup_is_root(memcg)) > > > > Possible, we should do the same for nr_deferred: 1)always allocate shrinker->nr_deferred, > > 2)use shrinker->nr_deferred in count_nr_deferred() and set_nr_deferred(). > > I looked over my mail box, and I can't find that crash report and conditions to reproduce. > > Hm, let's remain this as is, and we rework this in case of such early shrinker call is still > possible, and there will be a report... Sure. But I'm wondering how that could happen. On a very small machine? > > Reviewed-by: Kirill Tkhai > > With only nit: > > >> > >> @@ -534,26 +547,14 @@ int prealloc_shrinker(struct shrinker *shrinker) > >> if (!shrinker->nr_deferred) > >> return -ENOMEM; > >> > >> - if (shrinker->flags & SHRINKER_MEMCG_AWARE) { > >> - if (prealloc_memcg_shrinker(shrinker)) > >> - goto free_deferred; > >> - } > >> > >> return 0; > >> - > >> -free_deferred: > >> - kfree(shrinker->nr_deferred); > >> - shrinker->nr_deferred = NULL; > >> - return -ENOMEM; > >> } > >> > >> void free_prealloced_shrinker(struct shrinker *shrinker) > >> { > >> - if (!shrinker->nr_deferred) > >> - return; > >> - > >> if (shrinker->flags & SHRINKER_MEMCG_AWARE) > >> - unregister_memcg_shrinker(shrinker); > >> + return unregister_memcg_shrinker(shrinker); > > I've never seen return of void function in linux kernel. I'm not sure this won't confuse people. Will fix in v7. > > >> > >> kfree(shrinker->nr_deferred); > >> shrinker->nr_deferred = NULL; > >> > > > >