oom: use same_thread_group instead comparing ->mm
in [Kernel]
|
Prev: [PATCH] cred - synchronize rcu before releasing cred
Next: hmc6352: Add driver for the HMC6352 compass
From: Oleg Nesterov on 16 Jun 2010 08:30 On 06/16, KOSAKI Motohiro wrote: > > Now, oom are using "child->mm != p->mm" check to distinguish subthread. Heh. is it true??? I never undestood what oom_kill_process()->list_for_each_entry() is supposed to do. > But It's incorrect. vfork() child also have the same ->mm. Yes. > This patch change to use same_thread_group() instead. I don't think we need same_thread_group(). Please note that any children must be from the different thread_group. So, > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -161,7 +161,7 @@ unsigned long oom_badness(struct task_struct *p, unsigned long uptime) > list_for_each_entry(c, &t->children, sibling) { > child = find_lock_task_mm(c); > if (child) { > - if (child->mm != p->mm) > + if (same_thread_group(p, child)) > points += child->mm->total_vm/2 + 1; > task_unlock(child); > } > @@ -486,7 +486,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order, > list_for_each_entry(child, &t->children, sibling) { > unsigned long child_points; > > - if (child->mm == p->mm) > + if (same_thread_group(p, child)) > continue; In both cases same_thread_group() must be false. This means that the change in oom_badness() doesn't look right, "child->mm != p->mm" is the correct check to decide whether we should account child->mm. The change in oom_kill_process() merely removes this "continue". Could someone please explain what this code _should_ do? Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Minchan Kim on 16 Jun 2010 11:20 On Wed, Jun 16, 2010 at 08:34:02PM +0900, KOSAKI Motohiro wrote: > Now, oom are using "child->mm != p->mm" check to distinguish subthread. > But It's incorrect. vfork() child also have the same ->mm. > > This patch change to use same_thread_group() instead. Hmm. I think we don't use it to distinguish subthread. We use it for finding child process which is not vforked. I can't understand your point. > > Cc: Oleg Nesterov <oleg(a)redhat.com> > Signed-off-by: KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com> > --- > mm/oom_kill.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 12204c7..e4b1146 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -161,7 +161,7 @@ unsigned long oom_badness(struct task_struct *p, unsigned long uptime) > list_for_each_entry(c, &t->children, sibling) { > child = find_lock_task_mm(c); > if (child) { > - if (child->mm != p->mm) > + if (same_thread_group(p, child)) > points += child->mm->total_vm/2 + 1; > task_unlock(child); > } > @@ -486,7 +486,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order, > list_for_each_entry(child, &t->children, sibling) { > unsigned long child_points; > > - if (child->mm == p->mm) > + if (same_thread_group(p, child)) > continue; > if (oom_unkillable_task(child, mem, nodemask)) > continue; > -- > 1.6.5.2 > > > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo(a)kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont(a)kvack.org"> email(a)kvack.org </a> -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: KOSAKI Motohiro on 16 Jun 2010 22:00 > On Wed, Jun 16, 2010 at 08:34:02PM +0900, KOSAKI Motohiro wrote: > > Now, oom are using "child->mm != p->mm" check to distinguish subthread. > > But It's incorrect. vfork() child also have the same ->mm. > > > > This patch change to use same_thread_group() instead. > > Hmm. I think we don't use it to distinguish subthread. > We use it for finding child process which is not vforked. > > I can't understand your point. Thank you. my fault. respin. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: KOSAKI Motohiro on 16 Jun 2010 22:00
> On 06/16, KOSAKI Motohiro wrote: > > > > Now, oom are using "child->mm != p->mm" check to distinguish subthread. > > Heh. is it true??? I never undestood what oom_kill_process()->list_for_each_entry() > is supposed to do. I guessed. true history was gone long time ago ;) ok, I'll remove dubious guess. > > But It's incorrect. vfork() child also have the same ->mm. > > Yes. > > > This patch change to use same_thread_group() instead. > > I don't think we need same_thread_group(). Please note that any children must > be from the different thread_group. Agghh. I see. ok, probably, I've got correct original author intention now. To be honest, andrea's ancient patch is very hard to understand for me ;) > > So, > > > --- a/mm/oom_kill.c > > +++ b/mm/oom_kill.c > > @@ -161,7 +161,7 @@ unsigned long oom_badness(struct task_struct *p, unsigned long uptime) > > list_for_each_entry(c, &t->children, sibling) { > > child = find_lock_task_mm(c); > > if (child) { > > - if (child->mm != p->mm) > > + if (same_thread_group(p, child)) > > points += child->mm->total_vm/2 + 1; > > task_unlock(child); > > } > > @@ -486,7 +486,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order, > > list_for_each_entry(child, &t->children, sibling) { > > unsigned long child_points; > > > > - if (child->mm == p->mm) > > + if (same_thread_group(p, child)) > > continue; > > In both cases same_thread_group() must be false. > > This means that the change in oom_badness() doesn't look right, > "child->mm != p->mm" is the correct check to decide whether we should > account child->mm. > > The change in oom_kill_process() merely removes this "continue". > Could someone please explain what this code _should_ do? I think you are right. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |