Re: [PATCH] mm: terminate shrink_slab loop if signal is pending
> 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.
Re: [PATCH] mm: terminate shrink_slab loop if signal is pending
> 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.
Re: [PATCH] mm: terminate shrink_slab loop if signal is pending
On Thu, Dec 7, 2017 at 12:34 AM, Michal Hockowrote: > 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
On Thu, Dec 7, 2017 at 12:34 AM, 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 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
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 Hockowrote: > 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 >> > --- >> > 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
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 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 >> > --- >> > 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
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
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
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
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
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> > --- > > 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
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 > > --- > > 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
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> --- > 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
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 > --- > 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
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> --- > 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
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 > --- > 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
> > 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
> > 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
On Wed, 6 Dec 2017 11:20:26 -0800 Suren Baghdasaryanwrote: > 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
On Wed, 6 Dec 2017 11:20:26 -0800 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. Some quantification of "quite time consuming" and "delay" would be interesting, please.
[PATCH] mm: terminate shrink_slab loop if signal is pending
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--- 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
[PATCH] mm: terminate shrink_slab loop if signal is pending
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 --- 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