[08/26] mm: directed shrinker work deferral

Commit Message

Dave Chinner Oct. 9, 2019, 3:21 a.m. UTC

From: Dave Chinner <[e-mail]>

Introduce a mechanism for ->count_objects() to indicate to the
shrinker infrastructure that the reclaim context will not allow
scanning work to be done and so the work it decides is necessary
needs to be deferred.

This simplifies the code by separating out the accounting of
deferred work from the actual doing of the work, and allows better
decisions to be made by the shrinekr control logic on what action it
can take.

Signed-off-by: Dave Chinner <[e-mail]>
---
 include/linux/shrinker.h | 7 +++++++
 mm/vmscan.c              | 8 ++++++++
 2 files changed, 15 insertions(+)

Comments

Christoph Hellwig Oct. 14, 2019, 8:46 a.m. UTC | #1

On Wed, Oct 09, 2019 at 02:21:06PM +1100, Dave Chinner wrote:
> From: Dave Chinner <[e-mail]>
> 
> Introduce a mechanism for ->count_objects() to indicate to the
> shrinker infrastructure that the reclaim context will not allow
> scanning work to be done and so the work it decides is necessary
> needs to be deferred.
> 
> This simplifies the code by separating out the accounting of
> deferred work from the actual doing of the work, and allows better
> decisions to be made by the shrinekr control logic on what action it
> can take.

I hate all this boilerplate code in the scanners.  Can't we just add
a a required_gfp_mask field to struct shrinker and lift the pattern
to common code?

Brian Foster Oct. 14, 2019, 1:06 p.m. UTC | #2

On Mon, Oct 14, 2019 at 01:46:04AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 09, 2019 at 02:21:06PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <[e-mail]>
> > 
> > Introduce a mechanism for ->count_objects() to indicate to the
> > shrinker infrastructure that the reclaim context will not allow
> > scanning work to be done and so the work it decides is necessary
> > needs to be deferred.
> > 
> > This simplifies the code by separating out the accounting of
> > deferred work from the actual doing of the work, and allows better
> > decisions to be made by the shrinekr control logic on what action it
> > can take.
> 
> I hate all this boilerplate code in the scanners.  Can't we just add
> a a required_gfp_mask field to struct shrinker and lift the pattern
> to common code?

FWIW, I suggested something similar on the RFC as well. Based on where
that discussion ended up, however, I'm also kind of wondering why we
wouldn't move towards infrastructure that supports the more granular
per-item deferred tracking that is the supposed end goal (and at the
same time avoid shifting this logic back and forth between the count
callback and the scan/reclaim callback)..?

Brian

Dave Chinner Oct. 18, 2019, 7:59 a.m. UTC | #3

On Mon, Oct 14, 2019 at 01:46:04AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 09, 2019 at 02:21:06PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <[e-mail]>
> > 
> > Introduce a mechanism for ->count_objects() to indicate to the
> > shrinker infrastructure that the reclaim context will not allow
> > scanning work to be done and so the work it decides is necessary
> > needs to be deferred.
> > 
> > This simplifies the code by separating out the accounting of
> > deferred work from the actual doing of the work, and allows better
> > decisions to be made by the shrinekr control logic on what action it
> > can take.
> 
> I hate all this boilerplate code in the scanners.  Can't we just add
> a a required_gfp_mask field to struct shrinker and lift the pattern
> to common code?

Because the deferral isn't intended to support just deferring work
from GFP_NOFS reclaim context.

e.g. i915_gem_shrinker_scan() stops the shrinker if it can't get a
lock to avoid recursion deadlocks. There's several other shrinkers
that have similar logic that punts work to avoid deadlocks, and so
they could also use deferred work to punt it kswapd....

i.e. while I currently use it for GFP_NOFS deferal, that's no the
only reason for defering work...

Cheers,

Dave.

Patch

diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 0f80123650e2..3405c39ab92c 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -31,6 +31,13 @@  struct shrink_control {
 
 	/* current memcg being shrunk (for memcg aware shrinkers) */
 	struct mem_cgroup *memcg;
+
+	/*
+	 * set by ->count_objects if reclaim context prevents reclaim from
+	 * occurring. This allows the shrinker to immediately defer all the
+	 * work and not even attempt to scan the cache.
+	 */
+	bool defer_work;
 };
 
 #define SHRINK_STOP (~0UL)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index c6659bb758a4..1445bc7578c0 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -535,6 +535,13 @@  static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
 				   freeable, delta, total_scan, priority);
 
+	/*
+	 * If the shrinker can't run (e.g. due to gfp_mask constraints), then
+	 * defer the work to a context that can scan the cache.
+	 */
+	if (shrinkctl->defer_work)
+		goto done;
+
 	/*
 	 * Normally, we should not scan less than batch_size objects in one
 	 * pass to avoid too frequent shrinker calls, but if the slab has less
@@ -569,6 +576,7 @@  static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 		cond_resched();
 	}
 
+done:
 	if (next_deferred >= scanned)
 		next_deferred -= scanned;
 	else