Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 22 Apr 2022 11:19:05 +1000
From:   Dave Chinner <dchinner@redhat.com>
To:     Roman Gushchin <roman.gushchin@linux.dev>
Cc:     David Hildenbrand <david@redhat.com>, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Shakeel Butt <shakeelb@google.com>,
        Yang Shi <shy828301@gmail.com>
Subject: Re: [PATCH] mm: do not call add_nr_deferred() with zero deferred
Message-ID: <YmICiX2DFSveY17Z@rh>
References: <20220416004104.4089743-1-roman.gushchin@linux.dev>
 <59404249-de0c-c569-d04a-9da38ed14b0a@redhat.com>
 <Yl7mdguNR3CabMAN@carbon>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yl7mdguNR3CabMAN@carbon>
Precedence: bulk

On Tue, Apr 19, 2022 at 09:42:30AM -0700, 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,

Never been able to measure it myself.

HwoeverI'd much prefer the tracepoint output stays accurate - I've had to
post-process and/or graph the shrinker progress as reported by the
start/end tracpoints to find problems in the algorithms in the past.
That's why there is the additional complexity in the code to make
sure the coutners are accurate in the first place.

> however the added complexity is also small: one "if" statement.

Yeah, complexity is not the problem here - it's that accuracy of the
tracepoints has actually mattered to me in the past...

Cheers,

DAve.
-- 
Dave Chinner
dchinner@redhat.com