* [PATCH 01/24] mm: directed shrinker work deferral
2019-08-01 2:17 [RFC] [PATCH 00/24] mm, xfs: non-blocking inode reclaim Dave Chinner
@ 2019-08-01 2:17 ` Dave Chinner
2019-08-02 15:27 ` Brian Foster
2019-08-01 2:17 ` [PATCH 02/24] shrinkers: use will_defer for GFP_NOFS sensitive shrinkers Dave Chinner
` (23 subsequent siblings)
24 siblings, 1 reply; 87+ messages in thread
From: Dave Chinner @ 2019-08-01 2:17 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-mm, linux-fsdevel
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
include/linux/shrinker.h | 7 +++++++
mm/vmscan.c | 8 ++++++++
2 files changed, 15 insertions(+)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 9443cafd1969..af78c475fc32 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 will_defer;
};
#define SHRINK_STOP (~0UL)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 44df66a98f2a..ae3035fe94bc 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -541,6 +541,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->will_defer)
+ 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
@@ -575,6 +582,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
cond_resched();
}
+done:
if (next_deferred >= scanned)
next_deferred -= scanned;
else
--
2.22.0
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 01/24] mm: directed shrinker work deferral
2019-08-01 2:17 ` [PATCH 01/24] mm: directed shrinker work deferral Dave Chinner
@ 2019-08-02 15:27 ` Brian Foster
2019-08-04 1:49 ` Dave Chinner
0 siblings, 1 reply; 87+ messages in thread
From: Brian Foster @ 2019-08-02 15:27 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Thu, Aug 01, 2019 at 12:17:29PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
> include/linux/shrinker.h | 7 +++++++
> mm/vmscan.c | 8 ++++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> index 9443cafd1969..af78c475fc32 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 will_defer;
Functionality wise this seems fairly straightforward. FWIW, I find the
'will_defer' name a little confusing because it implies to me that the
shrinker is telling the caller about something it would do if called as
opposed to explicitly telling the caller to defer. I'd just call it
'defer' I guess, but that's just my .02. ;P
> };
>
> #define SHRINK_STOP (~0UL)
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 44df66a98f2a..ae3035fe94bc 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -541,6 +541,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->will_defer)
> + goto done;
> +
Who's responsible for clearing the flag? Perhaps we should do so here
once it's acted upon since we don't call into the shrinker again?
Note that I see this structure is reinitialized on every iteration in
the caller, but there already is the SHRINK_EMPTY case where we call
back into do_shrink_slab(). Granted the deferred state likely hasn't
changed, but the fact that we'd call back into the count callback to set
it again implies the logic could be a bit more explicit, particularly if
this will eventually be used for more dynamic shrinker state that might
change call to call (i.e., object dirty state, etc.).
BTW, do we need to care about the ->nr_cached_objects() call from the
generic superblock shrinker (super_cache_scan())?
Brian
> /*
> * 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
> @@ -575,6 +582,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> cond_resched();
> }
>
> +done:
> if (next_deferred >= scanned)
> next_deferred -= scanned;
> else
> --
> 2.22.0
>
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 01/24] mm: directed shrinker work deferral
2019-08-02 15:27 ` Brian Foster
@ 2019-08-04 1:49 ` Dave Chinner
2019-08-05 17:42 ` Brian Foster
0 siblings, 1 reply; 87+ messages in thread
From: Dave Chinner @ 2019-08-04 1:49 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Fri, Aug 02, 2019 at 11:27:09AM -0400, Brian Foster wrote:
> On Thu, Aug 01, 2019 at 12:17:29PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > 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 <dchinner@redhat.com>
> > ---
> > include/linux/shrinker.h | 7 +++++++
> > mm/vmscan.c | 8 ++++++++
> > 2 files changed, 15 insertions(+)
> >
> > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> > index 9443cafd1969..af78c475fc32 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 will_defer;
>
> Functionality wise this seems fairly straightforward. FWIW, I find the
> 'will_defer' name a little confusing because it implies to me that the
> shrinker is telling the caller about something it would do if called as
> opposed to explicitly telling the caller to defer. I'd just call it
> 'defer' I guess, but that's just my .02. ;P
Ok, I'll change it to something like "defer_work" or "defer_scan"
here.
> > };
> >
> > #define SHRINK_STOP (~0UL)
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 44df66a98f2a..ae3035fe94bc 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -541,6 +541,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->will_defer)
> > + goto done;
> > +
>
> Who's responsible for clearing the flag? Perhaps we should do so here
> once it's acted upon since we don't call into the shrinker again?
Each shrinker invocation has it's own shrink_control context - they
are not shared between shrinkers - the higher level is responsible
for setting up the control state of each individual shrinker
invocation...
> Note that I see this structure is reinitialized on every iteration in
> the caller, but there already is the SHRINK_EMPTY case where we call
> back into do_shrink_slab().
.... because there is external state tracking in memcgs that
determine what shrinkers get run. See shrink_slab_memcg().
i.e. The SHRINK_EMPTY return value is a special hack for memcg
shrinkers so it can track whether there are freeable objects in the
cache externally to try to avoid calling into shrinkers where no
work can be done. Think about having hundreds of shrinkers and
hundreds of memcgs...
Anyway, the tracking of the freeable bit is racy, so the
SHRINK_EMPTY hack where it clears the bit and calls back into the
shrinker is handling the case where objects were freed between the
shrinker running and shrink_slab_memcg() clearing the freeable bit
from the slab. Hence it has to call back into the shrinker again -
if it gets anything other than SHRINK_EMPTY returned, then it will
set the bit again.
In reality, SHRINK_EMPTY and deferring work are mutually exclusive.
Work only gets deferred when there's work that can be done and in
that case SHRINK_EMPTY will not be returned - a value of "0 freed
objects" will be returned when we defer work. So if the first call
returns SHRINK_EMPTY, the "defer" state has not been touched and
so doesn't require resetting to zero here.
> Granted the deferred state likely hasn't
> changed, but the fact that we'd call back into the count callback to set
> it again implies the logic could be a bit more explicit, particularly if
> this will eventually be used for more dynamic shrinker state that might
> change call to call (i.e., object dirty state, etc.).
>
> BTW, do we need to care about the ->nr_cached_objects() call from the
> generic superblock shrinker (super_cache_scan())?
No, and we never had to because it is inside the superblock shrinker
and the superblock shrinker does the GFP_NOFS context checks.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 01/24] mm: directed shrinker work deferral
2019-08-04 1:49 ` Dave Chinner
@ 2019-08-05 17:42 ` Brian Foster
2019-08-05 23:43 ` Dave Chinner
0 siblings, 1 reply; 87+ messages in thread
From: Brian Foster @ 2019-08-05 17:42 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Sun, Aug 04, 2019 at 11:49:30AM +1000, Dave Chinner wrote:
> On Fri, Aug 02, 2019 at 11:27:09AM -0400, Brian Foster wrote:
> > On Thu, Aug 01, 2019 at 12:17:29PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > >
> > > 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 <dchinner@redhat.com>
> > > ---
> > > include/linux/shrinker.h | 7 +++++++
> > > mm/vmscan.c | 8 ++++++++
> > > 2 files changed, 15 insertions(+)
> > >
> > > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> > > index 9443cafd1969..af78c475fc32 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 will_defer;
> >
> > Functionality wise this seems fairly straightforward. FWIW, I find the
> > 'will_defer' name a little confusing because it implies to me that the
> > shrinker is telling the caller about something it would do if called as
> > opposed to explicitly telling the caller to defer. I'd just call it
> > 'defer' I guess, but that's just my .02. ;P
>
> Ok, I'll change it to something like "defer_work" or "defer_scan"
> here.
>
Either sounds better to me, thanks.
> > > };
> > >
> > > #define SHRINK_STOP (~0UL)
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 44df66a98f2a..ae3035fe94bc 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -541,6 +541,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->will_defer)
> > > + goto done;
> > > +
> >
> > Who's responsible for clearing the flag? Perhaps we should do so here
> > once it's acted upon since we don't call into the shrinker again?
>
> Each shrinker invocation has it's own shrink_control context - they
> are not shared between shrinkers - the higher level is responsible
> for setting up the control state of each individual shrinker
> invocation...
>
Yes, but more specifically, it appears to me that each level is
responsible for setting up control state managed by that level. E.g.,
shrink_slab_memcg() initializes the unchanging state per iteration and
do_shrink_slab() (re)sets the scan state prior to ->scan_objects().
> > Note that I see this structure is reinitialized on every iteration in
> > the caller, but there already is the SHRINK_EMPTY case where we call
> > back into do_shrink_slab().
>
> .... because there is external state tracking in memcgs that
> determine what shrinkers get run. See shrink_slab_memcg().
>
> i.e. The SHRINK_EMPTY return value is a special hack for memcg
> shrinkers so it can track whether there are freeable objects in the
> cache externally to try to avoid calling into shrinkers where no
> work can be done. Think about having hundreds of shrinkers and
> hundreds of memcgs...
>
> Anyway, the tracking of the freeable bit is racy, so the
> SHRINK_EMPTY hack where it clears the bit and calls back into the
> shrinker is handling the case where objects were freed between the
> shrinker running and shrink_slab_memcg() clearing the freeable bit
> from the slab. Hence it has to call back into the shrinker again -
> if it gets anything other than SHRINK_EMPTY returned, then it will
> set the bit again.
>
Yeah, I grokked most of that from the code. The current implementation
looks fine to me, but I could easily see how changes in the higher level
do_shrink_slab() caller(s) or lower level shrinker callbacks could
quietly break this in the future. IOW, once this code hits the tree any
shrinker across the kernel is free to try and defer slab reclaim work
for any reason.
> In reality, SHRINK_EMPTY and deferring work are mutually exclusive.
> Work only gets deferred when there's work that can be done and in
> that case SHRINK_EMPTY will not be returned - a value of "0 freed
> objects" will be returned when we defer work. So if the first call
> returns SHRINK_EMPTY, the "defer" state has not been touched and
> so doesn't require resetting to zero here.
>
Yep. The high level semantics make sense, but note that that the generic
superblock shrinker can now set ->will_defer true and return
SHRINK_EMPTY so that last bit about defer state not being touched is not
technically true.
> > Granted the deferred state likely hasn't
> > changed, but the fact that we'd call back into the count callback to set
> > it again implies the logic could be a bit more explicit, particularly if
> > this will eventually be used for more dynamic shrinker state that might
> > change call to call (i.e., object dirty state, etc.).
> >
> > BTW, do we need to care about the ->nr_cached_objects() call from the
> > generic superblock shrinker (super_cache_scan())?
>
> No, and we never had to because it is inside the superblock shrinker
> and the superblock shrinker does the GFP_NOFS context checks.
>
Ok. Though tbh this topic has me wondering whether a shrink_control
boolean is the right approach here. Do you envision ->will_defer being
used for anything other than allocation context restrictions? If not,
perhaps we should do something like optionally set alloc flags required
for direct scanning in the struct shrinker itself and let the core
shrinker code decide when to defer to kswapd based on the shrink_control
flags and the current shrinker. That way an arbitrary shrinker can't
muck around with core behavior in unintended ways. Hm?
Brian
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 01/24] mm: directed shrinker work deferral
2019-08-05 17:42 ` Brian Foster
@ 2019-08-05 23:43 ` Dave Chinner
2019-08-06 12:27 ` Brian Foster
0 siblings, 1 reply; 87+ messages in thread
From: Dave Chinner @ 2019-08-05 23:43 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Mon, Aug 05, 2019 at 01:42:26PM -0400, Brian Foster wrote:
> On Sun, Aug 04, 2019 at 11:49:30AM +1000, Dave Chinner wrote:
> > On Fri, Aug 02, 2019 at 11:27:09AM -0400, Brian Foster wrote:
> > > On Thu, Aug 01, 2019 at 12:17:29PM +1000, Dave Chinner wrote:
> > > > };
> > > >
> > > > #define SHRINK_STOP (~0UL)
> > > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > > index 44df66a98f2a..ae3035fe94bc 100644
> > > > --- a/mm/vmscan.c
> > > > +++ b/mm/vmscan.c
> > > > @@ -541,6 +541,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->will_defer)
> > > > + goto done;
> > > > +
> > >
> > > Who's responsible for clearing the flag? Perhaps we should do so here
> > > once it's acted upon since we don't call into the shrinker again?
> >
> > Each shrinker invocation has it's own shrink_control context - they
> > are not shared between shrinkers - the higher level is responsible
> > for setting up the control state of each individual shrinker
> > invocation...
> >
>
> Yes, but more specifically, it appears to me that each level is
> responsible for setting up control state managed by that level. E.g.,
> shrink_slab_memcg() initializes the unchanging state per iteration and
> do_shrink_slab() (re)sets the scan state prior to ->scan_objects().
do_shrink_slab() is responsible for iterating the scan in
shrinker->batch sizes, that's all it's doing there. We have to do
some accounting work from scan to scan. However, if ->will_defer is
set, we skip that entire loop, so it's largely irrelevant IMO.
> > > Granted the deferred state likely hasn't
> > > changed, but the fact that we'd call back into the count callback to set
> > > it again implies the logic could be a bit more explicit, particularly if
> > > this will eventually be used for more dynamic shrinker state that might
> > > change call to call (i.e., object dirty state, etc.).
> > >
> > > BTW, do we need to care about the ->nr_cached_objects() call from the
> > > generic superblock shrinker (super_cache_scan())?
> >
> > No, and we never had to because it is inside the superblock shrinker
> > and the superblock shrinker does the GFP_NOFS context checks.
> >
>
> Ok. Though tbh this topic has me wondering whether a shrink_control
> boolean is the right approach here. Do you envision ->will_defer being
> used for anything other than allocation context restrictions? If not,
Not at this point. If there are other control flags needed, we can
ad them in future - I don't like the idea of having a single control
flag mean different things in different contexts.
> perhaps we should do something like optionally set alloc flags required
> for direct scanning in the struct shrinker itself and let the core
> shrinker code decide when to defer to kswapd based on the shrink_control
> flags and the current shrinker. That way an arbitrary shrinker can't
> muck around with core behavior in unintended ways. Hm?
Arbitrary shrinkers can't "muck about" with the core behaviour any
more than they already could with this code. If you want to screw up
the core reclaim by always returning SHRINK_STOP to ->scan_objects
instead of doing work, then there is nothing stopping you from doing
that right now. Formalising there work deferral into a flag in the
shrink_control doesn't really change that at all, adn as such I
don't see any need for over-complicating the mechanism here....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 01/24] mm: directed shrinker work deferral
2019-08-05 23:43 ` Dave Chinner
@ 2019-08-06 12:27 ` Brian Foster
2019-08-06 22:22 ` Dave Chinner
0 siblings, 1 reply; 87+ messages in thread
From: Brian Foster @ 2019-08-06 12:27 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Tue, Aug 06, 2019 at 09:43:18AM +1000, Dave Chinner wrote:
> On Mon, Aug 05, 2019 at 01:42:26PM -0400, Brian Foster wrote:
> > On Sun, Aug 04, 2019 at 11:49:30AM +1000, Dave Chinner wrote:
> > > On Fri, Aug 02, 2019 at 11:27:09AM -0400, Brian Foster wrote:
> > > > On Thu, Aug 01, 2019 at 12:17:29PM +1000, Dave Chinner wrote:
> > > > > };
> > > > >
> > > > > #define SHRINK_STOP (~0UL)
> > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > > > index 44df66a98f2a..ae3035fe94bc 100644
> > > > > --- a/mm/vmscan.c
> > > > > +++ b/mm/vmscan.c
> > > > > @@ -541,6 +541,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->will_defer)
> > > > > + goto done;
> > > > > +
> > > >
> > > > Who's responsible for clearing the flag? Perhaps we should do so here
> > > > once it's acted upon since we don't call into the shrinker again?
> > >
> > > Each shrinker invocation has it's own shrink_control context - they
> > > are not shared between shrinkers - the higher level is responsible
> > > for setting up the control state of each individual shrinker
> > > invocation...
> > >
> >
> > Yes, but more specifically, it appears to me that each level is
> > responsible for setting up control state managed by that level. E.g.,
> > shrink_slab_memcg() initializes the unchanging state per iteration and
> > do_shrink_slab() (re)sets the scan state prior to ->scan_objects().
>
> do_shrink_slab() is responsible for iterating the scan in
> shrinker->batch sizes, that's all it's doing there. We have to do
> some accounting work from scan to scan. However, if ->will_defer is
> set, we skip that entire loop, so it's largely irrelevant IMO.
>
The point is very simply that there are scenarios where ->will_defer
might be true or might be false on do_shrink_slab() entry and I'm just
noting it as a potential landmine. It's not a bug in the current code
from what I can tell. I can't imagine why we wouldn't just reset the
flag prior to the ->count_objects() call, but alas I'm not a maintainer
of this code so I'll leave it to other reviewers/maintainers at this
point..
> > > > Granted the deferred state likely hasn't
> > > > changed, but the fact that we'd call back into the count callback to set
> > > > it again implies the logic could be a bit more explicit, particularly if
> > > > this will eventually be used for more dynamic shrinker state that might
> > > > change call to call (i.e., object dirty state, etc.).
> > > >
> > > > BTW, do we need to care about the ->nr_cached_objects() call from the
> > > > generic superblock shrinker (super_cache_scan())?
> > >
> > > No, and we never had to because it is inside the superblock shrinker
> > > and the superblock shrinker does the GFP_NOFS context checks.
> > >
> >
> > Ok. Though tbh this topic has me wondering whether a shrink_control
> > boolean is the right approach here. Do you envision ->will_defer being
> > used for anything other than allocation context restrictions? If not,
>
> Not at this point. If there are other control flags needed, we can
> ad them in future - I don't like the idea of having a single control
> flag mean different things in different contexts.
>
I don't think we're talking about the same thing here..
> > perhaps we should do something like optionally set alloc flags required
> > for direct scanning in the struct shrinker itself and let the core
> > shrinker code decide when to defer to kswapd based on the shrink_control
> > flags and the current shrinker. That way an arbitrary shrinker can't
> > muck around with core behavior in unintended ways. Hm?
>
> Arbitrary shrinkers can't "muck about" with the core behaviour any
> more than they already could with this code. If you want to screw up
> the core reclaim by always returning SHRINK_STOP to ->scan_objects
> instead of doing work, then there is nothing stopping you from doing
> that right now. Formalising there work deferral into a flag in the
> shrink_control doesn't really change that at all, adn as such I
> don't see any need for over-complicating the mechanism here....
>
If you add a generic "defer work" knob to the shrinker mechanism, but
only process it as an "allocation context" check, I expect it could be
easily misused. For example, some shrinkers may decide to set the the
flag dynamically based on in-core state. This will work when called from
some contexts but not from others (unrelated to allocation context),
which is confusing. Therefore, what I'm saying is that if the only
current use case is to defer work from shrinkers that currently skip
work due to allocation context restraints, this might be better codified
with something like the appended (untested) example patch. This may or
may not be a preferable interface to the flag, but it's certainly not an
overcomplication...
Brian
--- 8< ---
diff --git a/fs/super.c b/fs/super.c
index 113c58f19425..4e05ed9d6154 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -69,13 +69,6 @@ static unsigned long super_cache_scan(struct shrinker *shrink,
sb = container_of(shrink, struct super_block, s_shrink);
- /*
- * Deadlock avoidance. We may hold various FS locks, and we don't want
- * to recurse into the FS that called us in clear_inode() and friends..
- */
- if (!(sc->gfp_mask & __GFP_FS))
- return SHRINK_STOP;
-
if (!trylock_super(sb))
return SHRINK_STOP;
@@ -264,6 +257,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
s->s_shrink.count_objects = super_cache_count;
s->s_shrink.batch = 1024;
s->s_shrink.flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE;
+ s->s_shrink.direct_mask = __GFP_FS;
if (prealloc_shrinker(&s->s_shrink))
goto fail;
if (list_lru_init_memcg(&s->s_dentry_lru, &s->s_shrink))
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 9443cafd1969..e94e4edf7f1e 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -75,6 +75,8 @@ struct shrinker {
#endif
/* objs pending delete, per node */
atomic_long_t *nr_deferred;
+
+ gfp_t direct_mask;
};
#define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 44df66a98f2a..fb339399e26a 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -541,6 +541,15 @@ 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 (shrinker->direct_mask &&
+ ((shrinkctl->gfp_mask & shrinker->direct_mask) !=
+ shrinker->direct_mask))
+ 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
@@ -575,6 +584,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
cond_resched();
}
+done:
if (next_deferred >= scanned)
next_deferred -= scanned;
else
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 01/24] mm: directed shrinker work deferral
2019-08-06 12:27 ` Brian Foster
@ 2019-08-06 22:22 ` Dave Chinner
2019-08-07 11:13 ` Brian Foster
0 siblings, 1 reply; 87+ messages in thread
From: Dave Chinner @ 2019-08-06 22:22 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Tue, Aug 06, 2019 at 08:27:54AM -0400, Brian Foster wrote:
> If you add a generic "defer work" knob to the shrinker mechanism, but
> only process it as an "allocation context" check, I expect it could be
> easily misused. For example, some shrinkers may decide to set the the
> flag dynamically based on in-core state.
Which is already the case. e.g. There are shrinkers that don't do
anything because a try-lock fails. I haven't attempted to change
them, but they are a clear example of how even ->scan_object to
->scan_object the shrinker context can change.
> This will work when called from
> some contexts but not from others (unrelated to allocation context),
> which is confusing. Therefore, what I'm saying is that if the only
> current use case is to defer work from shrinkers that currently skip
> work due to allocation context restraints, this might be better codified
> with something like the appended (untested) example patch. This may or
> may not be a preferable interface to the flag, but it's certainly not an
> overcomplication...
I don't think this is the right way to go.
I want the filesystem shrinkers to become entirely non-blocking so
that we can dynamically decide on an object-by-object basis whether
we can reclaim the object in GFP_NOFS context.
That is, a clean XFS inode that requires no special cleanup can be
reclaimed even in GFP_NOFS context. The problem we have is that
dentry reclaim can drop the last reference to an inode, causing
inactivation and hence modification. However, if it's only going to
move to the inode LRU and not evict the inode, we can reclaim that
dentry. Similarly for inodes - if evicting the inode is not going to
block or modify the inode, we can reclaim the inode even under
GFP_NOFS constraints. And the same for XFS indoes - it if's clean
we can reclaim it, GFP_NOFS context or not.
IMO, that's the direction we need to be heading in, and in those
cases the "deferred work" tends towards a count of objects we could
not reclaim during the scan because they require blocking work to be
done. i.e. deferred work is a boolean now because the GFP_NOFS
decision is boolean, but it's lays the ground work for deferred work
to be integrated at a much finer-grained level in the shrinker
scanning routines in future...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 01/24] mm: directed shrinker work deferral
2019-08-06 22:22 ` Dave Chinner
@ 2019-08-07 11:13 ` Brian Foster
0 siblings, 0 replies; 87+ messages in thread
From: Brian Foster @ 2019-08-07 11:13 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Wed, Aug 07, 2019 at 08:22:20AM +1000, Dave Chinner wrote:
> On Tue, Aug 06, 2019 at 08:27:54AM -0400, Brian Foster wrote:
> > If you add a generic "defer work" knob to the shrinker mechanism, but
> > only process it as an "allocation context" check, I expect it could be
> > easily misused. For example, some shrinkers may decide to set the the
> > flag dynamically based on in-core state.
>
> Which is already the case. e.g. There are shrinkers that don't do
> anything because a try-lock fails. I haven't attempted to change
> them, but they are a clear example of how even ->scan_object to
> ->scan_object the shrinker context can change.
>
That's a similar point to what I'm trying to make wrt to
->count_objects() and the new defer state..
> > This will work when called from
> > some contexts but not from others (unrelated to allocation context),
> > which is confusing. Therefore, what I'm saying is that if the only
> > current use case is to defer work from shrinkers that currently skip
> > work due to allocation context restraints, this might be better codified
> > with something like the appended (untested) example patch. This may or
> > may not be a preferable interface to the flag, but it's certainly not an
> > overcomplication...
>
> I don't think this is the right way to go.
>
> I want the filesystem shrinkers to become entirely non-blocking so
> that we can dynamically decide on an object-by-object basis whether
> we can reclaim the object in GFP_NOFS context.
>
This is why I was asking about whether/how you envisioned the defer flag
looking in the future. Though I think this is somewhat orthogonal to the
discussion between having a bool or internal alloc mask set, because
both are of the same granularity and would need to change to operate on
a per objects basis.
> That is, a clean XFS inode that requires no special cleanup can be
> reclaimed even in GFP_NOFS context. The problem we have is that
> dentry reclaim can drop the last reference to an inode, causing
> inactivation and hence modification. However, if it's only going to
> move to the inode LRU and not evict the inode, we can reclaim that
> dentry. Similarly for inodes - if evicting the inode is not going to
> block or modify the inode, we can reclaim the inode even under
> GFP_NOFS constraints. And the same for XFS indoes - it if's clean
> we can reclaim it, GFP_NOFS context or not.
>
> IMO, that's the direction we need to be heading in, and in those
> cases the "deferred work" tends towards a count of objects we could
> not reclaim during the scan because they require blocking work to be
> done. i.e. deferred work is a boolean now because the GFP_NOFS
> decision is boolean, but it's lays the ground work for deferred work
> to be integrated at a much finer-grained level in the shrinker
> scanning routines in future...
>
Yeah, this sounds more like it warrants a ->nr_deferred field or some
such, which could ultimately replace either of the previously discussed
options for deferring the entire instance. BTW, ISTM we could use that
kind of interface now for exactly what this patch is trying to
accomplish by changing those shrinkers with allocation context
restrictions to just transfer the entire scan count to the deferred
count in ->scan_objects() instead of setting the flag. That's somewhat
less churn in the long run because we aren't shifting the defer logic
back and forth between the count and scan callbacks unnecessarily. IMO,
it's also a cleaner interface than both options above.
Brian
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 02/24] shrinkers: use will_defer for GFP_NOFS sensitive shrinkers
2019-08-01 2:17 [RFC] [PATCH 00/24] mm, xfs: non-blocking inode reclaim Dave Chinner
2019-08-01 2:17 ` [PATCH 01/24] mm: directed shrinker work deferral Dave Chinner
@ 2019-08-01 2:17 ` Dave Chinner
2019-08-02 15:27 ` Brian Foster
2019-08-01 2:17 ` [PATCH 03/24] mm: factor shrinker work calculations Dave Chinner
` (22 subsequent siblings)
24 siblings, 1 reply; 87+ messages in thread
From: Dave Chinner @ 2019-08-01 2:17 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-mm, linux-fsdevel
From: Dave Chinner <dchinner@redhat.com>
For shrinkers that currently avoid scanning when called under
GFP_NOFS contexts, conver them to use the new ->will_defer flag
rather than checking and returning errors during scans.
This makes it very clear that these shrinkers are not doing any work
because of the context limitations, not because there is no work
that can be done.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
drivers/staging/android/ashmem.c | 8 ++++----
fs/gfs2/glock.c | 5 +++--
fs/gfs2/quota.c | 6 +++---
fs/nfs/dir.c | 6 +++---
fs/super.c | 6 +++---
fs/xfs/xfs_buf.c | 4 ++++
fs/xfs/xfs_qm.c | 11 ++++++++---
net/sunrpc/auth.c | 5 ++---
8 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 74d497d39c5a..fd9027dbd28c 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -438,10 +438,6 @@ ashmem_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
{
unsigned long freed = 0;
- /* We might recurse into filesystem code, so bail out if necessary */
- if (!(sc->gfp_mask & __GFP_FS))
- return SHRINK_STOP;
-
if (!mutex_trylock(&ashmem_mutex))
return -1;
@@ -478,6 +474,10 @@ ashmem_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
static unsigned long
ashmem_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
{
+ /* We might recurse into filesystem code, so bail out if necessary */
+ if (!(sc->gfp_mask & __GFP_FS))
+ sc->will_defer = true;
+
/*
* note that lru_count is count of pages on the lru, not a count of
* objects on the list. This means the scan function needs to return the
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index e23fb8b7b020..08c95172d0e5 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1517,14 +1517,15 @@ static long gfs2_scan_glock_lru(int nr)
static unsigned long gfs2_glock_shrink_scan(struct shrinker *shrink,
struct shrink_control *sc)
{
- if (!(sc->gfp_mask & __GFP_FS))
- return SHRINK_STOP;
return gfs2_scan_glock_lru(sc->nr_to_scan);
}
static unsigned long gfs2_glock_shrink_count(struct shrinker *shrink,
struct shrink_control *sc)
{
+ if (!(sc->gfp_mask & __GFP_FS))
+ sc->will_defer = true;
+
return vfs_pressure_ratio(atomic_read(&lru_count));
}
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 69c4b77f127b..d35beda906e8 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -166,9 +166,6 @@ static unsigned long gfs2_qd_shrink_scan(struct shrinker *shrink,
LIST_HEAD(dispose);
unsigned long freed;
- if (!(sc->gfp_mask & __GFP_FS))
- return SHRINK_STOP;
-
freed = list_lru_shrink_walk(&gfs2_qd_lru, sc,
gfs2_qd_isolate, &dispose);
@@ -180,6 +177,9 @@ static unsigned long gfs2_qd_shrink_scan(struct shrinker *shrink,
static unsigned long gfs2_qd_shrink_count(struct shrinker *shrink,
struct shrink_control *sc)
{
+ if (!(sc->gfp_mask & __GFP_FS))
+ sc->will_defer = true;
+
return vfs_pressure_ratio(list_lru_shrink_count(&gfs2_qd_lru, sc));
}
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 8d501093660f..73735ab1d623 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2202,10 +2202,7 @@ unsigned long
nfs_access_cache_scan(struct shrinker *shrink, struct shrink_control *sc)
{
int nr_to_scan = sc->nr_to_scan;
- gfp_t gfp_mask = sc->gfp_mask;
- if ((gfp_mask & GFP_KERNEL) != GFP_KERNEL)
- return SHRINK_STOP;
return nfs_do_access_cache_scan(nr_to_scan);
}
@@ -2213,6 +2210,9 @@ nfs_access_cache_scan(struct shrinker *shrink, struct shrink_control *sc)
unsigned long
nfs_access_cache_count(struct shrinker *shrink, struct shrink_control *sc)
{
+ if ((sc->gfp_mask & GFP_KERNEL) != GFP_KERNEL)
+ sc->will_defer = true;
+
return vfs_pressure_ratio(atomic_long_read(&nfs_access_nr_entries));
}
diff --git a/fs/super.c b/fs/super.c
index 113c58f19425..66dd2af6cfde 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -73,9 +73,6 @@ static unsigned long super_cache_scan(struct shrinker *shrink,
* Deadlock avoidance. We may hold various FS locks, and we don't want
* to recurse into the FS that called us in clear_inode() and friends..
*/
- if (!(sc->gfp_mask & __GFP_FS))
- return SHRINK_STOP;
-
if (!trylock_super(sb))
return SHRINK_STOP;
@@ -140,6 +137,9 @@ static unsigned long super_cache_count(struct shrinker *shrink,
return 0;
smp_rmb();
+ if (!(sc->gfp_mask & __GFP_FS))
+ sc->will_defer = true;
+
if (sb->s_op && sb->s_op->nr_cached_objects)
total_objects = sb->s_op->nr_cached_objects(sb, sc);
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index ca0849043f54..6e0f76532535 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1680,6 +1680,10 @@ xfs_buftarg_shrink_count(
{
struct xfs_buftarg *btp = container_of(shrink,
struct xfs_buftarg, bt_shrinker);
+
+ if (!(sc->gfp_mask & __GFP_FS))
+ sc->will_defer = true;
+
return list_lru_shrink_count(&btp->bt_lru, sc);
}
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 5e7a37f0cf84..13c842e8f13b 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -502,9 +502,6 @@ xfs_qm_shrink_scan(
unsigned long freed;
int error;
- if ((sc->gfp_mask & (__GFP_FS|__GFP_DIRECT_RECLAIM)) != (__GFP_FS|__GFP_DIRECT_RECLAIM))
- return 0;
-
INIT_LIST_HEAD(&isol.buffers);
INIT_LIST_HEAD(&isol.dispose);
@@ -534,6 +531,14 @@ xfs_qm_shrink_count(
struct xfs_quotainfo *qi = container_of(shrink,
struct xfs_quotainfo, qi_shrinker);
+ /*
+ * __GFP_DIRECT_RECLAIM is used here to avoid blocking kswapd
+ */
+ if ((sc->gfp_mask & (__GFP_FS|__GFP_DIRECT_RECLAIM)) !=
+ (__GFP_FS|__GFP_DIRECT_RECLAIM)) {
+ sc->will_defer = true;
+ }
+
return list_lru_shrink_count(&qi->qi_lru, sc);
}
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index cdb05b48de44..6babcbac4a00 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -527,9 +527,6 @@ static unsigned long
rpcauth_cache_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
{
- if ((sc->gfp_mask & GFP_KERNEL) != GFP_KERNEL)
- return SHRINK_STOP;
-
/* nothing left, don't come back */
if (list_empty(&cred_unused))
return SHRINK_STOP;
@@ -541,6 +538,8 @@ static unsigned long
rpcauth_cache_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
{
+ if ((sc->gfp_mask & GFP_KERNEL) != GFP_KERNEL)
+ sc->will_defer = true;
return number_cred_unused * sysctl_vfs_cache_pressure / 100;
}
--
2.22.0
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 02/24] shrinkers: use will_defer for GFP_NOFS sensitive shrinkers
2019-08-01 2:17 ` [PATCH 02/24] shrinkers: use will_defer for GFP_NOFS sensitive shrinkers Dave Chinner
@ 2019-08-02 15:27 ` Brian Foster
2019-08-04 1:50 ` Dave Chinner
0 siblings, 1 reply; 87+ messages in thread
From: Brian Foster @ 2019-08-02 15:27 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Thu, Aug 01, 2019 at 12:17:30PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> For shrinkers that currently avoid scanning when called under
> GFP_NOFS contexts, conver them to use the new ->will_defer flag
> rather than checking and returning errors during scans.
>
> This makes it very clear that these shrinkers are not doing any work
> because of the context limitations, not because there is no work
> that can be done.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> drivers/staging/android/ashmem.c | 8 ++++----
> fs/gfs2/glock.c | 5 +++--
> fs/gfs2/quota.c | 6 +++---
> fs/nfs/dir.c | 6 +++---
> fs/super.c | 6 +++---
> fs/xfs/xfs_buf.c | 4 ++++
> fs/xfs/xfs_qm.c | 11 ++++++++---
> net/sunrpc/auth.c | 5 ++---
> 8 files changed, 30 insertions(+), 21 deletions(-)
>
...
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ca0849043f54..6e0f76532535 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1680,6 +1680,10 @@ xfs_buftarg_shrink_count(
> {
> struct xfs_buftarg *btp = container_of(shrink,
> struct xfs_buftarg, bt_shrinker);
> +
> + if (!(sc->gfp_mask & __GFP_FS))
> + sc->will_defer = true;
> +
> return list_lru_shrink_count(&btp->bt_lru, sc);
> }
This hunk looks like a behavior change / bug fix..? The rest of the
patch converts existing logic to bail out of scans to use the new count
time defer mechanism. The change is probably fine, but I think we should
have a separate patch to introduce this behavior in the first place
(which BTW could be sent as a standalone patch and just picked up by
this on eventual rebase).
Brian
>
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 5e7a37f0cf84..13c842e8f13b 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -502,9 +502,6 @@ xfs_qm_shrink_scan(
> unsigned long freed;
> int error;
>
> - if ((sc->gfp_mask & (__GFP_FS|__GFP_DIRECT_RECLAIM)) != (__GFP_FS|__GFP_DIRECT_RECLAIM))
> - return 0;
> -
> INIT_LIST_HEAD(&isol.buffers);
> INIT_LIST_HEAD(&isol.dispose);
>
> @@ -534,6 +531,14 @@ xfs_qm_shrink_count(
> struct xfs_quotainfo *qi = container_of(shrink,
> struct xfs_quotainfo, qi_shrinker);
>
> + /*
> + * __GFP_DIRECT_RECLAIM is used here to avoid blocking kswapd
> + */
> + if ((sc->gfp_mask & (__GFP_FS|__GFP_DIRECT_RECLAIM)) !=
> + (__GFP_FS|__GFP_DIRECT_RECLAIM)) {
> + sc->will_defer = true;
> + }
> +
> return list_lru_shrink_count(&qi->qi_lru, sc);
> }
>
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index cdb05b48de44..6babcbac4a00 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -527,9 +527,6 @@ static unsigned long
> rpcauth_cache_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
>
> {
> - if ((sc->gfp_mask & GFP_KERNEL) != GFP_KERNEL)
> - return SHRINK_STOP;
> -
> /* nothing left, don't come back */
> if (list_empty(&cred_unused))
> return SHRINK_STOP;
> @@ -541,6 +538,8 @@ static unsigned long
> rpcauth_cache_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
>
> {
> + if ((sc->gfp_mask & GFP_KERNEL) != GFP_KERNEL)
> + sc->will_defer = true;
> return number_cred_unused * sysctl_vfs_cache_pressure / 100;
> }
>
> --
> 2.22.0
>
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 02/24] shrinkers: use will_defer for GFP_NOFS sensitive shrinkers
2019-08-02 15:27 ` Brian Foster
@ 2019-08-04 1:50 ` Dave Chinner
0 siblings, 0 replies; 87+ messages in thread
From: Dave Chinner @ 2019-08-04 1:50 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Fri, Aug 02, 2019 at 11:27:37AM -0400, Brian Foster wrote:
> On Thu, Aug 01, 2019 at 12:17:30PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > For shrinkers that currently avoid scanning when called under
> > GFP_NOFS contexts, conver them to use the new ->will_defer flag
> > rather than checking and returning errors during scans.
> >
> > This makes it very clear that these shrinkers are not doing any work
> > because of the context limitations, not because there is no work
> > that can be done.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> > drivers/staging/android/ashmem.c | 8 ++++----
> > fs/gfs2/glock.c | 5 +++--
> > fs/gfs2/quota.c | 6 +++---
> > fs/nfs/dir.c | 6 +++---
> > fs/super.c | 6 +++---
> > fs/xfs/xfs_buf.c | 4 ++++
> > fs/xfs/xfs_qm.c | 11 ++++++++---
> > net/sunrpc/auth.c | 5 ++---
> > 8 files changed, 30 insertions(+), 21 deletions(-)
> >
> ...
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index ca0849043f54..6e0f76532535 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -1680,6 +1680,10 @@ xfs_buftarg_shrink_count(
> > {
> > struct xfs_buftarg *btp = container_of(shrink,
> > struct xfs_buftarg, bt_shrinker);
> > +
> > + if (!(sc->gfp_mask & __GFP_FS))
> > + sc->will_defer = true;
> > +
> > return list_lru_shrink_count(&btp->bt_lru, sc);
> > }
>
> This hunk looks like a behavior change / bug fix..? The rest of the
Yeah, forgot to move that to the patch that fixes the accounting for
the xfs_buf cache later on in the series. Will fix.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 03/24] mm: factor shrinker work calculations
2019-08-01 2:17 [RFC] [PATCH 00/24] mm, xfs: non-blocking inode reclaim Dave Chinner
2019-08-01 2:17 ` [PATCH 01/24] mm: directed shrinker work deferral Dave Chinner
2019-08-01 2:17 ` [PATCH 02/24] shrinkers: use will_defer for GFP_NOFS sensitive shrinkers Dave Chinner
@ 2019-08-01 2:17 ` Dave Chinner
2019-08-02 15:08 ` Nikolay Borisov
2019-08-02 15:31 ` Brian Foster
2019-08-01 2:17 ` [PATCH 04/24] shrinker: defer work only to kswapd Dave Chinner
` (21 subsequent siblings)
24 siblings, 2 replies; 87+ messages in thread
From: Dave Chinner @ 2019-08-01 2:17 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-mm, linux-fsdevel
From: Dave Chinner <dchinner@redhat.com>
Start to clean up the shrinker code by factoring out the calculation
that determines how much work to do. This separates the calculation
from clamping and other adjustments that are done before the
shrinker work is run.
Also convert the calculation for the amount of work to be done to
use 64 bit logic so we don't have to keep jumping through hoops to
keep calculations within 32 bits on 32 bit systems.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mm/vmscan.c | 74 ++++++++++++++++++++++++++++++++++-------------------
1 file changed, 47 insertions(+), 27 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index ae3035fe94bc..b7472953b0e6 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -464,13 +464,45 @@ EXPORT_SYMBOL(unregister_shrinker);
#define SHRINK_BATCH 128
+/*
+ * Calculate the number of new objects to scan this time around. Return
+ * the work to be done. If there are freeable objects, return that number in
+ * @freeable_objects.
+ */
+static int64_t shrink_scan_count(struct shrink_control *shrinkctl,
+ struct shrinker *shrinker, int priority,
+ int64_t *freeable_objects)
+{
+ uint64_t delta;
+ uint64_t freeable;
+
+ freeable = shrinker->count_objects(shrinker, shrinkctl);
+ if (freeable == 0 || freeable == SHRINK_EMPTY)
+ return freeable;
+
+ if (shrinker->seeks) {
+ delta = freeable >> (priority - 2);
+ do_div(delta, shrinker->seeks);
+ } else {
+ /*
+ * These objects don't require any IO to create. Trim
+ * them aggressively under memory pressure to keep
+ * them from causing refetches in the IO caches.
+ */
+ delta = freeable / 2;
+ }
+
+ *freeable_objects = freeable;
+ return delta > 0 ? delta : 0;
+}
+
static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
struct shrinker *shrinker, int priority)
{
unsigned long freed = 0;
- unsigned long long delta;
long total_scan;
- long freeable;
+ int64_t freeable_objects = 0;
+ int64_t scan_count;
long nr;
long new_nr;
int nid = shrinkctl->nid;
@@ -481,9 +513,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
nid = 0;
- freeable = shrinker->count_objects(shrinker, shrinkctl);
- if (freeable == 0 || freeable == SHRINK_EMPTY)
- return freeable;
+ scan_count = shrink_scan_count(shrinkctl, shrinker, priority,
+ &freeable_objects);
+ if (scan_count == 0 || scan_count == SHRINK_EMPTY)
+ return scan_count;
/*
* copy the current shrinker scan count into a local variable
@@ -492,25 +525,11 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
*/
nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
- total_scan = nr;
- if (shrinker->seeks) {
- delta = freeable >> priority;
- delta *= 4;
- do_div(delta, shrinker->seeks);
- } else {
- /*
- * These objects don't require any IO to create. Trim
- * them aggressively under memory pressure to keep
- * them from causing refetches in the IO caches.
- */
- delta = freeable / 2;
- }
-
- total_scan += delta;
+ total_scan = nr + scan_count;
if (total_scan < 0) {
pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
shrinker->scan_objects, total_scan);
- total_scan = freeable;
+ total_scan = scan_count;
next_deferred = nr;
} else
next_deferred = total_scan;
@@ -527,19 +546,20 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
* Hence only allow the shrinker to scan the entire cache when
* a large delta change is calculated directly.
*/
- if (delta < freeable / 4)
- total_scan = min(total_scan, freeable / 2);
+ if (scan_count < freeable_objects / 4)
+ total_scan = min_t(long, total_scan, freeable_objects / 2);
/*
* Avoid risking looping forever due to too large nr value:
* never try to free more than twice the estimate number of
* freeable entries.
*/
- if (total_scan > freeable * 2)
- total_scan = freeable * 2;
+ if (total_scan > freeable_objects * 2)
+ total_scan = freeable_objects * 2;
trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
- freeable, delta, total_scan, priority);
+ freeable_objects, scan_count,
+ total_scan, priority);
/*
* If the shrinker can't run (e.g. due to gfp_mask constraints), then
@@ -564,7 +584,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
* possible.
*/
while (total_scan >= batch_size ||
- total_scan >= freeable) {
+ total_scan >= freeable_objects) {
unsigned long ret;
unsigned long nr_to_scan = min(batch_size, total_scan);
--
2.22.0
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 03/24] mm: factor shrinker work calculations
2019-08-01 2:17 ` [PATCH 03/24] mm: factor shrinker work calculations Dave Chinner
@ 2019-08-02 15:08 ` Nikolay Borisov
2019-08-04 2:05 ` Dave Chinner
2019-08-02 15:31 ` Brian Foster
1 sibling, 1 reply; 87+ messages in thread
From: Nikolay Borisov @ 2019-08-02 15:08 UTC (permalink / raw)
To: Dave Chinner, linux-xfs; +Cc: linux-mm, linux-fsdevel
On 1.08.19 г. 5:17 ч., Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Start to clean up the shrinker code by factoring out the calculation
> that determines how much work to do. This separates the calculation
> from clamping and other adjustments that are done before the
> shrinker work is run.
>
> Also convert the calculation for the amount of work to be done to
> use 64 bit logic so we don't have to keep jumping through hoops to
> keep calculations within 32 bits on 32 bit systems.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> mm/vmscan.c | 74 ++++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 47 insertions(+), 27 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index ae3035fe94bc..b7472953b0e6 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -464,13 +464,45 @@ EXPORT_SYMBOL(unregister_shrinker);
>
> #define SHRINK_BATCH 128
>
> +/*
> + * Calculate the number of new objects to scan this time around. Return
> + * the work to be done. If there are freeable objects, return that number in
> + * @freeable_objects.
> + */
> +static int64_t shrink_scan_count(struct shrink_control *shrinkctl,
> + struct shrinker *shrinker, int priority,
> + int64_t *freeable_objects)
nit: make the return parm definition also uin64_t, also we have u64 types.
> +{
> + uint64_t delta;
> + uint64_t freeable;
> +
> + freeable = shrinker->count_objects(shrinker, shrinkctl);
> + if (freeable == 0 || freeable == SHRINK_EMPTY)
> + return freeable;
> +
> + if (shrinker->seeks) {
> + delta = freeable >> (priority - 2);
> + do_div(delta, shrinker->seeks);
a comment about the reasoning behind this calculation would be nice.
> + } else {
> + /*
> + * These objects don't require any IO to create. Trim
> + * them aggressively under memory pressure to keep
> + * them from causing refetches in the IO caches.
> + */
> + delta = freeable / 2;
> + }
> +
> + *freeable_objects = freeable;
> + return delta > 0 ? delta : 0;
> +}
> +
> static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> struct shrinker *shrinker, int priority)
> {
> unsigned long freed = 0;
> - unsigned long long delta;
> long total_scan;
> - long freeable;
> + int64_t freeable_objects = 0;
> + int64_t scan_count;
why int and not uint64 ? We can never have negative object count, right?
> long nr;
> long new_nr;
> int nid = shrinkctl->nid;
> @@ -481,9 +513,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
> nid = 0;
>
> - freeable = shrinker->count_objects(shrinker, shrinkctl);
> - if (freeable == 0 || freeable == SHRINK_EMPTY)
> - return freeable;
> + scan_count = shrink_scan_count(shrinkctl, shrinker, priority,
> + &freeable_objects);
> + if (scan_count == 0 || scan_count == SHRINK_EMPTY)
> + return scan_count;
>
> /*
> * copy the current shrinker scan count into a local variable
> @@ -492,25 +525,11 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> */
> nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
>
> - total_scan = nr;
> - if (shrinker->seeks) {
> - delta = freeable >> priority;
> - delta *= 4;
> - do_div(delta, shrinker->seeks);
> - } else {
> - /*
> - * These objects don't require any IO to create. Trim
> - * them aggressively under memory pressure to keep
> - * them from causing refetches in the IO caches.
> - */
> - delta = freeable / 2;
> - }
> -
> - total_scan += delta;
> + total_scan = nr + scan_count;
> if (total_scan < 0) {
> pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
> shrinker->scan_objects, total_scan);
> - total_scan = freeable;
> + total_scan = scan_count;
> next_deferred = nr;
> } else
> next_deferred = total_scan;
> @@ -527,19 +546,20 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> * Hence only allow the shrinker to scan the entire cache when
> * a large delta change is calculated directly.
> */
> - if (delta < freeable / 4)
> - total_scan = min(total_scan, freeable / 2);
> + if (scan_count < freeable_objects / 4)
> + total_scan = min_t(long, total_scan, freeable_objects / 2);
>
> /*
> * Avoid risking looping forever due to too large nr value:
> * never try to free more than twice the estimate number of
> * freeable entries.
> */
> - if (total_scan > freeable * 2)
> - total_scan = freeable * 2;
> + if (total_scan > freeable_objects * 2)
> + total_scan = freeable_objects * 2;
>
> trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
> - freeable, delta, total_scan, priority);
> + freeable_objects, scan_count,
> + total_scan, priority);
>
> /*
> * If the shrinker can't run (e.g. due to gfp_mask constraints), then
> @@ -564,7 +584,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> * possible.
> */
> while (total_scan >= batch_size ||
> - total_scan >= freeable) {
> + total_scan >= freeable_objects) {
> unsigned long ret;
> unsigned long nr_to_scan = min(batch_size, total_scan);
>
>
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 03/24] mm: factor shrinker work calculations
2019-08-02 15:08 ` Nikolay Borisov
@ 2019-08-04 2:05 ` Dave Chinner
0 siblings, 0 replies; 87+ messages in thread
From: Dave Chinner @ 2019-08-04 2:05 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Fri, Aug 02, 2019 at 06:08:37PM +0300, Nikolay Borisov wrote:
>
>
> On 1.08.19 г. 5:17 ч., Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Start to clean up the shrinker code by factoring out the calculation
> > that determines how much work to do. This separates the calculation
> > from clamping and other adjustments that are done before the
> > shrinker work is run.
> >
> > Also convert the calculation for the amount of work to be done to
> > use 64 bit logic so we don't have to keep jumping through hoops to
> > keep calculations within 32 bits on 32 bit systems.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> > mm/vmscan.c | 74 ++++++++++++++++++++++++++++++++++-------------------
> > 1 file changed, 47 insertions(+), 27 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index ae3035fe94bc..b7472953b0e6 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -464,13 +464,45 @@ EXPORT_SYMBOL(unregister_shrinker);
> >
> > #define SHRINK_BATCH 128
> >
> > +/*
> > + * Calculate the number of new objects to scan this time around. Return
> > + * the work to be done. If there are freeable objects, return that number in
> > + * @freeable_objects.
> > + */
> > +static int64_t shrink_scan_count(struct shrink_control *shrinkctl,
> > + struct shrinker *shrinker, int priority,
> > + int64_t *freeable_objects)
>
> nit: make the return parm definition also uin64_t, also we have u64 types.
SHRINK_EMPTY is actually a negative number (-2), and it gets whacked
back into a signed long value in the caller. So returning a signed
integer is actually correct.
> > +{
> > + uint64_t delta;
> > + uint64_t freeable;
> > +
> > + freeable = shrinker->count_objects(shrinker, shrinkctl);
> > + if (freeable == 0 || freeable == SHRINK_EMPTY)
> > + return freeable;
> > +
> > + if (shrinker->seeks) {
> > + delta = freeable >> (priority - 2);
> > + do_div(delta, shrinker->seeks);
>
> a comment about the reasoning behind this calculation would be nice.
I'm just moving code here.
The reason for this calculation requires an awfully long description
that isn't actually appropriate here or in this patch set.
If there should be any comment describing how shrinker work biasing
should be configured, it needs to be in include/linux/shrinker.h
around the definition of DEFAULT_SEEKS and shrinker->seeks as this
code requires shrinker->seeks to be configured appropriately by the
code that registers the shrinker.
> > static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > struct shrinker *shrinker, int priority)
> > {
> > unsigned long freed = 0;
> > - unsigned long long delta;
> > long total_scan;
> > - long freeable;
> > + int64_t freeable_objects = 0;
> > + int64_t scan_count;
>
> why int and not uint64 ? We can never have negative object count, right?
SHRINK_STOP, SHRINK_EMPTY are negative numbers, and the higher level
interface uses longs, not unsigned longs. So we have to treat
numbers greater than LONG_MAX as invalid for object/scan counts.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 03/24] mm: factor shrinker work calculations
2019-08-01 2:17 ` [PATCH 03/24] mm: factor shrinker work calculations Dave Chinner
2019-08-02 15:08 ` Nikolay Borisov
@ 2019-08-02 15:31 ` Brian Foster
1 sibling, 0 replies; 87+ messages in thread
From: Brian Foster @ 2019-08-02 15:31 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Thu, Aug 01, 2019 at 12:17:31PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Start to clean up the shrinker code by factoring out the calculation
> that determines how much work to do. This separates the calculation
> from clamping and other adjustments that are done before the
> shrinker work is run.
>
> Also convert the calculation for the amount of work to be done to
> use 64 bit logic so we don't have to keep jumping through hoops to
> keep calculations within 32 bits on 32 bit systems.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> mm/vmscan.c | 74 ++++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 47 insertions(+), 27 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index ae3035fe94bc..b7472953b0e6 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -464,13 +464,45 @@ EXPORT_SYMBOL(unregister_shrinker);
>
> #define SHRINK_BATCH 128
>
> +/*
> + * Calculate the number of new objects to scan this time around. Return
> + * the work to be done. If there are freeable objects, return that number in
> + * @freeable_objects.
> + */
> +static int64_t shrink_scan_count(struct shrink_control *shrinkctl,
> + struct shrinker *shrinker, int priority,
> + int64_t *freeable_objects)
> +{
> + uint64_t delta;
> + uint64_t freeable;
> +
> + freeable = shrinker->count_objects(shrinker, shrinkctl);
> + if (freeable == 0 || freeable == SHRINK_EMPTY)
> + return freeable;
> +
> + if (shrinker->seeks) {
> + delta = freeable >> (priority - 2);
> + do_div(delta, shrinker->seeks);
> + } else {
> + /*
> + * These objects don't require any IO to create. Trim
> + * them aggressively under memory pressure to keep
> + * them from causing refetches in the IO caches.
> + */
> + delta = freeable / 2;
> + }
> +
> + *freeable_objects = freeable;
> + return delta > 0 ? delta : 0;
I see Nikolay had some similar comments but FWIW delta is unsigned so
I'm not sure the point of the > 0 check.
Brian
> +}
> +
> static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> struct shrinker *shrinker, int priority)
> {
> unsigned long freed = 0;
> - unsigned long long delta;
> long total_scan;
> - long freeable;
> + int64_t freeable_objects = 0;
> + int64_t scan_count;
> long nr;
> long new_nr;
> int nid = shrinkctl->nid;
> @@ -481,9 +513,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
> nid = 0;
>
> - freeable = shrinker->count_objects(shrinker, shrinkctl);
> - if (freeable == 0 || freeable == SHRINK_EMPTY)
> - return freeable;
> + scan_count = shrink_scan_count(shrinkctl, shrinker, priority,
> + &freeable_objects);
> + if (scan_count == 0 || scan_count == SHRINK_EMPTY)
> + return scan_count;
>
> /*
> * copy the current shrinker scan count into a local variable
> @@ -492,25 +525,11 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> */
> nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
>
> - total_scan = nr;
> - if (shrinker->seeks) {
> - delta = freeable >> priority;
> - delta *= 4;
> - do_div(delta, shrinker->seeks);
> - } else {
> - /*
> - * These objects don't require any IO to create. Trim
> - * them aggressively under memory pressure to keep
> - * them from causing refetches in the IO caches.
> - */
> - delta = freeable / 2;
> - }
> -
> - total_scan += delta;
> + total_scan = nr + scan_count;
> if (total_scan < 0) {
> pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
> shrinker->scan_objects, total_scan);
> - total_scan = freeable;
> + total_scan = scan_count;
Why the change from the (now) freeable_objects value to scan_count?
Brian
> next_deferred = nr;
> } else
> next_deferred = total_scan;
> @@ -527,19 +546,20 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> * Hence only allow the shrinker to scan the entire cache when
> * a large delta change is calculated directly.
> */
> - if (delta < freeable / 4)
> - total_scan = min(total_scan, freeable / 2);
> + if (scan_count < freeable_objects / 4)
> + total_scan = min_t(long, total_scan, freeable_objects / 2);
>
> /*
> * Avoid risking looping forever due to too large nr value:
> * never try to free more than twice the estimate number of
> * freeable entries.
> */
> - if (total_scan > freeable * 2)
> - total_scan = freeable * 2;
> + if (total_scan > freeable_objects * 2)
> + total_scan = freeable_objects * 2;
>
> trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
> - freeable, delta, total_scan, priority);
> + freeable_objects, scan_count,
> + total_scan, priority);
>
> /*
> * If the shrinker can't run (e.g. due to gfp_mask constraints), then
> @@ -564,7 +584,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> * possible.
> */
> while (total_scan >= batch_size ||
> - total_scan >= freeable) {
> + total_scan >= freeable_objects) {
> unsigned long ret;
> unsigned long nr_to_scan = min(batch_size, total_scan);
>
> --
> 2.22.0
>
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 04/24] shrinker: defer work only to kswapd
2019-08-01 2:17 [RFC] [PATCH 00/24] mm, xfs: non-blocking inode reclaim Dave Chinner
` (2 preceding siblings ...)
2019-08-01 2:17 ` [PATCH 03/24] mm: factor shrinker work calculations Dave Chinner
@ 2019-08-01 2:17 ` Dave Chinner
2019-08-02 15:34 ` Brian Foster
` (3 more replies)
2019-08-01 2:17 ` [PATCH 05/24] shrinker: clean up variable types and tracepoints Dave Chinner
` (20 subsequent siblings)
24 siblings, 4 replies; 87+ messages in thread
From: Dave Chinner @ 2019-08-01 2:17 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-mm, linux-fsdevel
From: Dave Chinner <dchinner@redhat.com>
Right now deferred work is picked up by whatever GFP_KERNEL context
reclaimer that wins the race to empty the node's deferred work
counter. However, if there are lots of direct reclaimers, that
work might be continually picked up by contexts taht can't do any
work and so the opportunities to do the work are missed by contexts
that could do them.
A further problem with the current code is that the deferred work
can be picked up by a random direct reclaimer, resulting in that
specific process having to do all the deferred reclaim work and
hence can take extremely long latencies if the reclaim work blocks
regularly. This is not good for direct reclaim fairness or for
minimising long tail latency events.
To avoid these problems, simply limit deferred work to kswapd
contexts. We know kswapd is a context that can always do reclaim
work, and hence deferring work to kswapd allows the deferred work to
be done in the background and not adversely affect any specific
process context doing direct reclaim.
The advantage of this is that amount of work to be done in direct
reclaim is now bound and predictable - it is entirely based on
the cache's freeable objects and the reclaim priority. hence all
direct reclaimers running at the same time should be doing
relatively equal amounts of work, thereby reducing the incidence of
long tail latencies due to uneven reclaim workloads.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mm/vmscan.c | 93 ++++++++++++++++++++++++++++-------------------------
1 file changed, 50 insertions(+), 43 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index b7472953b0e6..c583b4efb9bf 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -500,15 +500,15 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
struct shrinker *shrinker, int priority)
{
unsigned long freed = 0;
- long total_scan;
int64_t freeable_objects = 0;
int64_t scan_count;
- long nr;
+ int64_t scanned_objects = 0;
+ int64_t next_deferred = 0;
+ int64_t deferred_count = 0;
long new_nr;
int nid = shrinkctl->nid;
long batch_size = shrinker->batch ? shrinker->batch
: SHRINK_BATCH;
- long scanned = 0, next_deferred;
if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
nid = 0;
@@ -519,47 +519,53 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
return scan_count;
/*
- * copy the current shrinker scan count into a local variable
- * and zero it so that other concurrent shrinker invocations
- * don't also do this scanning work.
+ * If kswapd, we take all the deferred work and do it here. We don't let
+ * direct reclaim do this, because then it means some poor sod is going
+ * to have to do somebody else's GFP_NOFS reclaim, and it hides the real
+ * amount of reclaim work from concurrent kswapd operations. Hence we do
+ * the work in the wrong place, at the wrong time, and it's largely
+ * unpredictable.
+ *
+ * By doing the deferred work only in kswapd, we can schedule the work
+ * according the the reclaim priority - low priority reclaim will do
+ * less deferred work, hence we'll do more of the deferred work the more
+ * desperate we become for free memory. This avoids the need for needing
+ * to specifically avoid deferred work windup as low amount os memory
+ * pressure won't excessive trim caches anymore.
*/
- nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
+ if (current_is_kswapd()) {
+ int64_t deferred_scan;
- total_scan = nr + scan_count;
- if (total_scan < 0) {
- pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
- shrinker->scan_objects, total_scan);
- total_scan = scan_count;
- next_deferred = nr;
- } else
- next_deferred = total_scan;
+ deferred_count = atomic64_xchg(&shrinker->nr_deferred[nid], 0);
- /*
- * We need to avoid excessive windup on filesystem shrinkers
- * due to large numbers of GFP_NOFS allocations causing the
- * shrinkers to return -1 all the time. This results in a large
- * nr being built up so when a shrink that can do some work
- * comes along it empties the entire cache due to nr >>>
- * freeable. This is bad for sustaining a working set in
- * memory.
- *
- * Hence only allow the shrinker to scan the entire cache when
- * a large delta change is calculated directly.
- */
- if (scan_count < freeable_objects / 4)
- total_scan = min_t(long, total_scan, freeable_objects / 2);
+ /* we want to scan 5-10% of the deferred work here at minimum */
+ deferred_scan = deferred_count;
+ if (priority)
+ do_div(deferred_scan, priority);
+ scan_count += deferred_scan;
+
+ /*
+ * If there is more deferred work than the number of freeable
+ * items in the cache, limit the amount of work we will carry
+ * over to the next kswapd run on this cache. This prevents
+ * deferred work windup.
+ */
+ if (deferred_count > freeable_objects * 2)
+ deferred_count = freeable_objects * 2;
+
+ }
/*
* Avoid risking looping forever due to too large nr value:
* never try to free more than twice the estimate number of
* freeable entries.
*/
- if (total_scan > freeable_objects * 2)
- total_scan = freeable_objects * 2;
+ if (scan_count > freeable_objects * 2)
+ scan_count = freeable_objects * 2;
- trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
+ trace_mm_shrink_slab_start(shrinker, shrinkctl, deferred_count,
freeable_objects, scan_count,
- total_scan, priority);
+ scan_count, priority);
/*
* If the shrinker can't run (e.g. due to gfp_mask constraints), then
@@ -583,10 +589,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;
@@ -596,17 +602,17 @@ 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;
- else
- next_deferred = 0;
+ if (deferred_count)
+ next_deferred = deferred_count - scanned_objects;
+ else if (scan_count > 0)
+ next_deferred = scan_count;
/*
* move the unused scan count back into the shrinker in a
* manner that handles concurrent updates. If we exhausted the
@@ -618,7 +624,8 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
else
new_nr = atomic_long_read(&shrinker->nr_deferred[nid]);
- trace_mm_shrink_slab_end(shrinker, nid, freed, nr, new_nr, total_scan);
+ trace_mm_shrink_slab_end(shrinker, nid, freed, deferred_count, new_nr,
+ scan_count);
return freed;
}
--
2.22.0
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 04/24] shrinker: defer work only to kswapd
2019-08-01 2:17 ` [PATCH 04/24] shrinker: defer work only to kswapd Dave Chinner
@ 2019-08-02 15:34 ` Brian Foster
2019-08-04 16:48 ` Nikolay Borisov
` (2 subsequent siblings)
3 siblings, 0 replies; 87+ messages in thread
From: Brian Foster @ 2019-08-02 15:34 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Thu, Aug 01, 2019 at 12:17:32PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Right now deferred work is picked up by whatever GFP_KERNEL context
> reclaimer that wins the race to empty the node's deferred work
> counter. However, if there are lots of direct reclaimers, that
> work might be continually picked up by contexts taht can't do any
> work and so the opportunities to do the work are missed by contexts
> that could do them.
>
> A further problem with the current code is that the deferred work
> can be picked up by a random direct reclaimer, resulting in that
> specific process having to do all the deferred reclaim work and
> hence can take extremely long latencies if the reclaim work blocks
> regularly. This is not good for direct reclaim fairness or for
> minimising long tail latency events.
>
> To avoid these problems, simply limit deferred work to kswapd
> contexts. We know kswapd is a context that can always do reclaim
> work, and hence deferring work to kswapd allows the deferred work to
> be done in the background and not adversely affect any specific
> process context doing direct reclaim.
>
> The advantage of this is that amount of work to be done in direct
> reclaim is now bound and predictable - it is entirely based on
> the cache's freeable objects and the reclaim priority. hence all
> direct reclaimers running at the same time should be doing
> relatively equal amounts of work, thereby reducing the incidence of
> long tail latencies due to uneven reclaim workloads.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> mm/vmscan.c | 93 ++++++++++++++++++++++++++++-------------------------
> 1 file changed, 50 insertions(+), 43 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index b7472953b0e6..c583b4efb9bf 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -500,15 +500,15 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> struct shrinker *shrinker, int priority)
> {
> unsigned long freed = 0;
> - long total_scan;
> int64_t freeable_objects = 0;
> int64_t scan_count;
> - long nr;
> + int64_t scanned_objects = 0;
> + int64_t next_deferred = 0;
> + int64_t deferred_count = 0;
> long new_nr;
> int nid = shrinkctl->nid;
> long batch_size = shrinker->batch ? shrinker->batch
> : SHRINK_BATCH;
> - long scanned = 0, next_deferred;
>
> if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
> nid = 0;
> @@ -519,47 +519,53 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> return scan_count;
>
> /*
> - * copy the current shrinker scan count into a local variable
> - * and zero it so that other concurrent shrinker invocations
> - * don't also do this scanning work.
> + * If kswapd, we take all the deferred work and do it here. We don't let
> + * direct reclaim do this, because then it means some poor sod is going
> + * to have to do somebody else's GFP_NOFS reclaim, and it hides the real
> + * amount of reclaim work from concurrent kswapd operations. Hence we do
> + * the work in the wrong place, at the wrong time, and it's largely
> + * unpredictable.
> + *
> + * By doing the deferred work only in kswapd, we can schedule the work
> + * according the the reclaim priority - low priority reclaim will do
> + * less deferred work, hence we'll do more of the deferred work the more
> + * desperate we become for free memory. This avoids the need for needing
> + * to specifically avoid deferred work windup as low amount os memory
> + * pressure won't excessive trim caches anymore.
> */
> - nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
> + if (current_is_kswapd()) {
> + int64_t deferred_scan;
>
> - total_scan = nr + scan_count;
> - if (total_scan < 0) {
> - pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
> - shrinker->scan_objects, total_scan);
> - total_scan = scan_count;
> - next_deferred = nr;
> - } else
> - next_deferred = total_scan;
> + deferred_count = atomic64_xchg(&shrinker->nr_deferred[nid], 0);
>
> - /*
> - * We need to avoid excessive windup on filesystem shrinkers
> - * due to large numbers of GFP_NOFS allocations causing the
> - * shrinkers to return -1 all the time. This results in a large
> - * nr being built up so when a shrink that can do some work
> - * comes along it empties the entire cache due to nr >>>
> - * freeable. This is bad for sustaining a working set in
> - * memory.
> - *
> - * Hence only allow the shrinker to scan the entire cache when
> - * a large delta change is calculated directly.
> - */
> - if (scan_count < freeable_objects / 4)
> - total_scan = min_t(long, total_scan, freeable_objects / 2);
> + /* we want to scan 5-10% of the deferred work here at minimum */
> + deferred_scan = deferred_count;
> + if (priority)
> + do_div(deferred_scan, priority);
> + scan_count += deferred_scan;
> +
> + /*
> + * If there is more deferred work than the number of freeable
> + * items in the cache, limit the amount of work we will carry
> + * over to the next kswapd run on this cache. This prevents
> + * deferred work windup.
> + */
> + if (deferred_count > freeable_objects * 2)
> + deferred_count = freeable_objects * 2;
> +
Hmm, what's the purpose of this check? Is this not handled once the
deferred count is absorbed into scan_count (where we apply the same
logic a few lines below)? Perhaps the latter prevents too much scanning
in a single call into the shrinker whereas this check prevents kswapd
from getting too far behind?
> + }
>
> /*
> * Avoid risking looping forever due to too large nr value:
> * never try to free more than twice the estimate number of
> * freeable entries.
> */
> - if (total_scan > freeable_objects * 2)
> - total_scan = freeable_objects * 2;
> + if (scan_count > freeable_objects * 2)
> + scan_count = freeable_objects * 2;
>
> - trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
> + trace_mm_shrink_slab_start(shrinker, shrinkctl, deferred_count,
> freeable_objects, scan_count,
> - total_scan, priority);
> + scan_count, priority);
>
> /*
> * If the shrinker can't run (e.g. due to gfp_mask constraints), then
> @@ -583,10 +589,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;
> @@ -596,17 +602,17 @@ 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;
> - else
> - next_deferred = 0;
> + if (deferred_count)
> + next_deferred = deferred_count - scanned_objects;
> + else if (scan_count > 0)
> + next_deferred = scan_count;
I was wondering why we dropped the >= scanned_objects check, but I see
that next_deferred is signed and we check for next_deferred > 0 below.
What is odd is that so is scan_count, yet we check > 0 here for
assignment to the same variable. Can we be a little more consistent here
one way or the other?
Brian
> /*
> * move the unused scan count back into the shrinker in a
> * manner that handles concurrent updates. If we exhausted the
> @@ -618,7 +624,8 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> else
> new_nr = atomic_long_read(&shrinker->nr_deferred[nid]);
>
> - trace_mm_shrink_slab_end(shrinker, nid, freed, nr, new_nr, total_scan);
> + trace_mm_shrink_slab_end(shrinker, nid, freed, deferred_count, new_nr,
> + scan_count);
> return freed;
> }
>
> --
> 2.22.0
>
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 04/24] shrinker: defer work only to kswapd
2019-08-01 2:17 ` [PATCH 04/24] shrinker: defer work only to kswapd Dave Chinner
2019-08-02 15:34 ` Brian Foster
@ 2019-08-04 16:48 ` Nikolay Borisov
2019-08-04 21:37 ` Dave Chinner
2019-08-07 16:12 ` kbuild test robot
2019-08-07 18:00 ` kbuild test robot
3 siblings, 1 reply; 87+ messages in thread
From: Nikolay Borisov @ 2019-08-04 16:48 UTC (permalink / raw)
To: Dave Chinner, linux-xfs; +Cc: linux-mm, linux-fsdevel
On 1.08.19 г. 5:17 ч., Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Right now deferred work is picked up by whatever GFP_KERNEL context
> reclaimer that wins the race to empty the node's deferred work
> counter. However, if there are lots of direct reclaimers, that
> work might be continually picked up by contexts taht can't do any
> work and so the opportunities to do the work are missed by contexts
> that could do them.
>
> A further problem with the current code is that the deferred work
> can be picked up by a random direct reclaimer, resulting in that
> specific process having to do all the deferred reclaim work and
> hence can take extremely long latencies if the reclaim work blocks
> regularly. This is not good for direct reclaim fairness or for
> minimising long tail latency events.
>
> To avoid these problems, simply limit deferred work to kswapd
> contexts. We know kswapd is a context that can always do reclaim
> work, and hence deferring work to kswapd allows the deferred work to
> be done in the background and not adversely affect any specific
> process context doing direct reclaim.
>
> The advantage of this is that amount of work to be done in direct
> reclaim is now bound and predictable - it is entirely based on
> the cache's freeable objects and the reclaim priority. hence all
> direct reclaimers running at the same time should be doing
> relatively equal amounts of work, thereby reducing the incidence of
> long tail latencies due to uneven reclaim workloads.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> mm/vmscan.c | 93 ++++++++++++++++++++++++++++-------------------------
> 1 file changed, 50 insertions(+), 43 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index b7472953b0e6..c583b4efb9bf 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -500,15 +500,15 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> struct shrinker *shrinker, int priority)
> {
> unsigned long freed = 0;
> - long total_scan;
> int64_t freeable_objects = 0;
> int64_t scan_count;
> - long nr;
> + int64_t scanned_objects = 0;
> + int64_t next_deferred = 0;
> + int64_t deferred_count = 0;
> long new_nr;
> int nid = shrinkctl->nid;
> long batch_size = shrinker->batch ? shrinker->batch
> : SHRINK_BATCH;
> - long scanned = 0, next_deferred;
>
> if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
> nid = 0;
> @@ -519,47 +519,53 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> return scan_count;
>
> /*
> - * copy the current shrinker scan count into a local variable
> - * and zero it so that other concurrent shrinker invocations
> - * don't also do this scanning work.
> + * If kswapd, we take all the deferred work and do it here. We don't let
> + * direct reclaim do this, because then it means some poor sod is going
> + * to have to do somebody else's GFP_NOFS reclaim, and it hides the real
> + * amount of reclaim work from concurrent kswapd operations. Hence we do
> + * the work in the wrong place, at the wrong time, and it's largely
> + * unpredictable.
> + *
> + * By doing the deferred work only in kswapd, we can schedule the work
> + * according the the reclaim priority - low priority reclaim will do
> + * less deferred work, hence we'll do more of the deferred work the more
> + * desperate we become for free memory. This avoids the need for needing
> + * to specifically avoid deferred work windup as low amount os memory
> + * pressure won't excessive trim caches anymore.
> */
> - nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
> + if (current_is_kswapd()) {
> + int64_t deferred_scan;
>
> - total_scan = nr + scan_count;
> - if (total_scan < 0) {
> - pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
> - shrinker->scan_objects, total_scan);
> - total_scan = scan_count;
> - next_deferred = nr;
> - } else
> - next_deferred = total_scan;
> + deferred_count = atomic64_xchg(&shrinker->nr_deferred[nid], 0);
>
> - /*
> - * We need to avoid excessive windup on filesystem shrinkers
> - * due to large numbers of GFP_NOFS allocations causing the
> - * shrinkers to return -1 all the time. This results in a large
> - * nr being built up so when a shrink that can do some work
> - * comes along it empties the entire cache due to nr >>>
> - * freeable. This is bad for sustaining a working set in
> - * memory.
> - *
> - * Hence only allow the shrinker to scan the entire cache when
> - * a large delta change is calculated directly.
> - */
> - if (scan_count < freeable_objects / 4)
> - total_scan = min_t(long, total_scan, freeable_objects / 2);
> + /* we want to scan 5-10% of the deferred work here at minimum */
> + deferred_scan = deferred_count;
> + if (priority)
> + do_div(deferred_scan, priority);
> + scan_count += deferred_scan;
> +
> + /*
> + * If there is more deferred work than the number of freeable
> + * items in the cache, limit the amount of work we will carry
> + * over to the next kswapd run on this cache. This prevents
> + * deferred work windup.
> + */
> + if (deferred_count > freeable_objects * 2)
> + deferred_count = freeable_objects * 2;
nit : deferred_count = min(deferred_count, freeable_objects * 2).
How can we have more deferred objects than are currently on the LRU?
Aren't deferred objects always some part of freeable objects. Shouldn't
this mean that for a particular shrinker deferred_count <= freeable_objects?
> +
> + }
>
> /*
> * Avoid risking looping forever due to too large nr value:
> * never try to free more than twice the estimate number of
> * freeable entries.
> */
> - if (total_scan > freeable_objects * 2)
> - total_scan = freeable_objects * 2;
> + if (scan_count > freeable_objects * 2)
> + scan_count = freeable_objects * 2;
nit: scan_count = min(scan_count, freeable_objects * 2);
>
> - trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
> + trace_mm_shrink_slab_start(shrinker, shrinkctl, deferred_count,
> freeable_objects, scan_count,
> - total_scan, priority);
> + scan_count, priority);
>
> /*
> * If the shrinker can't run (e.g. due to gfp_mask constraints), then
> @@ -583,10 +589,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;
> @@ -596,17 +602,17 @@ 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;
> - else
> - next_deferred = 0;
> + if (deferred_count)
> + next_deferred = deferred_count - scanned_objects;
> + else if (scan_count > 0)
> + next_deferred = scan_count;
> /*
> * move the unused scan count back into the shrinker in a
> * manner that handles concurrent updates. If we exhausted the
> @@ -618,7 +624,8 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> else
> new_nr = atomic_long_read(&shrinker->nr_deferred[nid]);
>
> - trace_mm_shrink_slab_end(shrinker, nid, freed, nr, new_nr, total_scan);
> + trace_mm_shrink_slab_end(shrinker, nid, freed, deferred_count, new_nr,
> + scan_count);
> return freed;
> }
>
>
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 04/24] shrinker: defer work only to kswapd
2019-08-04 16:48 ` Nikolay Borisov
@ 2019-08-04 21:37 ` Dave Chinner
0 siblings, 0 replies; 87+ messages in thread
From: Dave Chinner @ 2019-08-04 21:37 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-xfs, linux-mm, linux-fsdevel
On Sun, Aug 04, 2019 at 07:48:01PM +0300, Nikolay Borisov wrote:
>
>
> On 1.08.19 г. 5:17 ч., Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Right now deferred work is picked up by whatever GFP_KERNEL context
> > reclaimer that wins the race to empty the node's deferred work
> > counter. However, if there are lots of direct reclaimers, that
> > work might be continually picked up by contexts taht can't do any
> > work and so the opportunities to do the work are missed by contexts
> > that could do them.
> >
> > A further problem with the current code is that the deferred work
> > can be picked up by a random direct reclaimer, resulting in that
> > specific process having to do all the deferred reclaim work and
> > hence can take extremely long latencies if the reclaim work blocks
> > regularly. This is not good for direct reclaim fairness or for
> > minimising long tail latency events.
> >
> > To avoid these problems, simply limit deferred work to kswapd
> > contexts. We know kswapd is a context that can always do reclaim
> > work, and hence deferring work to kswapd allows the deferred work to
> > be done in the background and not adversely affect any specific
> > process context doing direct reclaim.
> >
> > The advantage of this is that amount of work to be done in direct
> > reclaim is now bound and predictable - it is entirely based on
> > the cache's freeable objects and the reclaim priority. hence all
> > direct reclaimers running at the same time should be doing
> > relatively equal amounts of work, thereby reducing the incidence of
> > long tail latencies due to uneven reclaim workloads.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> > mm/vmscan.c | 93 ++++++++++++++++++++++++++++-------------------------
> > 1 file changed, 50 insertions(+), 43 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index b7472953b0e6..c583b4efb9bf 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -500,15 +500,15 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > struct shrinker *shrinker, int priority)
> > {
> > unsigned long freed = 0;
> > - long total_scan;
> > int64_t freeable_objects = 0;
> > int64_t scan_count;
> > - long nr;
> > + int64_t scanned_objects = 0;
> > + int64_t next_deferred = 0;
> > + int64_t deferred_count = 0;
> > long new_nr;
> > int nid = shrinkctl->nid;
> > long batch_size = shrinker->batch ? shrinker->batch
> > : SHRINK_BATCH;
> > - long scanned = 0, next_deferred;
> >
> > if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
> > nid = 0;
> > @@ -519,47 +519,53 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > return scan_count;
> >
> > /*
> > - * copy the current shrinker scan count into a local variable
> > - * and zero it so that other concurrent shrinker invocations
> > - * don't also do this scanning work.
> > + * If kswapd, we take all the deferred work and do it here. We don't let
> > + * direct reclaim do this, because then it means some poor sod is going
> > + * to have to do somebody else's GFP_NOFS reclaim, and it hides the real
> > + * amount of reclaim work from concurrent kswapd operations. Hence we do
> > + * the work in the wrong place, at the wrong time, and it's largely
> > + * unpredictable.
> > + *
> > + * By doing the deferred work only in kswapd, we can schedule the work
> > + * according the the reclaim priority - low priority reclaim will do
> > + * less deferred work, hence we'll do more of the deferred work the more
> > + * desperate we become for free memory. This avoids the need for needing
> > + * to specifically avoid deferred work windup as low amount os memory
> > + * pressure won't excessive trim caches anymore.
> > */
> > - nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
> > + if (current_is_kswapd()) {
> > + int64_t deferred_scan;
> >
> > - total_scan = nr + scan_count;
> > - if (total_scan < 0) {
> > - pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
> > - shrinker->scan_objects, total_scan);
> > - total_scan = scan_count;
> > - next_deferred = nr;
> > - } else
> > - next_deferred = total_scan;
> > + deferred_count = atomic64_xchg(&shrinker->nr_deferred[nid], 0);
> >
> > - /*
> > - * We need to avoid excessive windup on filesystem shrinkers
> > - * due to large numbers of GFP_NOFS allocations causing the
> > - * shrinkers to return -1 all the time. This results in a large
> > - * nr being built up so when a shrink that can do some work
> > - * comes along it empties the entire cache due to nr >>>
> > - * freeable. This is bad for sustaining a working set in
> > - * memory.
> > - *
> > - * Hence only allow the shrinker to scan the entire cache when
> > - * a large delta change is calculated directly.
> > - */
> > - if (scan_count < freeable_objects / 4)
> > - total_scan = min_t(long, total_scan, freeable_objects / 2);
> > + /* we want to scan 5-10% of the deferred work here at minimum */
> > + deferred_scan = deferred_count;
> > + if (priority)
> > + do_div(deferred_scan, priority);
> > + scan_count += deferred_scan;
> > +
> > + /*
> > + * If there is more deferred work than the number of freeable
> > + * items in the cache, limit the amount of work we will carry
> > + * over to the next kswapd run on this cache. This prevents
> > + * deferred work windup.
> > + */
> > + if (deferred_count > freeable_objects * 2)
> > + deferred_count = freeable_objects * 2;
>
> nit : deferred_count = min(deferred_count, freeable_objects * 2).
*nod*
> How can we have more deferred objects than are currently on the LRU?
deferred work is aggregated. Put enough direct reclaimers in action
in GFP_NOFS context (e.g. fsmark create workload) and it will wind
up the deferred count much faster than kswapd can drain it.
> Aren't deferred objects always some part of freeable objects.
For a single scan, yes. In aggregate, no.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 04/24] shrinker: defer work only to kswapd
2019-08-01 2:17 ` [PATCH 04/24] shrinker: defer work only to kswapd Dave Chinner
2019-08-02 15:34 ` Brian Foster
2019-08-04 16:48 ` Nikolay Borisov
@ 2019-08-07 16:12 ` kbuild test robot
2019-08-07 18:00 ` kbuild test robot
3 siblings, 0 replies; 87+ messages in thread
From: kbuild test robot @ 2019-08-07 16:12 UTC (permalink / raw)
To: Dave Chinner; +Cc: kbuild-all, linux-xfs, linux-mm, linux-fsdevel
Hi Dave,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc3 next-20190807]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dave-Chinner/mm-xfs-non-blocking-inode-reclaim/20190804-042311
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-rc1-7-g2b96cd8-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> mm/vmscan.c:539:70: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected struct atomic64_t [usertype] *v @@ got ruct atomic64_t [usertype] *v @@
>> mm/vmscan.c:539:70: sparse: expected struct atomic64_t [usertype] *v
>> mm/vmscan.c:539:70: sparse: got struct atomic_t [usertype] *
arch/x86/include/asm/irqflags.h:54:9: sparse: sparse: context imbalance in 'check_move_unevictable_pages' - unexpected unlock
vim +539 mm/vmscan.c
498
499 static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
500 struct shrinker *shrinker, int priority)
501 {
502 unsigned long freed = 0;
503 int64_t freeable_objects = 0;
504 int64_t scan_count;
505 int64_t scanned_objects = 0;
506 int64_t next_deferred = 0;
507 int64_t deferred_count = 0;
508 long new_nr;
509 int nid = shrinkctl->nid;
510 long batch_size = shrinker->batch ? shrinker->batch
511 : SHRINK_BATCH;
512
513 if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
514 nid = 0;
515
516 scan_count = shrink_scan_count(shrinkctl, shrinker, priority,
517 &freeable_objects);
518 if (scan_count == 0 || scan_count == SHRINK_EMPTY)
519 return scan_count;
520
521 /*
522 * If kswapd, we take all the deferred work and do it here. We don't let
523 * direct reclaim do this, because then it means some poor sod is going
524 * to have to do somebody else's GFP_NOFS reclaim, and it hides the real
525 * amount of reclaim work from concurrent kswapd operations. Hence we do
526 * the work in the wrong place, at the wrong time, and it's largely
527 * unpredictable.
528 *
529 * By doing the deferred work only in kswapd, we can schedule the work
530 * according the the reclaim priority - low priority reclaim will do
531 * less deferred work, hence we'll do more of the deferred work the more
532 * desperate we become for free memory. This avoids the need for needing
533 * to specifically avoid deferred work windup as low amount os memory
534 * pressure won't excessive trim caches anymore.
535 */
536 if (current_is_kswapd()) {
537 int64_t deferred_scan;
538