[PATCH 4.3-rc6] proc: fix oom_adj value read from /proc/<pid>/oom_adj

[PATCH 4.3-rc6] proc: fix oom_adj value read from /proc/<pid>/oom_adj

[Hongjie Fang (方洪杰)]

The oom_adj's value reading through /proc/<pid>/oom_adj is different 
with the value written into /proc/<pid>/oom_adj. 
Fix this by adding a adjustment factor.

Signed-off-by: Hongjie Fang <hongjie.fang@spreadtrum.com>
---
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b25eee4..1ea0589 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1043,6 +1043,7 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf,
 	int oom_adj;
 	unsigned long flags;
 	int err;
+	int adjust;
 
 	memset(buffer, 0, sizeof(buffer));
 	if (count > sizeof(buffer) - 1)
@@ -1084,8 +1085,10 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf,
 	 */
 	if (oom_adj == OOM_ADJUST_MAX)
 		oom_adj = OOM_SCORE_ADJ_MAX;
-	else
-		oom_adj = (oom_adj * OOM_SCORE_ADJ_MAX) / -OOM_DISABLE;
+	else{
+		adjust = oom_adj > 0 ? (-OOM_DISABLE-1) : -(-OOM_DISABLE-1);
+		oom_adj = (oom_adj * OOM_SCORE_ADJ_MAX + adjust) / -OOM_DISABLE;
+	}
 
 	if (oom_adj < task->signal->oom_score_adj &&
 	    !capable(CAP_SYS_RESOURCE)) {

Re: [PATCH 4.3-rc6] proc: fix oom_adj value read from /proc/<pid>/oom_adj

[Eric W. Biederman]

"Hongjie Fang (方洪杰)" <Hongjie.Fang@spreadtrum.com> writes:

> The oom_adj's value reading through /proc/<pid>/oom_adj is different 
> with the value written into /proc/<pid>/oom_adj.
> Fix this by adding a adjustment factor.

*Scratches my head*

Won't changing the interpretation of what is written break existing
userspace applications that write this value?

Added a few more likely memory management suspects that might understand
what is going on here.

Eric

>
> Signed-off-by: Hongjie Fang <hongjie.fang@spreadtrum.com>
> ---
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index b25eee4..1ea0589 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1043,6 +1043,7 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf,
>  	int oom_adj;
>  	unsigned long flags;
>  	int err;
> +	int adjust;
>  
>  	memset(buffer, 0, sizeof(buffer));
>  	if (count > sizeof(buffer) - 1)
> @@ -1084,8 +1085,10 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf,
>  	 */
>  	if (oom_adj == OOM_ADJUST_MAX)
>  		oom_adj = OOM_SCORE_ADJ_MAX;
> -	else
> -		oom_adj = (oom_adj * OOM_SCORE_ADJ_MAX) / -OOM_DISABLE;
> +	else{
> +		adjust = oom_adj > 0 ? (-OOM_DISABLE-1) : -(-OOM_DISABLE-1);
> +		oom_adj = (oom_adj * OOM_SCORE_ADJ_MAX + adjust) / -OOM_DISABLE;
> +	}
>  
>  	if (oom_adj < task->signal->oom_score_adj &&
>  	    !capable(CAP_SYS_RESOURCE)) {
>
> --

Re: [PATCH 4.3-rc6] proc: fix oom_adj value read from /proc/<pid>/oom_adj

[Michal Hocko]

[CC David as well]

The original patch has been posted here:
http://lkml.kernel.org/r/65a10261038346b1a778443fd15f0980%40SHMBX01.spreadtrum.com

On Tue 20-10-15 12:27:58, Eric W. Biederman wrote:
> "Hongjie Fang (方洪杰)" <Hongjie.Fang@spreadtrum.com> writes:
> 
> > The oom_adj's value reading through /proc/<pid>/oom_adj is different 
> > with the value written into /proc/<pid>/oom_adj.
> > Fix this by adding a adjustment factor.
> 
> *Scratches my head*
> 
> Won't changing the interpretation of what is written break existing
> userspace applications that write this value?

No, because they will see the same value they wrote. The current state
is broken because you get a different value than you wrote.

I am just wondering, how have you found this problem? Code review or
have you encountered a real failure because of this?

> Added a few more likely memory management suspects that might understand
> what is going on here.
> 
> Eric
> 
> >
> > Signed-off-by: Hongjie Fang <hongjie.fang@spreadtrum.com>
> > ---
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index b25eee4..1ea0589 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1043,6 +1043,7 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf,
> >  	int oom_adj;
> >  	unsigned long flags;
> >  	int err;
> > +	int adjust;

This doesn't need the function visibility.

> >  
> >  	memset(buffer, 0, sizeof(buffer));
> >  	if (count > sizeof(buffer) - 1)
> > @@ -1084,8 +1085,10 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf,
> >  	 */
> >  	if (oom_adj == OOM_ADJUST_MAX)
> >  		oom_adj = OOM_SCORE_ADJ_MAX;
> > -	else
> > -		oom_adj = (oom_adj * OOM_SCORE_ADJ_MAX) / -OOM_DISABLE;
> > +	else{

space after else and checkpatch will probably complain about missing { }
for if...

Other than that the patch looks good to me. The changelog coul be
slightly improved as well.

> > +		adjust = oom_adj > 0 ? (-OOM_DISABLE-1) : -(-OOM_DISABLE-1);
> > +		oom_adj = (oom_adj * OOM_SCORE_ADJ_MAX + adjust) / -OOM_DISABLE;
> > +	}
> >  
> >  	if (oom_adj < task->signal->oom_score_adj &&
> >  	    !capable(CAP_SYS_RESOURCE)) {
> >
> > --
> 
> --
> 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

Re: [PATCH 4.3-rc6] proc: fix oom_adj value read from /proc/<pid>/oom_adj

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1212 bytes --]

On Tue, 20 Oct 2015, Hongjie Fang (方洪杰) wrote:

> The oom_adj's value reading through /proc/<pid>/oom_adj is different 
> with the value written into /proc/<pid>/oom_adj. 

/proc/pid/oom_adj is deprecated and has been for years.  When writing to 
/proc/pid/oom_adj, for legacy purposes, the value is converted to 
/proc/pid/oom_score_adj.  There is no exact way to do this since the 
scales of the tunables are different (the former acted as a simple bit 
shift on a badness score, the latter is a proportion of available memory).

You'll notice we never store the written oom_adj, and that's because after 
the conversion to oom_score_adj is done, in units the oom killer actually 
uses to make killing decisions, it is no longer interesting.  Userspace 
needs to only know what the effective policy is, and that may be different 
because there is no 1:1 mapping for tunables of different units.

Rounding up positive oom_adj values and rounding down negative oom_adj 
values, as your patch does, creates an inconsistency in how the mapping 
has been done for years.  It risks current users biasing against their 
processes more than expected, so it's not a safe change to make as Eric 
also suggested.