Re: [patch v2 2/2] oom: kill all threads sharing oom killed task's mm

[David Rientjes <rientjes@google.com>]

On Fri, 20 Aug 2010, KOSAKI Motohiro wrote:

> > > Why don't you use pthread library? Is there any good reason? That said,
> > > If you are trying to optimize neither thread nor vfork case, I'm not charmed
> > > this because 99.99% user don't use it. but even though every user will get 
> > > performance degression. Can you please consider typical use case optimization?
> > > 
> > 
> > Non-NPTL threaded applications exist in the wild, and I can't change that.  
> 
> Which application? If it is major opensource application, I think this one
> makes end user happy.
> 

Being a "major opensource application" is not the requirement to prevent a 
livelock in the kernel.  The livelock exists, we hit it all the time, and 
your change to remove this code from the oom killer caused a regression.

Unless you can guarantee that a thread holding mm->mmap_sem cannot loop in 
the page allocator, then we need a way for that allocation to succeed.

> > This mm->mmap_sem livelock is a problem for them, we've hit it internally 
> > (we're forced to carry this patch internally), and we aren't the only ones 
> > running at least some non-NPTL apps.  That's why this code existed in the 
> > oom killer for over eight years since the 2.4 kernel before you removed it 
> > based on the mempolicy policy of killing current, which has since been 
> > obsoleted.  Until CLONE_VM without CLONE_THREAD is prohibited entirely on 
> > Linux, this livelock can exist.
> 
> Or, can you eliminate O(n^2) thing? The cost is enough low, I don't oppose 
> this.
> 

Suggestions on improvements are welcome in the form of a patch.

> > Users who do not want the tasklist scan here, which only iterates over 
> > thread group leaders and not threads, can enable 
> > /proc/sys/vm/oom_kill_allocating_task.  That's its whole purpose.  Other 
> > than that, the oom killer will never be the most efficient part of the 
> > kernel and doing for_each_process() is much less expensive than all the 
> > task_lock()s we take already.
> 
> No.
> Please don't forget typical end user don't use any kernel knob. kernel knob
> is not a way for developer excuse.
> 

Users who find tasklist scans with for_each_process() in the oom killer 
are a very, very special case and is typically only SGI.  They requested 
that the sysctl be added years ago to prevent the lengthy scan, so they 
are already protected.

Unless you can propose a different fix for the regression that you 
introduced with 8c5cd6f3 and fix the livelock, this type of check is 
mandatory.

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