[PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Tetsuo Handa]

After the OOM killer is disabled during suspend operation,
any !__GFP_NOFAIL && __GFP_FS allocations are forced to fail.
Thus, any !__GFP_NOFAIL && !__GFP_FS allocations should be
forced to fail as well.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3c3a5c5..214f824 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2766,7 +2766,7 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
 			 * and the OOM killer can't be invoked, but
 			 * keep looping as per tradition.
 			 */
-			*did_some_progress = 1;
+			*did_some_progress = !oom_killer_disabled;
 			goto out;
 		}
 		if (pm_suspended_storage())
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Michal Hocko]

On Mon 11-01-16 14:07:16, Tetsuo Handa wrote:
> After the OOM killer is disabled during suspend operation,
> any !__GFP_NOFAIL && __GFP_FS allocations are forced to fail.
> Thus, any !__GFP_NOFAIL && !__GFP_FS allocations should be
> forced to fail as well.

I hoped for minimum exposure of oom_killer_disabled outside of the OOM
proper but this seems to be the easiest way to go.

> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 3c3a5c5..214f824 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2766,7 +2766,7 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  			 * and the OOM killer can't be invoked, but
>  			 * keep looping as per tradition.
>  			 */
> -			*did_some_progress = 1;
> +			*did_some_progress = !oom_killer_disabled;
>  			goto out;
>  		}
>  		if (pm_suspended_storage())
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Johannes Weiner]

On Mon, Jan 11, 2016 at 02:07:16PM +0900, Tetsuo Handa wrote:
> After the OOM killer is disabled during suspend operation,
> any !__GFP_NOFAIL && __GFP_FS allocations are forced to fail.
> Thus, any !__GFP_NOFAIL && !__GFP_FS allocations should be
> forced to fail as well.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Why? We had to acknowledge that !__GFP_FS allocations can not fail
even when they can't invoke the OOM killer. They are NOFAIL. Just like
an explicit __GFP_NOFAIL they should trigger a warning when they occur
after the OOM killer has been disabled and then keep looping.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Michal Hocko]

On Mon 11-01-16 12:00:47, Johannes Weiner wrote:
> On Mon, Jan 11, 2016 at 02:07:16PM +0900, Tetsuo Handa wrote:
> > After the OOM killer is disabled during suspend operation,
> > any !__GFP_NOFAIL && __GFP_FS allocations are forced to fail.
> > Thus, any !__GFP_NOFAIL && !__GFP_FS allocations should be
> > forced to fail as well.
> > 
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> 
> Why? We had to acknowledge that !__GFP_FS allocations can not fail
> even when they can't invoke the OOM killer. They are NOFAIL. Just like
> an explicit __GFP_NOFAIL they should trigger a warning when they occur
> after the OOM killer has been disabled and then keep looping.

They are more like GFP_KERNEL than GFP_NOFAIL IMO because unlike
GFP_NOFAIL they are already allowed to fail due to fatal_signals_pending
and this has been the case for a really long time.  Even semantically
they are basically GFP_KERNEL with FS recursion protection in majority
cases. And I believe that we should allow them to fail long term after
some FS (btrfs at least) catch up and start handling failures properly.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Johannes Weiner]

On Mon, Jan 11, 2016 at 06:20:58PM +0100, Michal Hocko wrote:
> On Mon 11-01-16 12:00:47, Johannes Weiner wrote:
> > On Mon, Jan 11, 2016 at 02:07:16PM +0900, Tetsuo Handa wrote:
> > > After the OOM killer is disabled during suspend operation,
> > > any !__GFP_NOFAIL && __GFP_FS allocations are forced to fail.
> > > Thus, any !__GFP_NOFAIL && !__GFP_FS allocations should be
> > > forced to fail as well.
> > > 
> > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > 
> > Why? We had to acknowledge that !__GFP_FS allocations can not fail
> > even when they can't invoke the OOM killer. They are NOFAIL. Just like
> > an explicit __GFP_NOFAIL they should trigger a warning when they occur
> > after the OOM killer has been disabled and then keep looping.
> 
> They are more like GFP_KERNEL than GFP_NOFAIL IMO because unlike
> GFP_NOFAIL they are already allowed to fail due to fatal_signals_pending
> and this has been the case for a really long time.  Even semantically
> they are basically GFP_KERNEL with FS recursion protection in majority
> cases. And I believe that we should allow them to fail long term after
> some FS (btrfs at least) catch up and start handling failures properly.

I see, yeah that's probably a better way to look at it.

Thanks!

Scratch my objection to this patch then. But please do add to/update
that XXX comment above that line, or it'll be confusing. Hm?

			/*
			 * XXX: Page reclaim didn't yield anything,
			 * and the OOM killer can't be invoked, but
			 * keep looping as per tradition. Unless the
			 * system is trying to enter a quiescent state
			 * during suspend and the OOM killer has been
			 * shut off already. Give up like with other
			 * !__GFP_NOFAIL allocations in that case.
			 */
			*did_some_progress = !oom_killer_disabled;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Michal Hocko]

On Mon 11-01-16 12:43:29, Johannes Weiner wrote:
> On Mon, Jan 11, 2016 at 06:20:58PM +0100, Michal Hocko wrote:
> > On Mon 11-01-16 12:00:47, Johannes Weiner wrote:
> > > On Mon, Jan 11, 2016 at 02:07:16PM +0900, Tetsuo Handa wrote:
> > > > After the OOM killer is disabled during suspend operation,
> > > > any !__GFP_NOFAIL && __GFP_FS allocations are forced to fail.
> > > > Thus, any !__GFP_NOFAIL && !__GFP_FS allocations should be
> > > > forced to fail as well.
> > > > 
> > > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > > 
> > > Why? We had to acknowledge that !__GFP_FS allocations can not fail
> > > even when they can't invoke the OOM killer. They are NOFAIL. Just like
> > > an explicit __GFP_NOFAIL they should trigger a warning when they occur
> > > after the OOM killer has been disabled and then keep looping.
> > 
> > They are more like GFP_KERNEL than GFP_NOFAIL IMO because unlike
> > GFP_NOFAIL they are already allowed to fail due to fatal_signals_pending
> > and this has been the case for a really long time.  Even semantically
> > they are basically GFP_KERNEL with FS recursion protection in majority
> > cases. And I believe that we should allow them to fail long term after
> > some FS (btrfs at least) catch up and start handling failures properly.
> 
> I see, yeah that's probably a better way to look at it.
> 
> Thanks!
> 
> Scratch my objection to this patch then. But please do add to/update
> that XXX comment above that line, or it'll be confusing. Hm?
> 
> 			/*
> 			 * XXX: Page reclaim didn't yield anything,
> 			 * and the OOM killer can't be invoked, but
> 			 * keep looping as per tradition. Unless the
> 			 * system is trying to enter a quiescent state
> 			 * during suspend and the OOM killer has been
> 			 * shut off already. Give up like with other
> 			 * !__GFP_NOFAIL allocations in that case.
> 			 */
> 			*did_some_progress = !oom_killer_disabled;

Yes this makes it more clear IMO.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Tetsuo Handa]

Michal Hocko wrote:
> > Scratch my objection to this patch then. But please do add to/update
> > that XXX comment above that line, or it'll be confusing. Hm?
> > 
> > 			/*
> > 			 * XXX: Page reclaim didn't yield anything,
> > 			 * and the OOM killer can't be invoked, but
> > 			 * keep looping as per tradition. Unless the
> > 			 * system is trying to enter a quiescent state
> > 			 * during suspend and the OOM killer has been
> > 			 * shut off already. Give up like with other
> > 			 * !__GFP_NOFAIL allocations in that case.
> > 			 */
> > 			*did_some_progress = !oom_killer_disabled;
> 
> Yes this makes it more clear IMO.
> 
If you don't want to expose oom_killer_disabled outside of the OOM proper,
can't we move this "if (!(gfp_mask & __GFP_FS)) { ... }" block to before
constraint = constrained_alloc(oc, &totalpages) line in out_of_memory() ?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Johannes Weiner]

On Tue, Jan 12, 2016 at 06:30:15AM +0900, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > > Scratch my objection to this patch then. But please do add to/update
> > > that XXX comment above that line, or it'll be confusing. Hm?
> > > 
> > > 			/*
> > > 			 * XXX: Page reclaim didn't yield anything,
> > > 			 * and the OOM killer can't be invoked, but
> > > 			 * keep looping as per tradition. Unless the
> > > 			 * system is trying to enter a quiescent state
> > > 			 * during suspend and the OOM killer has been
> > > 			 * shut off already. Give up like with other
> > > 			 * !__GFP_NOFAIL allocations in that case.
> > > 			 */
> > > 			*did_some_progress = !oom_killer_disabled;
> > 
> > Yes this makes it more clear IMO.
> > 
> If you don't want to expose oom_killer_disabled outside of the OOM proper,
> can't we move this "if (!(gfp_mask & __GFP_FS)) { ... }" block to before
> constraint = constrained_alloc(oc, &totalpages) line in out_of_memory() ?

I think your patch is fine as it is.

It's better to pull out oom_killer_disabled. We want the logic that
filters OOM invocation based on allocation type in one place. And as
per the XXX we eventually want to drop that bogus *did_some_progress
setting anyway.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Michal Hocko]

On Mon 11-01-16 17:02:16, Johannes Weiner wrote:
> On Tue, Jan 12, 2016 at 06:30:15AM +0900, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > > Scratch my objection to this patch then. But please do add to/update
> > > > that XXX comment above that line, or it'll be confusing. Hm?
> > > > 
> > > > 			/*
> > > > 			 * XXX: Page reclaim didn't yield anything,
> > > > 			 * and the OOM killer can't be invoked, but
> > > > 			 * keep looping as per tradition. Unless the
> > > > 			 * system is trying to enter a quiescent state
> > > > 			 * during suspend and the OOM killer has been
> > > > 			 * shut off already. Give up like with other
> > > > 			 * !__GFP_NOFAIL allocations in that case.
> > > > 			 */
> > > > 			*did_some_progress = !oom_killer_disabled;
> > > 
> > > Yes this makes it more clear IMO.
> > > 
> > If you don't want to expose oom_killer_disabled outside of the OOM proper,
> > can't we move this "if (!(gfp_mask & __GFP_FS)) { ... }" block to before
> > constraint = constrained_alloc(oc, &totalpages) line in out_of_memory() ?
> 
> I think your patch is fine as it is.
> 
> It's better to pull out oom_killer_disabled. We want the logic that
> filters OOM invocation based on allocation type in one place. And as
> per the XXX we eventually want to drop that bogus *did_some_progress
> setting anyway.

Completely agreed.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[David Rientjes]

On Mon, 11 Jan 2016, Tetsuo Handa wrote:

> After the OOM killer is disabled during suspend operation,
> any !__GFP_NOFAIL && __GFP_FS allocations are forced to fail.
> Thus, any !__GFP_NOFAIL && !__GFP_FS allocations should be
> forced to fail as well.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Please update this patch with the suggested comment from Johannes.  After 
that's done, feel free to add my

Acked-by: David Rientjes <rientjes@google.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Tetsuo Handa]

After the OOM killer is disabled during suspend operation,
any !__GFP_NOFAIL && __GFP_FS allocations are forced to fail.
Thus, any !__GFP_NOFAIL && !__GFP_FS allocations should be
forced to fail as well.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Michal Hocko <mhocko@suse.com>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/page_alloc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6463426..2f71caa 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2749,8 +2749,12 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
 			 * XXX: Page reclaim didn't yield anything,
 			 * and the OOM killer can't be invoked, but
 			 * keep looping as per tradition.
+			 *
+			 * But do not keep looping if oom_killer_disable()
+			 * was already called, for the system is trying to
+			 * enter a quiescent state during suspend.
 			 */
-			*did_some_progress = 1;
+			*did_some_progress = !oom_killer_disabled;
 			goto out;
 		}
 		if (pm_suspended_storage())
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,oom: do not loop !__GFP_FS allocation if the OOM killer is disabled.

[Michal Hocko]

It would really help if you CCed all people who participated in the
previous discussion... and Andrew (CCed now) to pick up the patch as
well.

On Sun 24-01-16 00:38:51, Tetsuo Handa wrote:
> After the OOM killer is disabled during suspend operation,
> any !__GFP_NOFAIL && __GFP_FS allocations are forced to fail.
> Thus, any !__GFP_NOFAIL && !__GFP_FS allocations should be
> forced to fail as well.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Acked-by: David Rientjes <rientjes@google.com>
> ---
>  mm/page_alloc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6463426..2f71caa 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2749,8 +2749,12 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  			 * XXX: Page reclaim didn't yield anything,
>  			 * and the OOM killer can't be invoked, but
>  			 * keep looping as per tradition.
> +			 *
> +			 * But do not keep looping if oom_killer_disable()
> +			 * was already called, for the system is trying to
> +			 * enter a quiescent state during suspend.
>  			 */
> -			*did_some_progress = 1;
> +			*did_some_progress = !oom_killer_disabled;
>  			goto out;
>  		}
>  		if (pm_suspended_storage())
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>