Prev: [PATCH] oom: Make coredump interruptible
Next: mac8390: raise error logging priority
From: KOSAKI Motohiro on 2 Jun 2010 20:50
> On Thu, Jun 3, 2010 at 9:06 AM, KOSAKI Motohiro
> <kosaki.motohiro(a)jp.fujitsu.com> wrote:
> > Hi
> >
> >> > @@ -344,35 +344,30 @@ static struct task_struct *select_bad_process(unsigned long *ppoints,
> >> >   */
> >> >  static void dump_tasks(const struct mem_cgroup *mem)
> >> >  {
> >> > -   struct task_struct *g, *p;
> >> > +   struct task_struct *p;
> >> > +   struct task_struct *task;
> >> >
> >> >     printk(KERN_INFO "[ pid ]   uid  tgid total_vm      rss cpu oom_adj "
> >> >            "name\n");
> >> > -   do_each_thread(g, p) {
> >> > +
> >> > +   for_each_process(p) {
> >> >             struct mm_struct *mm;
> >> >
> >> > -           if (mem && !task_in_mem_cgroup(p, mem))
> >> > +           if (is_global_init(p) || (p->flags & PF_KTHREAD))
> >>
> >> select_bad_process needs is_global_init check to not select init as victim.
> >> But in this case, it is just for dumping information of tasks.
> >
> > But dumping oom unrelated process is useless and making confusion.
> > Do you have any suggestion? Instead, adding unkillable field?
>
> I think it's not unrelated. Of course, init process doesn't consume
> lots of memory but might consume more memory than old as time goes by
> or some BUG although it is unlikely.
>
> I think whether we print information of init or not isn't a big deal.
> But we have been done it until now and you are trying to change it.
> At least, we need some description why you want to remove it.
> Making confusion? Hmm.. I don't think it make many people confusion.

Hm. ok, I'll change logic as you said.



> >> > -           mm = p->mm;
> >> > -           if (!mm) {
> >> > -                   /*
> >> > -                    * total_vm and rss sizes do not exist for tasks with no
> >> > -                    * mm so there's no need to report them; they can't be
> >> > -                    * oom killed anyway.
> >> > -                    */
> >>
> >> Please, do not remove the comment for mm newbies unless you think it's useless.
> >
> > How is this?
> >
> >               task = find_lock_task_mm(p);
> >               if (!task)
> >                        /*
> >                         * Probably oom vs task-exiting race was happen and ->mm
> >                         * have been detached. thus there's no need to report them;
> >                         * they can't be oom killed anyway.
> >                         */
> >                        continue;
> >
>
> Looks good to adding story about racing. but my point was "total_vm
> and rss sizes do not exist for tasks with no mm". But I don't want to
> bother you due to trivial.
> It depends on you. :)


old ->mm check have two intention.

a) the task is kernel thread?
b) the task have alredy detached ->mm

but a) is not strictly correct check because we should think use_mm().
therefore we appended PF_KTHREAD check. then, here find_lock_task_mm()
focus exiting race, I think.




--
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: KAMEZAWA Hiroyuki on 3 Jun 2010 02:40
On Thu, 3 Jun 2010 15:24:36 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com> wrote:

> dump_task() should use find_lock_task_mm() too. It is necessary for
> protecting task-exiting race.
>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com>
Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>

--
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: Oleg Nesterov on 3 Jun 2010 11:30
On 06/03, KOSAKI Motohiro wrote:
>
> dump_task() should use find_lock_task_mm() too. It is necessary for
> protecting task-exiting race.

This patch also replaces the pointless do_each_thread() with
for_each_process(), good.

Reviewed-by: Oleg Nesterov <oleg(a)redhat.com>

> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com>
> ---
> mm/oom_kill.c | 37 ++++++++++++++++++++-----------------
> 1 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 35a2ecc..6360c56 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -345,35 +345,38 @@ static struct task_struct *select_bad_process(unsigned long *ppoints,
> */
> static void dump_tasks(const struct mem_cgroup *mem)
> {
> - struct task_struct *g, *p;
> + struct task_struct *p;
> + struct task_struct *task;
>
> printk(KERN_INFO "[ pid ] uid tgid total_vm rss cpu oom_adj "
> "name\n");
> - do_each_thread(g, p) {
> - struct mm_struct *mm;
>
> - if (mem && !task_in_mem_cgroup(p, mem))
> + for_each_process(p) {
> + /*
> + * We don't have is_global_init() check here, because the old
> + * code do that. printing init process is not big matter. But
> + * we don't hope to make unnecessary compatiblity breaking.
> + */
> + if (p->flags & PF_KTHREAD)
> continue;
> - if (!thread_group_leader(p))
> + if (mem && !task_in_mem_cgroup(p, mem))
> continue;
>
> - task_lock(p);
> - mm = p->mm;
> - if (!mm) {
> + task = find_lock_task_mm(p);
> + if (!task)
> /*
> - * total_vm and rss sizes do not exist for tasks with no
> - * mm so there's no need to report them; they can't be
> - * oom killed anyway.
> + * Probably oom vs task-exiting race was happen and ->mm
> + * have been detached. thus there's no need to report them;
> + * they can't be oom killed anyway.
> */
> - task_unlock(p);
> continue;
> - }
> +
> printk(KERN_INFO "[%5d] %5d %5d %8lu %8lu %3d %3d %s\n",
> - p->pid, __task_cred(p)->uid, p->tgid, mm->total_vm,
> - get_mm_rss(mm), (int)task_cpu(p), p->signal->oom_adj,
> + task->pid, __task_cred(task)->uid, task->tgid, task->mm->total_vm,
> + get_mm_rss(task->mm), (int)task_cpu(task), task->signal->oom_adj,
> p->comm);
> - task_unlock(p);
> - } while_each_thread(g, p);
> + task_unlock(task);
> + }
> }
>
> static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
> --
> 1.6.5.2
>
>
>

--
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: David Rientjes on 3 Jun 2010 16:20
On Thu, 3 Jun 2010, Oleg Nesterov wrote:

> (off-topic)
>
> out_of_memory() calls dump_header()->dump_tasks() lockless, we
> need tasklist.
>

Already fixed in my rewrite patchset, as most of these things are. Sigh.
--
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: Oleg Nesterov on 3 Jun 2010 18:10
On 06/03, David Rientjes wrote:
>
> On Thu, 3 Jun 2010, Oleg Nesterov wrote:
>
> > (off-topic)
> >
> > out_of_memory() calls dump_header()->dump_tasks() lockless, we
> > need tasklist.

forgot to mention, __out_of_memory() too.

> Already fixed in my rewrite patchset, as most of these things are. Sigh.

In 3/18, without any note in the changelog. Another minor thing
which can be fixed before rewrite.

And please note that it was me who pointed out we need tasklist
during the previous discussion. I'd suggest you to send a separate
patch on top of Kosaki's patches.

OK, this is boring ;) I am going to ignore everything except
technical issues in this thread.

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/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: [PATCH] oom: Make coredump interruptible
Next: mac8390: raise error logging priority