Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1340116 > unrolled thread

Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they  are OOM-unkillable. David Rientjes <rientjes@google.com> - 2016-02-23 02:10 +0100
    Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are  OOM-unkillable. Michal Hocko <mhocko@kernel.org> - 2016-02-23 13:40 +0100
      Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they  are OOM-unkillable. David Rientjes <rientjes@google.com> - 2016-02-23 23:40 +0100
        Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are  OOM-unkillable. Michal Hocko <mhocko@kernel.org> - 2016-02-24 11:10 +0100
          Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they  are OOM-unkillable. David Rientjes <rientjes@google.com> - 2016-02-24 22:40 +0100

#1340116 — Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

On Thu, 18 Feb 2016, Michal Hocko wrote:

> > Anyway, this is NACK'd since task->signal->oom_score_adj is checked under 
> > task_lock() for threads with memory attached, that's the purpose of 
> > finding the correct thread in oom_badness() and taking task_lock().  We 
> > aren't going to duplicate logic in several functions that all do the same 
> > thing.
> 
> Is the task_lock really necessary, though? E.g. oom_task_origin()
> doesn't seem to depend on it for task->signal safety. If you are
> referring to races with changing oom_score_adj does such a race matter
> at all?
> 

oom_badness() ranges from 0 (don't kill) to 1000 (please kill).  It 
factors in the setting of /proc/self/oom_score_adj to change that value.  
That is where OOM_SCORE_ADJ_MIN is enforced.  It is also needed in 
oom_badness() to determine whether a child process should be sacrificed 
for its parent.  We don't add duplicate logic everywhere if you want the 
code to be maintainable; the only exception would be for performance 
critical code which the oom killer most certainly is not.

I'm simply not entertaining any patch to the oom killer that duplicates 
code everywhere, increases its complexity, makes it grow in text size, and 
makes it more difficult to maintain.

[toc] | [next] | [standalone]

#1340590 — Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

On Mon 22-02-16 17:06:29, David Rientjes wrote:
> On Thu, 18 Feb 2016, Michal Hocko wrote:
> 
> > > Anyway, this is NACK'd since task->signal->oom_score_adj is checked under 
> > > task_lock() for threads with memory attached, that's the purpose of 
> > > finding the correct thread in oom_badness() and taking task_lock().  We 
> > > aren't going to duplicate logic in several functions that all do the same 
> > > thing.
> > 
> > Is the task_lock really necessary, though? E.g. oom_task_origin()
> > doesn't seem to depend on it for task->signal safety. If you are
> > referring to races with changing oom_score_adj does such a race matter
> > at all?
> > 
> 
> oom_badness() ranges from 0 (don't kill) to 1000 (please kill).  It 
> factors in the setting of /proc/self/oom_score_adj to change that value.  
> That is where OOM_SCORE_ADJ_MIN is enforced. 

The question is whether the current placement of OOM_SCORE_ADJ_MIN
is appropriate. Wouldn't it make more sense to check it in oom_unkillable_task
instead? Sure, checking oom_score_adj under task_lock inside oom_badness will
prevent from races but the question I raised previously was whether we
actually care about those races? When would it matter? Is it really
likely that the update happen during the oom killing? And if yes what
prevents from the update happening _after_ the check?

If for nothing else oom_unkillable_task would be complete that way. E.g.
sysctl_oom_kill_allocating_task has to check for OOM_SCORE_ADJ_MIN
because it doesn't rely on oom_badness and that alone would suggest
that the check is misplaced.

That being said I do not really care that much. I would just find it
neater to have oom_unkillable_task that would really consider all the
cases where the OOM should ignore a task.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1341144

On Tue, 23 Feb 2016, Michal Hocko wrote:

> > oom_badness() ranges from 0 (don't kill) to 1000 (please kill).  It 
> > factors in the setting of /proc/self/oom_score_adj to change that value.  
> > That is where OOM_SCORE_ADJ_MIN is enforced. 
> 
> The question is whether the current placement of OOM_SCORE_ADJ_MIN
> is appropriate. Wouldn't it make more sense to check it in oom_unkillable_task
> instead?

oom_unkillable_task() deals with the type of task it is (init or kthread) 
or being ineligible due to the memcg and cpuset placement.  We want to 
exclude them from consideration and also suppress them from the task dump 
in the kernel log.  We don't want to suppress oom disabled processes, we 
really want to know their rss, for example.  It could be renamed 
is_ineligible_task().

> Sure, checking oom_score_adj under task_lock inside oom_badness will
> prevent from races but the question I raised previously was whether we
> actually care about those races? When would it matter? Is it really
> likely that the update happen during the oom killing? And if yes what
> prevents from the update happening _after_ the check?
> 

It's not necessarily to take task_lock(), but find_lock_task_mm() is the 
means we have to iterate threads to find any with memory attached.  We 
need that logic in oom_badness() to avoid racing with threads that have 
entered exit_mm().  It's possible for a thread to have a non-NULL ->mm in 
oom_scan_process_thread(), the thread enters exit_mm() without kill, and 
oom_badness() can still find it to be eligible because other threads have 
not exited.  We still want to issue a kill to this process and task_lock() 
protects the setting of task->mm to NULL: don't consider it to be a race 
in setting oom_score_adj, consider it to be a race in unmapping (but not 
freeing) memory in th exit path.

[toc] | [prev] | [next] | [standalone]

#1341729 — Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

On Tue 23-02-16 14:33:01, David Rientjes wrote:
> On Tue, 23 Feb 2016, Michal Hocko wrote:
> 
> > > oom_badness() ranges from 0 (don't kill) to 1000 (please kill).  It 
> > > factors in the setting of /proc/self/oom_score_adj to change that value.  
> > > That is where OOM_SCORE_ADJ_MIN is enforced. 
> > 
> > The question is whether the current placement of OOM_SCORE_ADJ_MIN
> > is appropriate. Wouldn't it make more sense to check it in oom_unkillable_task
> > instead?
> 
> oom_unkillable_task() deals with the type of task it is (init or kthread) 
> or being ineligible due to the memcg and cpuset placement.

Yes and OOM disabled is yet another condition.

> We want to 
> exclude them from consideration and also suppress them from the task dump 
> in the kernel log.  We don't want to suppress oom disabled processes, we 
> really want to know their rss, for example.

Hmm, is it really helpful though? What would you deduce from seeing a
large rss an OOM_SCORE_ADJ_MIN task? Misconfigured system? There must
have been a reason to mark the task that way in the first place so you
can hardly do anything about it. Moreover you can deduce the same from
the available information.

I would even argue that displaying OOM_SCORE_ADJ_MIN might be a bit
counterproductive because you have to filter them out when looking at
the listing.

> It could be renamed is_ineligible_task().

That wouldn't really help imho because OOM_SCORE_ADJ_MIN is an
uneligible task.

> > Sure, checking oom_score_adj under task_lock inside oom_badness will
> > prevent from races but the question I raised previously was whether we
> > actually care about those races? When would it matter? Is it really
> > likely that the update happen during the oom killing? And if yes what
> > prevents from the update happening _after_ the check?
> > 
> 
> It's not necessarily to take task_lock(), but find_lock_task_mm() is the 
> means we have to iterate threads to find any with memory attached.  We 
> need that logic in oom_badness() to avoid racing with threads that have 
> entered exit_mm().  It's possible for a thread to have a non-NULL ->mm in 
> oom_scan_process_thread(), the thread enters exit_mm() without kill, and 
> oom_badness() can still find it to be eligible because other threads have 
> not exited.  We still want to issue a kill to this process and task_lock() 
> protects the setting of task->mm to NULL: don't consider it to be a race 
> in setting oom_score_adj, consider it to be a race in unmapping (but not 
> freeing) memory in th exit path.

I am confused now. This all is true but it is independent on OOM_SCORE_ADJ_MIN
check? The check is per signal_struct so checking all the threads will
not change anything.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1342456

On Wed, 24 Feb 2016, Michal Hocko wrote:

> Hmm, is it really helpful though? What would you deduce from seeing a
> large rss an OOM_SCORE_ADJ_MIN task? Misconfigured system? There must
> have been a reason to mark the task that way in the first place so you
> can hardly do anything about it. Moreover you can deduce the same from
> the available information.
> 

Users run processes that are vital to the machine with OOM_SCORE_ADJ_MIN.  
This does not make them immune to having memory leaks that caused the oom 
condition, and identifying that has triaged many bugs in the past.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web