linux-kernel - Re: [PATCH 12/28] shrinker: defer work only to kswapd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191115172301.GB55854@bfoster>
Date:   Fri, 15 Nov 2019 12:23:01 -0500
From:   Brian Foster <bfoster@...hat.com>
To:     Dave Chinner <david@...morbit.com>
Cc:     linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 12/28] shrinker: defer work only to kswapd

On Fri, Nov 15, 2019 at 08:11:50AM +1100, Dave Chinner wrote:
> On Mon, Nov 04, 2019 at 10:29:54AM -0500, Brian Foster wrote:
> > On Fri, Nov 01, 2019 at 10:46:02AM +1100, Dave Chinner wrote:
> > > @@ -601,10 +605,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > >  	 * scanning at high prio and therefore should try to reclaim as much as
> > >  	 * possible.
> > >  	 */
> > > -	while (total_scan >= batch_size ||
> > > -	       total_scan >= freeable_objects) {
> > > +	while (scan_count >= batch_size ||
> > > +	       scan_count >= freeable_objects) {
> > >  		unsigned long ret;
> > > -		unsigned long nr_to_scan = min(batch_size, total_scan);
> > > +		unsigned long nr_to_scan = min_t(long, batch_size, scan_count);
> > >  
> > >  		shrinkctl->nr_to_scan = nr_to_scan;
> > >  		shrinkctl->nr_scanned = nr_to_scan;
> > > @@ -614,29 +618,29 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > >  		freed += ret;
> > >  
> > >  		count_vm_events(SLABS_SCANNED, shrinkctl->nr_scanned);
> > > -		total_scan -= shrinkctl->nr_scanned;
> > > -		scanned += shrinkctl->nr_scanned;
> > > +		scan_count -= shrinkctl->nr_scanned;
> > > +		scanned_objects += shrinkctl->nr_scanned;
> > >  
> > >  		cond_resched();
> > >  	}
> > > -
> > >  done:
> > > -	if (next_deferred >= scanned)
> > > -		next_deferred -= scanned;
> > > +	if (deferred_count)
> > > +		next_deferred = deferred_count - scanned_objects;
> > >  	else
> > > -		next_deferred = 0;
> > > +		next_deferred = scan_count;
> > 
> > Hmm.. so if there was no deferred count on this cycle, we set
> > next_deferred to whatever is left from scan_count and add that back into
> > the shrinker struct below. If there was a pending deferred count on this
> > cycle, we subtract what we scanned from that and add that value back.
> > But what happens to the remaining scan_count in the latter case? Is it
> > lost, or am I missing something?
> 
> if deferred_count is not zero, then it is kswapd that is running. It
> does the deferred work, and if it doesn't make progress then adding
> it's scan count to the deferred work doesn't matter. That's because
> it will come back with an increased priority in a short while and
> try to scan more of the deferred count plus it's larger scan count.
> 

Ok, so perhaps there is no functional reason to defer remaining scan
count from a context (i.e. kswapd) that attempts to process deferred
work...

> IOWs, if we defer kswapd unused scan count, we effectively increase
> the pressure as the priority goes up, potentially making the
> deferred count increase out of control. i.e. kswapd can make
> progress and free items, but the result is that it increased the
> deferred scan count rather than reducing it. This leads to excessive
> reclaim of the slab caches and kswapd can trash the caches long
> after the memory pressure has gone away...
> 

... yet if kswapd runs without pre-existing deferred work, that's
precisely what it does. next_deferred is set to remaining scan_count and
that is added back to the shrinker struct. So should kswapd generally
defer work or not? If the answer is sometimes, then please add a comment
to the next_deferred assignment to explain when/why.

> > For example, suppose we start this cycle with a large scan_count and
> > ->scan_objects() returned SHRINK_STOP before doing much work. In that
> > scenario, it looks like whether ->nr_deferred is 0 or not is the only
> > thing that determines whether we defer the entire remaining scan_count
> > or just what is left from the previous ->nr_deferred. The existing code
> > appears to consistently factor in what is left from the current scan
> > with the previous deferred count. Hm?
> 
> If kswapd doesn't have any deferred work, then it's largely no
> different in behaviour to direct reclaim. If it has no deferred
> work, then the shrinker is not getting stopped early in direct
> reclaim, so it's unlikely that kswapd is going to get stopped early,
> either....
> 

Then perhaps the logic could be simplified to explicitly not defer from
kswapd..?

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@...morbit.com
>