[PATCH] mm: terminate shrink_slab loop if signal is pending

[PATCH] mm: terminate shrink_slab loop if signal is pending

[Suren Baghdasaryan]

Slab shrinkers can be quite time consuming and when signal
is pending they can delay handling of the signal. If fatal
signal is pending there is no point in shrinking that process
since it will be killed anyway. This change checks for pending
fatal signals inside shrink_slab loop and if one is detected
terminates this loop early.

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 mm/vmscan.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index c02c850ea349..69296528ff33 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -486,6 +486,13 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
 			.memcg = memcg,
 		};
 
+		/*
+		 * We are about to die and free our memory.
+		 * Stop shrinking which might delay signal handling.
+		 */
+		if (unlikely(fatal_signal_pending(current))
+			break;
+
 		/*
 		 * If kernel memory accounting is disabled, we ignore
 		 * SHRINKER_MEMCG_AWARE flag and call all shrinkers
-- 
2.15.1.424.g9478a66081-goog

Re: [PATCH] mm: terminate shrink_slab loop if signal is pending

[Andrew Morton]

On Wed,  6 Dec 2017 11:20:26 -0800 Suren Baghdasaryan <surenb@google.com> wrote:

> Slab shrinkers can be quite time consuming and when signal
> is pending they can delay handling of the signal. If fatal
> signal is pending there is no point in shrinking that process
> since it will be killed anyway. This change checks for pending
> fatal signals inside shrink_slab loop and if one is detected
> terminates this loop early.

Some quantification of "quite time consuming" and "delay" would be
interesting, please.

Re: [PATCH] mm: terminate shrink_slab loop if signal is pending

[Suren Baghdasaryan]

>
> Some quantification of "quite time consuming" and "delay" would be
> interesting, please.
>

Unfortunately that depends on the implementation of the shrinkers
registered in the system including the ones from drivers. I've
captured traces showing delays of up to 100ms where the process with
pending SIGKILL is in direct memory reclaim and signal handling is
delayed because of that. I realize that it's not the fault of
shrink_slab_lmk() that some shrinkers take long time to shrink their
slabs (sometimes because of justifiable reasons and sometimes because
of a bug which has to be fixed) but this can be a safeguard against
such cases.
Couple shrinker examples that I found most time consuming are (most of
that 100ms delay is the result of the first two ones):

https://patchwork.kernel.org/patch/10096641/
The patch fixes dm-bufio shrinker which in certain conditions reclaims
only one buffer per scan making the shrinking process very
inefficient.

https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/gpu/msm/kgsl_pool.c#420
This example is from a driver where shrinker returns 0 instead of
SHRINK_STOP when it's unable to reclaim anymore. As a result when
total_scan in do_shrink_slab() is large this will cause multiple
scan_objects() calls with no memory being reclaimed. Patch for this
one is under review by the owners.

Shrinker that seems to be justifiably heavy is super_cache_scan()
inside fs/super.c. I have traces where it takes up to 4ms to complete.

Re: [PATCH] mm: terminate shrink_slab loop if signal is pending

[Michal Hocko]

On Wed 06-12-17 11:20:26, Suren Baghdasaryan wrote:
> Slab shrinkers can be quite time consuming and when signal
> is pending they can delay handling of the signal. If fatal
> signal is pending there is no point in shrinking that process
> since it will be killed anyway. This change checks for pending
> fatal signals inside shrink_slab loop and if one is detected
> terminates this loop early.

This is not enough. You would have to make sure the direct reclaim will
bail out immeditally which is not at all that simple. We do check fatal
signals in throttle_direct_reclaim and conditionally in shrink_inactive_list
so even if you bail out from shrinkers we could still finish the full
reclaim cycle.

Besides that shrinkers shouldn't really take very long so this looks
like it papers over a real bug somewhere else. I am not saying the patch
is wrong but it would deserve much more details to judge wether this is
the right way to go for your particular problem.

> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>  mm/vmscan.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index c02c850ea349..69296528ff33 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -486,6 +486,13 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
>  			.memcg = memcg,
>  		};
>  
> +		/*
> +		 * We are about to die and free our memory.
> +		 * Stop shrinking which might delay signal handling.
> +		 */
> +		if (unlikely(fatal_signal_pending(current))
> +			break;
> +
>  		/*
>  		 * If kernel memory accounting is disabled, we ignore
>  		 * SHRINKER_MEMCG_AWARE flag and call all shrinkers
> -- 
> 2.15.1.424.g9478a66081-goog
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: terminate shrink_slab loop if signal is pending

[Sergey Senozhatsky]

On (12/06/17 11:20), Suren Baghdasaryan wrote:
> Slab shrinkers can be quite time consuming and when signal
> is pending they can delay handling of the signal. If fatal
> signal is pending there is no point in shrinking that process
> since it will be killed anyway. This change checks for pending
> fatal signals inside shrink_slab loop and if one is detected
> terminates this loop early.
> 
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>  mm/vmscan.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index c02c850ea349..69296528ff33 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -486,6 +486,13 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
>  			.memcg = memcg,
>  		};
>  
> +		/*
> +		 * We are about to die and free our memory.
> +		 * Stop shrinking which might delay signal handling.
> +		 */
> +		if (unlikely(fatal_signal_pending(current))

-               if (unlikely(fatal_signal_pending(current))
+               if (unlikely(fatal_signal_pending(current)))

	-ss

Re: [PATCH] mm: terminate shrink_slab loop if signal is pending

[Michal Hocko]

On Thu 07-12-17 18:52:23, Sergey Senozhatsky wrote:
> On (12/06/17 11:20), Suren Baghdasaryan wrote:
> > Slab shrinkers can be quite time consuming and when signal
> > is pending they can delay handling of the signal. If fatal
> > signal is pending there is no point in shrinking that process
> > since it will be killed anyway. This change checks for pending
> > fatal signals inside shrink_slab loop and if one is detected
> > terminates this loop early.
> > 
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> >  mm/vmscan.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index c02c850ea349..69296528ff33 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -486,6 +486,13 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
> >  			.memcg = memcg,
> >  		};
> >  
> > +		/*
> > +		 * We are about to die and free our memory.
> > +		 * Stop shrinking which might delay signal handling.
> > +		 */
> > +		if (unlikely(fatal_signal_pending(current))
> 
> -               if (unlikely(fatal_signal_pending(current))
> +               if (unlikely(fatal_signal_pending(current)))

Heh, well, spotted. This begs a question how this has been tested, if at
all?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: terminate shrink_slab loop if signal is pending

[Tetsuo Handa]

On 2017/12/07 17:34, Michal Hocko wrote:
> On Wed 06-12-17 11:20:26, Suren Baghdasaryan wrote:
>> Slab shrinkers can be quite time consuming and when signal
>> is pending they can delay handling of the signal. If fatal
>> signal is pending there is no point in shrinking that process
>> since it will be killed anyway. This change checks for pending
>> fatal signals inside shrink_slab loop and if one is detected
>> terminates this loop early.
> 
> This is not enough. You would have to make sure the direct reclaim will
> bail out immeditally which is not at all that simple. We do check fatal
> signals in throttle_direct_reclaim and conditionally in shrink_inactive_list
> so even if you bail out from shrinkers we could still finish the full
> reclaim cycle.
> 
> Besides that shrinkers shouldn't really take very long so this looks
> like it papers over a real bug somewhere else. I am not saying the patch
> is wrong but it would deserve much more details to judge wether this is
> the right way to go for your particular problem.
> 

I wish that normal threads do not invoke direct reclaim operation.
Only dedicated kernel threads (such as filesystem's writeback) invoke
direct reclaim operation. Then, we can implement __GFP_KILLABLE for
normal threads, and hopefully get rid of distinction between GFP_NOIO/
GFP_NOFS/GFP_KERNEL because reclaim (and locking) dependency becomes
simpler.

Re: [PATCH] mm: terminate shrink_slab loop if signal is pending

[Suren Baghdasaryan]

I'm, terribly sorry. My original code was checking for additional
condition which I realized is not useful here because it would mean
the signal was already processed. Should have missed the error while
removing it. Will address Michal's comments and fix the problem.

On Thu, Dec 7, 2017 at 1:58 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Thu 07-12-17 18:52:23, Sergey Senozhatsky wrote:
>> On (12/06/17 11:20), Suren Baghdasaryan wrote:
>> > Slab shrinkers can be quite time consuming and when signal
>> > is pending they can delay handling of the signal. If fatal
>> > signal is pending there is no point in shrinking that process
>> > since it will be killed anyway. This change checks for pending
>> > fatal signals inside shrink_slab loop and if one is detected
>> > terminates this loop early.
>> >
>> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>> > ---
>> >  mm/vmscan.c | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/mm/vmscan.c b/mm/vmscan.c
>> > index c02c850ea349..69296528ff33 100644
>> > --- a/mm/vmscan.c
>> > +++ b/mm/vmscan.c
>> > @@ -486,6 +486,13 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
>> >                     .memcg = memcg,
>> >             };
>> >
>> > +           /*
>> > +            * We are about to die and free our memory.
>> > +            * Stop shrinking which might delay signal handling.
>> > +            */
>> > +           if (unlikely(fatal_signal_pending(current))
>>
>> -               if (unlikely(fatal_signal_pending(current))
>> +               if (unlikely(fatal_signal_pending(current)))
>
> Heh, well, spotted. This begs a question how this has been tested, if at
> all?
> --
> Michal Hocko
> SUSE Labs

Re: [PATCH] mm: terminate shrink_slab loop if signal is pending

[Michal Hocko]

On Thu 07-12-17 07:46:07, Suren Baghdasaryan wrote:
> I'm, terribly sorry. My original code was checking for additional
> condition which I realized is not useful here because it would mean
> the signal was already processed. Should have missed the error while
> removing it. Will address Michal's comments and fix the problem.

yes, rebasing at last moment tend to screw things... No worries, I would
be more worried about the general approach here and its documentataion.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: terminate shrink_slab loop if signal is pending

[Suren Baghdasaryan]

On Thu, Dec 7, 2017 at 12:34 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Wed 06-12-17 11:20:26, Suren Baghdasaryan wrote:
>> Slab shrinkers can be quite time consuming and when signal
>> is pending they can delay handling of the signal. If fatal
>> signal is pending there is no point in shrinking that process
>> since it will be killed anyway. This change checks for pending
>> fatal signals inside shrink_slab loop and if one is detected
>> terminates this loop early.
>
> This is not enough. You would have to make sure the direct reclaim will
> bail out immeditally which is not at all that simple. We do check fatal
> signals in throttle_direct_reclaim and conditionally in shrink_inactive_list
> so even if you bail out from shrinkers we could still finish the full
> reclaim cycle.
>
> Besides that shrinkers shouldn't really take very long so this looks
> like it papers over a real bug somewhere else. I am not saying the patch
> is wrong but it would deserve much more details to judge wether this is
> the right way to go for your particular problem.
>

I agree that this check alone is not going to terminate direct
reclaim, rather it's designed to prevent a long-running shrinkers from
delaying SIGKILL handling. I tried to reflect that in the description
of the patch but maybe I should rephrase that. Why I want to prevent
specifically shrinkers from running with pending SIGKILL is because
drivers can register their own shrinkers and one poorly-written driver
can affect signal delivery of the whole system. I realize this can be
viewed as an attempt to hide bugs in shrinkers but my intend was to
place some safeguard for the system, not a replacement for a proper
fix.

fatal_signal_pending checks in throttle_direct_reclaim are
interesting. I might be missing something and relevant description of
the code here https://lkml.org/lkml/2012/11/21/566 did not help me
much but I think the logic inside throttle_direct_reclaim would not
prevent direct reclaim if signal is already pending. Looks like it
prevents direct reclaim only if fatal signal is received between the
two fatal_signal_pending checks inside throttle_direct_reclaim. I'm
not saying the code is wrong and looks like it was designed with a
specific use case in mind, just saying that it's not going to prevent
direct reclaim from running if signal was pending before
throttle_direct_reclaim is called. And indeed I can see in my traces
several invocations of do_try_to_free_pages from try_to_free_pages
while SIGKILL is pending (the worst case so far was 5 invocations).

I also agree that shrinkers in general should not take long time to
run. After fixing couple of them that I mentioned in my reply to
Andrew here: https://lkml.org/lkml/2017/12/6/1095 I collected more
traces using patched shrinkers. The problem is less pronounced and
harder to reproduce. The worst case delay for SIGKILL handling I've
got so far is 43ms and this patch would shave about 7% of it.
According to my traces this 43ms could drop to the average of 11ms and
worst case 25ms if throttle_direct_reclaim would return true when
fatal signal is pending but I would like to hear your opinion about
throttle_direct_reclaim logic.

And thank you all for the comments and corrections.

Re: [PATCH] mm: terminate shrink_slab loop if signal is pending

[Suren Baghdasaryan]

> According to my traces this 43ms could drop to the average of 11ms and
> worst case 25ms if throttle_direct_reclaim would return true when
> fatal signal is pending but I would like to hear your opinion about
> throttle_direct_reclaim logic.

Digging some more into this I realize my last statement might be
incorrect. Throttling in this situation might not help with the signal
handling delay because of the logic in __alloc_pages_slowpath. I'll
have to experiment with this first, please disregard that last
statement for now.