Re: [patch v3 3/3] mm, oom: do not panic for oom kills triggered from sysrq

Re: [patch v3 3/3] mm, oom: do not panic for oom kills triggered from sysrq

[Hillf Danton]

> Sysrq+f is used to kill a process either for debug or when the VM is
> otherwise unresponsive.
> 
> It is not intended to trigger a panic when no process may be killed.
> 
> Avoid panicking the system for sysrq+f when no processes are killed.
> 
> Suggested-by: Michal Hocko <mhocko@suse.cz>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  v2: no change
>  v3: fix title per Hillf
> 
>  Documentation/sysrq.txt | 3 ++-
>  mm/oom_kill.c           | 7 +++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/sysrq.txt b/Documentation/sysrq.txt
> --- a/Documentation/sysrq.txt
> +++ b/Documentation/sysrq.txt
> @@ -75,7 +75,8 @@ On all -  write a character to /proc/sysrq-trigger.  e.g.:
> 
>  'e'     - Send a SIGTERM to all processes, except for init.
> 
> -'f'	- Will call oom_kill to kill a memory hog process.
> +'f'	- Will call the oom killer to kill a memory hog process, but do not
> +	  panic if nothing can be killed.
> 
>  'g'	- Used by kgdb (kernel debugger)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -607,6 +607,9 @@ void check_panic_on_oom(struct oom_control *oc, enum oom_constraint constraint,
>  		if (constraint != CONSTRAINT_NONE)
>  			return;
>  	}
> +	/* Do not panic for oom kills triggered by sysrq */
> +	if (oc->order == -1)
> +		return;
>  	dump_header(oc, NULL, memcg);
>  	panic("Out of memory: %s panic_on_oom is enabled\n",
>  		sysctl_panic_on_oom == 2 ? "compulsory" : "system-wide");
> @@ -686,11 +689,11 @@ bool out_of_memory(struct oom_control *oc)
> 
>  	p = select_bad_process(oc, &points, totalpages);
>  	/* Found nothing?!?! Either we hang forever, or we panic. */
> -	if (!p) {
> +	if (!p && oc->order != -1) {
>  		dump_header(oc, NULL, NULL);
>  		panic("Out of memory and no killable processes...\n");
>  	}

Given sysctl_panic_on_oom checked, AFAICU there seems
no chance for panic, no matter -1 or not.

> -	if (p != (void *)-1UL) {
> +	if (p && p != (void *)-1UL) {
>  		oom_kill_process(oc, p, points, totalpages, NULL,
>  				 "Out of memory");
>  		killed = 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 v3 3/3] mm, oom: do not panic for oom kills triggered from sysrq

[David Rientjes]

On Thu, 9 Jul 2015, Hillf Danton wrote:

> > diff --git a/Documentation/sysrq.txt b/Documentation/sysrq.txt
> > --- a/Documentation/sysrq.txt
> > +++ b/Documentation/sysrq.txt
> > @@ -75,7 +75,8 @@ On all -  write a character to /proc/sysrq-trigger.  e.g.:
> > 
> >  'e'     - Send a SIGTERM to all processes, except for init.
> > 
> > -'f'	- Will call oom_kill to kill a memory hog process.
> > +'f'	- Will call the oom killer to kill a memory hog process, but do not
> > +	  panic if nothing can be killed.
> > 
> >  'g'	- Used by kgdb (kernel debugger)
> > 
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -607,6 +607,9 @@ void check_panic_on_oom(struct oom_control *oc, enum oom_constraint constraint,
> >  		if (constraint != CONSTRAINT_NONE)
> >  			return;
> >  	}
> > +	/* Do not panic for oom kills triggered by sysrq */
> > +	if (oc->order == -1)
> > +		return;
> >  	dump_header(oc, NULL, memcg);
> >  	panic("Out of memory: %s panic_on_oom is enabled\n",
> >  		sysctl_panic_on_oom == 2 ? "compulsory" : "system-wide");
> > @@ -686,11 +689,11 @@ bool out_of_memory(struct oom_control *oc)
> > 
> >  	p = select_bad_process(oc, &points, totalpages);
> >  	/* Found nothing?!?! Either we hang forever, or we panic. */
> > -	if (!p) {
> > +	if (!p && oc->order != -1) {
> >  		dump_header(oc, NULL, NULL);
> >  		panic("Out of memory and no killable processes...\n");
> >  	}
> 
> Given sysctl_panic_on_oom checked, AFAICU there seems
> no chance for panic, no matter -1 or not.
> 

I'm not sure I understand your point.

There are two oom killer panics: when panic_on_oom is enabled and when the 
oom killer can't find an eligible process.

The change to the panic_on_oom panic is dealt with in check_panic_on_oom() 
and the no eligible process panic is dealt with here.

If the sysctl is disabled, and there are no eligible processes to kill, 
the change in behavior here is that we don't panic when triggered from 
sysrq.  That's the change in the hunk above.

> > -	if (p != (void *)-1UL) {
> > +	if (p && p != (void *)-1UL) {
> >  		oom_kill_process(oc, p, points, totalpages, NULL,
> >  				 "Out of memory");
> >  		killed = 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 v3 3/3] mm, oom: do not panic for oom kills triggered from sysrq

[Hillf Danton]

> > > diff --git a/Documentation/sysrq.txt b/Documentation/sysrq.txt
> > > --- a/Documentation/sysrq.txt
> > > +++ b/Documentation/sysrq.txt
> > > @@ -75,7 +75,8 @@ On all -  write a character to /proc/sysrq-trigger.  e.g.:
> > >
> > >  'e'     - Send a SIGTERM to all processes, except for init.
> > >
> > > -'f'	- Will call oom_kill to kill a memory hog process.
> > > +'f'	- Will call the oom killer to kill a memory hog process, but do not
> > > +	  panic if nothing can be killed.
> > >
> > >  'g'	- Used by kgdb (kernel debugger)
> > >
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -607,6 +607,9 @@ void check_panic_on_oom(struct oom_control *oc, enum oom_constraint constraint,
> > >  		if (constraint != CONSTRAINT_NONE)
> > >  			return;
> > >  	}
> > > +	/* Do not panic for oom kills triggered by sysrq */
> > > +	if (oc->order == -1)
> > > +		return;
> > >  	dump_header(oc, NULL, memcg);
> > >  	panic("Out of memory: %s panic_on_oom is enabled\n",
> > >  		sysctl_panic_on_oom == 2 ? "compulsory" : "system-wide");
> > > @@ -686,11 +689,11 @@ bool out_of_memory(struct oom_control *oc)
> > >
> > >  	p = select_bad_process(oc, &points, totalpages);
> > >  	/* Found nothing?!?! Either we hang forever, or we panic. */
> > > -	if (!p) {
> > > +	if (!p && oc->order != -1) {
> > >  		dump_header(oc, NULL, NULL);
> > >  		panic("Out of memory and no killable processes...\n");
> > >  	}
> >
> > Given sysctl_panic_on_oom checked, AFAICU there seems
> > no chance for panic, no matter -1 or not.
> >
> 
> I'm not sure I understand your point.
> 
> There are two oom killer panics: when panic_on_oom is enabled and when the
> oom killer can't find an eligible process.
> 
> The change to the panic_on_oom panic is dealt with in check_panic_on_oom()
> and the no eligible process panic is dealt with here.
> 
> If the sysctl is disabled, and there are no eligible processes to kill,
> the change in behavior here is that we don't panic when triggered from
> sysrq.  That's the change in the hunk above.
> 
When no eligible processes is selected to kill, we are sure that we skip one
panic in check_panic_on_oom(), and we have no clear reason to panic again.

But we can simply answer the caller that there is no page, and let her
decide what to do.

So I prefer to fold the two panic into one.

Hillf
> > > -	if (p != (void *)-1UL) {
> > > +	if (p && p != (void *)-1UL) {
> > >  		oom_kill_process(oc, p, points, totalpages, NULL,
> > >  				 "Out of memory");
> > >  		killed = 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 v3 3/3] mm, oom: do not panic for oom kills triggered from sysrq

[David Rientjes]

On Fri, 10 Jul 2015, Hillf Danton wrote:

> > I'm not sure I understand your point.
> > 
> > There are two oom killer panics: when panic_on_oom is enabled and when the
> > oom killer can't find an eligible process.
> > 
> > The change to the panic_on_oom panic is dealt with in check_panic_on_oom()
> > and the no eligible process panic is dealt with here.
> > 
> > If the sysctl is disabled, and there are no eligible processes to kill,
> > the change in behavior here is that we don't panic when triggered from
> > sysrq.  That's the change in the hunk above.
> > 
> When no eligible processes is selected to kill, we are sure that we skip one
> panic in check_panic_on_oom(), and we have no clear reason to panic again.
> 
> But we can simply answer the caller that there is no page, and let her
> decide what to do.
> 
> So I prefer to fold the two panic into one.
> 
> Hillf
> > > > -	if (p != (void *)-1UL) {
> > > > +	if (p && p != (void *)-1UL) {
> > > >  		oom_kill_process(oc, p, points, totalpages, NULL,
> > > >  				 "Out of memory");
> > > >  		killed = 1;
> 

I'm still not sure I understand your point, unfortunately.  The new check:

	if (!p && oc->order != -1) {
		dump_header(oc, NULL, NULL);
		panic("Out of memory and no killable processes...\n");
	}

ensures we never panic when called from sysrq.  This is done because 
userspace can easily race when there is a single eligible process to kill 
that exits or is otherwise killed and the sysrq+f ends up panicking the 
machine unexpectedly.

--
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 v3 3/3] mm, oom: do not panic for oom kills triggered from sysrq

[David Rientjes]

Sysrq+f is used to kill a process either for debug or when the VM is
otherwise unresponsive.

It is not intended to trigger a panic when no process may be killed.

Avoid panicking the system for sysrq+f when no processes are killed.

Suggested-by: Michal Hocko <mhocko@suse.cz>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 v2: no change
 v3: fix title per Hillf

 Documentation/sysrq.txt | 3 ++-
 mm/oom_kill.c           | 7 +++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/sysrq.txt b/Documentation/sysrq.txt
--- a/Documentation/sysrq.txt
+++ b/Documentation/sysrq.txt
@@ -75,7 +75,8 @@ On all -  write a character to /proc/sysrq-trigger.  e.g.:
 
 'e'     - Send a SIGTERM to all processes, except for init.
 
-'f'	- Will call oom_kill to kill a memory hog process.
+'f'	- Will call the oom killer to kill a memory hog process, but do not
+	  panic if nothing can be killed.
 
 'g'	- Used by kgdb (kernel debugger)
 
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -607,6 +607,9 @@ void check_panic_on_oom(struct oom_control *oc, enum oom_constraint constraint,
 		if (constraint != CONSTRAINT_NONE)
 			return;
 	}
+	/* Do not panic for oom kills triggered by sysrq */
+	if (oc->order == -1)
+		return;
 	dump_header(oc, NULL, memcg);
 	panic("Out of memory: %s panic_on_oom is enabled\n",
 		sysctl_panic_on_oom == 2 ? "compulsory" : "system-wide");
@@ -686,11 +689,11 @@ bool out_of_memory(struct oom_control *oc)
 
 	p = select_bad_process(oc, &points, totalpages);
 	/* Found nothing?!?! Either we hang forever, or we panic. */
-	if (!p) {
+	if (!p && oc->order != -1) {
 		dump_header(oc, NULL, NULL);
 		panic("Out of memory and no killable processes...\n");
 	}
-	if (p != (void *)-1UL) {
+	if (p && p != (void *)-1UL) {
 		oom_kill_process(oc, p, points, totalpages, NULL,
 				 "Out of memory");
 		killed = 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>