public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Sergey Dyasly <dserrg@gmail.com>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	David Rientjes <rientjes@google.com>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Mandeep Singh Baines <msb@chromium.org>,
	"Ma, Xindong" <xindong.ma@intel.com>,
	Michal Hocko <mhocko@suse.cz>, Sameer Nanda <snanda@chromium.org>,
	"Tu, Xiaobing" <xiaobing.tu@intel.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/2] initial while_each_thread() fixes
Date: Tue, 3 Dec 2013 19:46:35 +0400	[thread overview]
Message-ID: <20131203194635.7be1c07ae042ee395bc83527@gmail.com> (raw)
In-Reply-To: <20131202152423.GA10878@redhat.com>

Hi Oleg,

I was waiting for this one!

On Mon, 2 Dec 2013 16:24:23 +0100
Oleg Nesterov <oleg@redhat.com> wrote:

> Hello.
> 
> This was reported several times, I believe the first report is
> http://marc.info/?l=linux-kernel&m=127688978121665. Hmm, 3 years
> ago. The lockless while_each_thread() is racy and broken, almost
> every user can loop forever.
> 
> Recently people started to report they actually hit this problem in
> oom_kill.c. This doesn't really matter and I can be wrong, but in
> fact I do not think they really hit this race, it is very unlikely.

The race is very easy to catch if you have a process with several threads,
all of which allocates memory simultaneously. This leads to:

  1) OOMk selects and sends SIGKILL to one of the threads

  2) another thread invokes OOMk and the first thread gets selected,
     but it gets unhashed before while_each_thread...

> Another problem with while_each_thread() is that it is very easy
> to use it wrongly, and oom_kill.c is the good example.
> 
> I came to conclusion that it is practically impossible to send a
> single series which fixes all problems, too many different users.
> 
> So 1/2 adds the new for_each_thread() interface, and 2/2 fixes oom
> kill as an example.
> 
> We obviously need a lot more changes like 2/2 before we can kill
> while_each_thread() and task_struct->thread_group, but I hope they
> will be straighforward. And in fact I hope that task->thread_group
> can go away before we change all users of while_each_thread().
> 
> David, et al, I din't actually test 2/2, I do not know how. Please
> review, although it looks simple.

The patches look correct and my test case no longer hangs, so

Reviewed-and-Tested-by: Sergey Dyasly <dserrg@gmail.com>

> 
> Oleg.
> 
>  include/linux/init_task.h |    2 ++
>  include/linux/sched.h     |   12 ++++++++++++
>  kernel/exit.c             |    1 +
>  kernel/fork.c             |    7 +++++++
>  mm/oom_kill.c             |   37 ++++++++++++++++++++-----------------
>  5 files changed, 42 insertions(+), 17 deletions(-)
> 

-- 
Sergey Dyasly <dserrg@gmail.com>

  parent reply	other threads:[~2013-12-03 15:46 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-02 15:24 [PATCH 0/2] initial while_each_thread() fixes Oleg Nesterov
2013-12-02 15:24 ` [PATCH 1/2] introduce for_each_thread() to replace the buggy while_each_thread() Oleg Nesterov
2013-12-03 19:24   ` Sameer Nanda
2013-12-02 15:24 ` [PATCH 2/2] oom_kill: change oom_kill.c to use for_each_thread() Oleg Nesterov
2013-12-03 18:57   ` Sameer Nanda
2013-12-03 20:05     ` Oleg Nesterov
2013-12-03 20:50       ` Oleg Nesterov
2013-12-04 12:57     ` Oleg Nesterov
2013-12-02 15:34 ` [PATCH 0/2] initial while_each_thread() fixes Oleg Nesterov
2013-12-03 15:46 ` Sergey Dyasly [this message]
2013-12-03 18:52   ` Oleg Nesterov
2013-12-03 19:01   ` Oleg Nesterov
2013-12-03 16:53 ` William Dauchy
2013-12-03 20:22   ` Oleg Nesterov
2013-12-03 20:28     ` William Dauchy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131203194635.7be1c07ae042ee395bc83527@gmail.com \
    --to=dserrg@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=fweisbec@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhocko@suse.cz \
    --cc=msb@chromium.org \
    --cc=oleg@redhat.com \
    --cc=rientjes@google.com \
    --cc=snanda@chromium.org \
    --cc=xiaobing.tu@intel.com \
    --cc=xindong.ma@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox