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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0EFF5C433EF for ; Wed, 20 Apr 2022 23:30:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 941976B0073; Wed, 20 Apr 2022 19:30:47 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8BF786B0074; Wed, 20 Apr 2022 19:30:47 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 78B656B0075; Wed, 20 Apr 2022 19:30:47 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.25]) by kanga.kvack.org (Postfix) with ESMTP id 660196B0073 for ; Wed, 20 Apr 2022 19:30:47 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 35182229F0 for ; Wed, 20 Apr 2022 23:30:47 +0000 (UTC) X-FDA: 79378854534.11.D36F6E3 Received: from mail-pl1-f182.google.com (mail-pl1-f182.google.com [209.85.214.182]) by imf31.hostedemail.com (Postfix) with ESMTP id 777F620018 for ; Wed, 20 Apr 2022 23:30:44 +0000 (UTC) Received: by mail-pl1-f182.google.com with SMTP id b7so3217245plh.2 for ; Wed, 20 Apr 2022 16:30:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=kLshpcQDnpXfKVw8FlVeldYYjsS62gfK++RoyzE7fzI=; b=oN1RALoxj7uG5b822TkWSWP45NtB+Ln9k0aoSR3WnyYaApx1+JSppvo2ykhYM//PRc IN/5wlSvS2z2LAZkQ0diIyFPVUDxVUFtQBLURIZkvPbc7vELcQIxPiEEfjFMQ8UaHW/e lJXAfSbLS/qXG0XbLR8IGzvW5hdJ3jp3U8WYLfNWCll3eryaibcxuaZk+9cx2YtDGF67 Q8qqx4JyiXCv6/ReHHFjKo1mog3EnbwYvdv4EXin04QCPlsXvw6+pZekC9jpOZQ51V+s p1WLH0nBkaZaBXW+UKvJrB28hqIYVFXxZK6LaQeuuYDLxdD8Ngo2cOEnYwVr2d9nfkHE a4Aw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=kLshpcQDnpXfKVw8FlVeldYYjsS62gfK++RoyzE7fzI=; b=bwVkg890NC00NXH6wvplfcw7W34BRRLHEmfKXYfknJh0GzmC56YdHZlOYC3E2pm0wv ouwzpfjjzmPEzMOrxjezutR66Psa6+JVBQ/d1p2V1REREf18dt+8paZ6ECw2XfNGi2ma QZPWw3ZD20V5oDsp9Puv6WDPqe/+akH/7b+9a9cWVLybdmXgt9b45Tk8RJMHYbGJTNdf fSu1Q+N3oehBzBY5l7M5HoVjUninvnnv5n6Jt4ZQ7kHhHCh/IC2SwCRz+y5YUR6R+Qzp aDJgy+pHB5LA7xfEVZmXz679/aoWK+tg6+LhtE6rMEARRI9WCDuYi58ZRmgXXHs2UaoJ fCKg== X-Gm-Message-State: AOAM533GNDASdDGBwdrPhddeVUlo3dvZJ5tahFrTGyWZkXkjhKLbitVl AyW7rzn6Pmblx7NxzCWR3dYWhUlW4HGkriQZ3WY= X-Google-Smtp-Source: ABdhPJyWXDZORaeQ0qCu3csj9OQySHeqsHkp4w1tIylPxsLUuQagD/rZ3vVbcJzMbMl/q6KmzPH3s0pezJ5GqBbh1mo= X-Received: by 2002:a17:90b:4a85:b0:1d2:aee9:23ce with SMTP id lp5-20020a17090b4a8500b001d2aee923cemr7161666pjb.99.1650497445807; Wed, 20 Apr 2022 16:30:45 -0700 (PDT) MIME-Version: 1.0 References: <20220416004104.4089743-1-roman.gushchin@linux.dev> <59404249-de0c-c569-d04a-9da38ed14b0a@redhat.com> In-Reply-To: From: Yang Shi Date: Wed, 20 Apr 2022 16:30:33 -0700 Message-ID: Subject: Re: [PATCH] mm: do not call add_nr_deferred() with zero deferred To: David Hildenbrand Cc: Roman Gushchin , Linux MM , Andrew Morton , Dave Chinner , Linux Kernel Mailing List , Johannes Weiner , Michal Hocko , Shakeel Butt Content-Type: text/plain; charset="UTF-8" X-Rspam-User: X-Rspamd-Queue-Id: 777F620018 X-Stat-Signature: ahd5ohiwqrounkjf4b9493f6m4r7m8zc Authentication-Results: imf31.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=oN1RALox; spf=pass (imf31.hostedemail.com: domain of shy828301@gmail.com designates 209.85.214.182 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspamd-Server: rspam01 X-HE-Tag: 1650497444-765211 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, Apr 19, 2022 at 9:57 AM David Hildenbrand wrote: > > On 19.04.22 18:42, Roman Gushchin wrote: > > On Tue, Apr 19, 2022 at 02:56:06PM +0200, David Hildenbrand wrote: > >> On 16.04.22 02:41, Roman Gushchin wrote: > >>> add_nr_deferred() is often called with next_deferred equal to 0. > >>> For instance, it's happening under low memory pressure for any > >>> shrinkers with a low number of cached objects. A corresponding trace > >>> looks like: > >>> <...>-619914 [005] .... 467456.345160: mm_shrink_slab_end: \ > >>> super_cache_scan+0x0/0x1a0 0000000087027f06: nid: 1 \ > >>> unused scan count 0 new scan count 0 total_scan 0 \ > >>> last shrinker return val 0 > >>> > >>> <...>-619914 [005] .... 467456.345371: mm_shrink_slab_end: \ > >>> super_cache_scan+0x0/0x1a0 0000000087027f06: nid: 1 \ > >>> unused scan count 0 new scan count 0 total_scan 0 \ > >>> last shrinker return val 0 > >>> > >>> <...>-619914 [005] .... 467456.345380: mm_shrink_slab_end: \ > >>> super_cache_scan+0x0/0x1a0 0000000087027f06: nid: 1 unused \ > >>> scan count 0 new scan count 0 total_scan 0 \ > >>> last shrinker return val 0 > >>> > >>> This lead to unnecessary checks and atomic operations, which can be > >>> avoided by checking next_deferred for not being zero before calling > >>> add_nr_deferred(). In this case the mm_shrink_slab_end trace point > >>> will get a potentially slightly outdated "new scan count" value, but > >>> it's totally fine. > >> > >> Sufficient improvement to justify added complexity for anybody reading > >> that code? > > > > I don't have any numbers and really doubt the difference is significant, > > however the added complexity is also small: one "if" statement. > > Anyway, if you feel strongly against this change, I'm fine with dropping it. > > > > No strong opinion, naturally, more conditions make the code harder to > read -- that's why I'm asking. This is why that "if" was removed by commit 867508304685 ("mm: vmscan: use per memcg nr_deferred of shrinker") since it didn't bring in measurable performance improvement. TBH I'm not sure whether it is worth it with no measurable performance boost but harder to read code and potential outdated "new scan count". > > >> > >>> > >>> Signed-off-by: Roman Gushchin > >>> --- > >>> mm/vmscan.c | 5 ++++- > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/mm/vmscan.c b/mm/vmscan.c > >>> index d4a7d2bd276d..19d3d4fa1aad 100644 > >>> --- a/mm/vmscan.c > >>> +++ b/mm/vmscan.c > >>> @@ -808,7 +808,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, > >>> * move the unused scan count back into the shrinker in a > >>> * manner that handles concurrent updates. > >>> */ > >>> - new_nr = add_nr_deferred(next_deferred, shrinker, shrinkctl); > >>> + if (next_deferred) > >>> + new_nr = add_nr_deferred(next_deferred, shrinker, shrinkctl); > >>> + else > >>> + new_nr = nr; > >>> > >>> trace_mm_shrink_slab_end(shrinker, shrinkctl->nid, freed, nr, new_nr, total_scan); > >>> return freed; > >> > >> And if we still want to do this optimization, why not put it into > >> add_nr_deferred()? > > > > Because of the semantics of add_nr_deferred(), which returns the deferred value. > > It's not used for anything except tracing, so maybe it's a place for another > > change. > > Skimming over the code I somehow missed that add_nr_deferred() doesn't > have "nr" naturally available. > > LGTM > > Acked-by: David Hildenbrand > > > -- > Thanks, > > David / dhildenb >