From: Mandeep Singh Baines <msb@chromium.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Mandeep Singh Baines <msb@chromium.org>,
Frederic Weisbecker <fweisbec@gmail.com>,
Li Zefan <lizf@cn.fujitsu.com>, Tejun Heo <tj@kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Containers <containers@lists.linux-foundation.org>,
Cgroups <cgroups@vger.kernel.org>,
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
Paul Menage <paul@paulmenage.org>,
Andrew Morton <akpm@linux-foundation.org>,
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Subject: Re: Q: cgroup: Questions about possible issues in cgroup locking
Date: Wed, 21 Mar 2012 11:59:55 -0700 [thread overview]
Message-ID: <20120321185955.GK27051@google.com> (raw)
In-Reply-To: <20120320193414.GA21277@redhat.com>
Oleg Nesterov (oleg@redhat.com) wrote:
> OK, finally we should do something with this problem ;)
>
> On 01/19, Oleg Nesterov wrote:
> >
> > I'll try to investigate if we can remove
> >
> > leader->group_leader = tsk;
> >
> > from de_thread(). In fact I already thought about this change a long
> > ago without any connection to while_each_thread(). This assignment
> > looks "assymetrical" compared to other threads we kill. But we did
> > have a reason (reasons?). Hopefully, the only really important reason
> > was already removed by 087806b1.
>
> On the second thought, I think we should not do this.
>
> For example, do_prlimit() assumes that tsk->group_leader is correct
> under tasklist_lock.
>
> OK, lets return to the thread_group_leader() check. To ensure we do
> not visit the same thread twice we can check 'g', not 't'.
>
> This is what I am going to send, after I re-check once again...
>
> I have the problem with the changelog ;) Somehow it should explain
> that while_each_thread_rcu(g, t) can't race with do_group_exit().
> I think it can't, list_del_rcu(leader->thread_group) happens when
> this entry is already "empty", it should be the last thread in group.
> If the non-leader thread goes away from the least, we still have
> the "path" to reach the leader. But this is not easy to explain.
>
> As for the barrier. If de_thread() changes the leader it drops
> and re-acquires tasklist_lock (this implies mb) after it changes
> old_leader->exit_signal (used in thread_group_leader) and before
> __unhash_process() which does list_del_rcu().
>
> This means that if while_each_thread() sees the result of
> list_del_rcu(old_leader) it must also see that
> thread_group_leader(old_leader) != T.
>
> What do you think? Do you see any problems?
>
> Oleg.
> ---
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 7d379a6..f169bfd 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2323,9 +2323,24 @@ extern bool current_is_single_threaded(void);
> #define do_each_thread(g, t) \
> for (g = t = &init_task ; (g = t = next_task(g)) != &init_task ; ) do
>
> +/*
> + * needs tasklist_lock or ->siglock, or rcu if the caller ensures
> + * that 'g' can't exit or exec.
> + */
> #define while_each_thread(g, t) \
> while ((t = next_thread(t)) != g)
>
> +/*
> + * rcu-safe, but should start at ->group_leader.
> + * thread_group_leader(g) protects against the race with exec which
> + * removes the leader from list.
> + * smp_rmb() pairs with implicit mb() implied by unlock + lock in
> + * de_thread()->release_task() path.
> + */
> +#define while_each_thread_rcu(g, t) \
> + while ((t = next_thread(t)) != g && \
> + ({ smp_rmb(); thread_group_leader(g); }))
> +
Couldn't you miss the exec_thread if:
t = exec_thread && !thread_group_leader(g)
i.e. if you just passed (leader->group_leader = tsk;) in de_thread().
Could we change do_prlimit()? Especially since its slow path.
I really like you're earlier solution (ignoring barrier):
#define while_each_thread(g, t) \
while (t->group_leader == g->group_leader && (t = next_thread(t)) != g)
Regards,
Mandeep
> static inline int get_nr_threads(struct task_struct *tsk)
> {
> return tsk->signal->nr_threads;
>
next prev parent reply other threads:[~2012-03-21 19:00 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-12-21 3:43 Q: cgroup: Questions about possible issues in cgroup locking Frederic Weisbecker
2011-12-21 13:08 ` Oleg Nesterov
2011-12-21 17:56 ` Frederic Weisbecker
2011-12-21 19:01 ` Mandeep Singh Baines
2011-12-21 19:08 ` Frederic Weisbecker
2011-12-21 19:24 ` Mandeep Singh Baines
2011-12-21 20:04 ` Frederic Weisbecker
2011-12-22 15:30 ` Oleg Nesterov
2012-01-04 19:36 ` Mandeep Singh Baines
2012-01-06 15:23 ` Oleg Nesterov
2012-01-06 18:25 ` Mandeep Singh Baines
2012-01-11 16:07 ` Oleg Nesterov
2012-01-12 0:31 ` Mandeep Singh Baines
2012-01-12 17:07 ` Oleg Nesterov
2012-01-12 17:57 ` Mandeep Singh Baines
2012-01-13 15:20 ` Oleg Nesterov
2012-01-13 18:27 ` Mandeep Singh Baines
2012-01-14 17:36 ` Oleg Nesterov
2012-01-18 23:17 ` Mandeep Singh Baines
2012-01-19 15:45 ` Oleg Nesterov
2012-01-19 18:18 ` Mandeep Singh Baines
2012-01-20 15:06 ` Oleg Nesterov
2012-03-20 19:34 ` Oleg Nesterov
2012-03-21 18:59 ` Mandeep Singh Baines [this message]
2012-03-23 17:51 ` Oleg Nesterov
2011-12-21 17:59 ` Frederic Weisbecker
2011-12-21 18:11 ` Oleg Nesterov
2011-12-21 18:23 ` Frederic Weisbecker
2012-02-01 16:28 ` Frederic Weisbecker
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=20120321185955.GK27051@google.com \
--to=msb@chromium.org \
--cc=akpm@linux-foundation.org \
--cc=cgroups@vger.kernel.org \
--cc=containers@lists.linux-foundation.org \
--cc=fweisbec@gmail.com \
--cc=kamezawa.hiroyu@jp.fujitsu.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lizf@cn.fujitsu.com \
--cc=oleg@redhat.com \
--cc=paul@paulmenage.org \
--cc=paulmck@linux.vnet.ibm.com \
--cc=tj@kernel.org \
/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;
as well as URLs for NNTP newsgroup(s).