[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20100916174433.GA4842@redhat.com>
Date: Thu, 16 Sep 2010 19:44:33 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org, oss-security@...ts.openwall.com,
Solar Designer <solar@...nwall.com>,
Kees Cook <kees.cook@...onical.com>, Al Viro <viro@...iv.linux.org.uk>,
Neil Horman <nhorman@...driver.com>, linux-fsdevel@...r.kernel.org,
pageexec@...email.hu, Brad Spengler <spender@...ecurity.net>,
Eugene Teo <eugene@...hat.com>,
KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
linux-mm <linux-mm@...ck.org>, David Rientjes <rientjes@...gle.com>
Subject: Re: [PATCH 4/4] oom: don't ignore rss in nascent mm
On 09/16, KOSAKI Motohiro wrote:
>
> ChangeLog
> o since v1
> - Always use thread group leader's ->in_exec_mm.
Confused ;)
> +static unsigned long oom_rss_swap_usage(struct task_struct *p)
> +{
> + struct task_struct *t = p;
> + struct task_struct *leader = p->group_leader;
> + unsigned long points = 0;
> +
> + do {
> + task_lock(t);
> + if (t->mm) {
> + points += get_mm_rss(t->mm);
> + points += get_mm_counter(t->mm, MM_SWAPENTS);
> + task_unlock(t);
> + break;
> + }
> + task_unlock(t);
> + } while_each_thread(p, t);
> +
> + /*
> + * If the process is in execve() processing, we have to concern
> + * about both old and new mm.
> + */
> + task_lock(leader);
> + if (leader->in_exec_mm) {
> + points += get_mm_rss(leader->in_exec_mm);
> + points += get_mm_counter(leader->in_exec_mm, MM_SWAPENTS);
> + }
> + task_unlock(leader);
> +
> + return points;
> +}
This patch relies on fact that we can't race with de_thread() (and btw
the change in de_thread() looks bogus). Then why ->in_exec_mm lives in
task_struct ?
To me, this looks a bit strange. I think we should either do not use
->group_leader to hold ->in_exec_mm like your previous patch did, or
move ->in_exec_mm into signal_struct. The previous 3/4 ensures that
only one thread can set ->in_exec_mm.
And I don't think oom_rss_swap_usage() should replace find_lock_task_mm()
in oom_badness(), I mean something like this:
static unsigned long oom_rss_swap_usage(struct mm_struct *mm)
{
return get_mm_rss(mm) + get_mm_counter(mm, MM_SWAPENTS);
}
unsigned int oom_badness(struct task_struct *p, ...)
{
int points = 0;
if (unlikely(p->signal->in_exec_mm)) {
task_lock(p->group_leader);
if (p->signal->in_exec_mm)
points = oom_rss_swap_usage(p->signal->in_exec_mm);
task_unlock(p->group_leader);
}
p = find_lock_task_mm(p);
if (!p)
return points;
...
}
but this is the matter of taste.
What do you think?
Oleg.
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
