linux-kernel - Re: [patch -mm v2] mm: introduce oom_adj

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.00.0907301157100.9652@chino.kir.corp.google.com>
Date:	Thu, 30 Jul 2009 12:05:30 -0700 (PDT)
From:	David Rientjes <rientjes@...gle.com>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>,
	Paul Menage <menage@...gle.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [patch -mm v2] mm: introduce oom_adj_child

On Thu, 30 Jul 2009, KAMEZAWA Hiroyuki wrote:

> > If you have suggestions for a better name, I'd happily ack it.
> > 
> 
> Simply, reset_oom_adj_at_new_mm_context or some.
> 

I think it's preferred to keep the name relatively short which is an 
unfortuante requirement in this case.  I also prefer to start the name 
with "oom_adj" so it appears alongside /proc/pid/oom_adj when listed 
alphabetically.

> > > 2. More simple plan is like this, IIUC.
> > > 
> > >   fix oom-killer's select_bad_process() not to be in deadlock.
> > > 
> > 
> > Alternate ideas?
> > 
> At brief thiking.
> 
> 1. move oom_adj from mm_struct to signal struct. or somewhere.
>    (see copy_signal())
>    Then,
>     - all threads in a process will have the same oom_adj.
>     - vfork()'ed thread will inherit its parent's oom_adj.   
>     - vfork()'ed thread can override oom_adj of its own.
> 
>     In other words, oom_adj is shared when CLONE_PARENT is not set.
> 

Hmm, didn't we talk about signal_struct already?  The problem with that 
approach is that oom_adj values represent a killable quantity of memory, 
so having multiple threads sharing the same mm_struct with one set to 
OOM_DISABLE and the other at +15 will still livelock because the oom 
killer can't kill either.

> 2. rename  mm_struct's oom_adj as shadow_oom_adj.
> 
>    update this shadow_oom_adj as the highest oom_adj among
>    the values all threads share this mm_struct have.
>    This update is done when
>    - mm_init()
>    - oom_adj is written.
> 
>    User's 
>    # echo XXXX > /proc/<x>/oom_adj
>    is not necessary to be very very fast.
> 
>    I don't think a process which calls vfork() is multi-threaded.
> 
> 3. use shadow_oom_adj in select_bad_process().
> 

Ideas 2 & 3 here seem to be a single proposal.  The problem is that it 
still leaves /proc/pid/oom_score to be inconsistent with the badness 
scoring that the oom killer will eventually use since if it oom kills one 
task, it must kill all tasks sharing the same mm_struct to lead to future 
memory freeing.

Additionally, if you were to set one thread to OOM_DISABLE, storing the 
highest oom_adj value in mm_struct isn't going to help because 
oom_kill_task() will still require a tasklist scan to ensure no threads 
sharing the mm_struct are OOM_DISABLE and the livelock persists.

In other words, the issue here is larger than the inheritance of the 
oom_adj value amongst children, it addresses a livelock that neither of 
your approaches solve.  The fix actually makes /proc/pid/oom_adj (and 
/proc/pid/oom_score) consistent with how the oom killer behaves.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/