Re: [patch 1/2] oom: avoid killing a task if a thread sharing its mm cannot be killed

[David Rientjes <rientjes@google.com>]

On Mon, 16 Aug 2010, Oleg Nesterov wrote:

> > There's no other way to detect threads in other thread groups that share
> > the same mm since subthreads of a process can have an oom_score_adj that
> > differ from that process, this includes the possibility of
> > OOM_SCORE_ADJ_MIN that we're interested in here.
> 
> Yes, you are right. Still, at least you can do
> 
> 	for_each_process(p) {
> 		if (p->mm != mm)
> 			continue;
> 		...
> 
> to quickly skip the thread group which doesn't share the same ->mm.
> 

Right, thanks.  I'll make that optimization and send out a second version 
of this series with the other changes you suggested.

> > > > -	if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
> > > > +	if (is_mm_unfreeable(p->mm)) {
> > >
> > > oom_badness() becomes O(n**2), not good.
> > >
> >
> > No, oom_badness() becomes O(n) from O(1); select_bad_process() becomes
> > slower for eligible tasks.
> 
> I meant, select_bad_process() becomes O(n^2). oom_badness() is O(n), yes.
> 

I'll follow my own suggestion for deferring this check to 
oom_kill_process() since it's certainly an unusual case if the tasks are 
sharing memory.  It'll require a second entire tasklist scan when it 
occurs, but definitely speeds up the common case.

> > > And, more importantly. This patch makes me think ->oom_score_adj should
> > > be moved from ->signal to ->mm.
> > >
> >
> > I did that several months ago but people were unhappy with how a parent's
> > oom_score_adj value would change if it did a vfork() and the child's
> > oom_score_adj value was changed prior to execve().
> 
> I see. But this patch in essence moves OOM_SCORE_ADJ_MIN from ->signal
> to ->mm (and btw personally I think this makes sense).
> 

Yes, and I still would have liked to embed it in struct mm_struct like I 
originally proposed, but I understand how some people didn't care much for 
the vfork() inheritance problem.  There are applications in the wild such 
as job schedulers that are OOM_DISABLE themselves and fork children and 
then reset their oom_adj value prior to exec.  So they do vfork() -> 
change child's oom_adj -> execve().  That currently works since the 
child's ->signal isn't shared (and before that, oom_adj was embedded in 
struct task_struct) and we can't change that behavior to also change the 
parent's oom_adj value at the same time because it shares an ->mm out from 
under them.

Thanks for reviewing the patches Oleg!

--
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>