From: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> To: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Containers <containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>, LKML <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Cgroups <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, "Paul E. McKenney" <paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>, Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org> Subject: Re: Q: cgroup: Questions about possible issues in cgroup locking Date: Sat, 14 Jan 2012 18:36:48 +0100 [thread overview] Message-ID: <20120114173648.GA32543@redhat.com> (raw) In-Reply-To: <20120113182750.GD18166-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> On 01/13, Mandeep Singh Baines wrote: > > Oleg Nesterov (oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org) wrote: > > > Yes, I thought about this too. Suppose we remove this assignment, > > then we can simply do > > > > #define while_each_thread(g, t) \ > > while (t->group_leader == g->group_leader && (t = next_thread(t)) != g) > > > > with the same effect. (to remind, currently I ignore the barriers/etc). > > > > Nice! I think this works. > > > But this can _only_ help if we start at the group leader! > > But I don't think this solution requires we start at the group leader. > > My thinking: > > Case 1: g is the exec thread > ... > Case 2: g is the group leader > ... > Case 3: g is some other thread > > In this case, g MUST be current Why? This is not true. But I guess this doesn't matter, I think I am starting to understand why our discussion was a bit confusing. The problem is _not_ exec/de_thread by itself. The problem is that while_each_thread(g, t) can race with removing 'g' from the list. IOW, list_for_each_rcu-like things assume that the head of the list is "stable" but this is not true. And note that de_thread() does not matter in this sense, only __unhash_process()->list_del_rcu(&p->thread_group) does matter. Now. If 'g' is the group leader, we should only worry about exec, otherwise it can't disappear before other threads. But if it is not the group leader, it can simply exit and while_each_thread(g, t) can never stop by the same reason. And we can't detect this case. OK, OK, we can, let me remind about http://marc.info/?l=linux-kernel&m=127714242731448 and the whole discussion. But this makes while_each_thread() more complex and this doesn't fork for zap_threads-like users which must not miss the "interesing" threads. Finally. If we restrict the lockless while_each_thread() so that is starts at the group leader, then we can rely on de_thread() which changes the leadership and try to detect this case. See? > > May be we should enforce this rule (for the lockless case), I dunno... > > In that case I'd prefer to add the new while_each_thread_rcu() helper. > > But! in this case we do not need to change de_thread(), we can simply do > > > > #define while_each_thread_rcu(t) \ > > while (({ t = next_thread(t); !thread_group_leader(t); })) > > > > Won't this terminate just before visiting the exec thread? Sure. But this is fine for zap_threads() or current_is_single_threaded(), the execing thread already has another ->mm. Just we shouldn't hang forever if we race with exec (we start at the group leader). And I hope this is fine in general (to remind, in the lockless case). If we race with exec we see either the old or the new leader with the same pid/signal/etc (at least). Hmm. On a second thought, perhaps I thought to much about zap_threads(), perhaps it would be better to find all threads we can... I dunno. But I am starting to like the ->group_leader more. Hmm, and with thread_group_leader() check we should ensure we do not visit the old leader twice. Thanks Mandeep. I'll try to recheck my thinking once again, what do you think? Anything else we could miss? Oleg.
WARNING: multiple messages have this Message-ID (diff)
From: Oleg Nesterov <oleg@redhat.com> To: Mandeep Singh Baines <msb@chromium.org> Cc: 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: Sat, 14 Jan 2012 18:36:48 +0100 [thread overview] Message-ID: <20120114173648.GA32543@redhat.com> (raw) In-Reply-To: <20120113182750.GD18166@google.com> On 01/13, Mandeep Singh Baines wrote: > > Oleg Nesterov (oleg@redhat.com) wrote: > > > Yes, I thought about this too. Suppose we remove this assignment, > > then we can simply do > > > > #define while_each_thread(g, t) \ > > while (t->group_leader == g->group_leader && (t = next_thread(t)) != g) > > > > with the same effect. (to remind, currently I ignore the barriers/etc). > > > > Nice! I think this works. > > > But this can _only_ help if we start at the group leader! > > But I don't think this solution requires we start at the group leader. > > My thinking: > > Case 1: g is the exec thread > ... > Case 2: g is the group leader > ... > Case 3: g is some other thread > > In this case, g MUST be current Why? This is not true. But I guess this doesn't matter, I think I am starting to understand why our discussion was a bit confusing. The problem is _not_ exec/de_thread by itself. The problem is that while_each_thread(g, t) can race with removing 'g' from the list. IOW, list_for_each_rcu-like things assume that the head of the list is "stable" but this is not true. And note that de_thread() does not matter in this sense, only __unhash_process()->list_del_rcu(&p->thread_group) does matter. Now. If 'g' is the group leader, we should only worry about exec, otherwise it can't disappear before other threads. But if it is not the group leader, it can simply exit and while_each_thread(g, t) can never stop by the same reason. And we can't detect this case. OK, OK, we can, let me remind about http://marc.info/?l=linux-kernel&m=127714242731448 and the whole discussion. But this makes while_each_thread() more complex and this doesn't fork for zap_threads-like users which must not miss the "interesing" threads. Finally. If we restrict the lockless while_each_thread() so that is starts at the group leader, then we can rely on de_thread() which changes the leadership and try to detect this case. See? > > May be we should enforce this rule (for the lockless case), I dunno... > > In that case I'd prefer to add the new while_each_thread_rcu() helper. > > But! in this case we do not need to change de_thread(), we can simply do > > > > #define while_each_thread_rcu(t) \ > > while (({ t = next_thread(t); !thread_group_leader(t); })) > > > > Won't this terminate just before visiting the exec thread? Sure. But this is fine for zap_threads() or current_is_single_threaded(), the execing thread already has another ->mm. Just we shouldn't hang forever if we race with exec (we start at the group leader). And I hope this is fine in general (to remind, in the lockless case). If we race with exec we see either the old or the new leader with the same pid/signal/etc (at least). Hmm. On a second thought, perhaps I thought to much about zap_threads(), perhaps it would be better to find all threads we can... I dunno. But I am starting to like the ->group_leader more. Hmm, and with thread_group_leader() check we should ensure we do not visit the old leader twice. Thanks Mandeep. I'll try to recheck my thinking once again, what do you think? Anything else we could miss? Oleg.
next prev parent reply other threads:[~2012-01-14 17:36 UTC|newest] Thread overview: 73+ 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 3:43 ` Frederic Weisbecker 2011-12-21 13:08 ` Oleg Nesterov 2011-12-21 13:08 ` Oleg Nesterov 2011-12-21 17:56 ` Frederic Weisbecker 2011-12-21 17:56 ` Frederic Weisbecker 2011-12-21 19:01 ` Mandeep Singh Baines 2011-12-21 19:01 ` Mandeep Singh Baines [not found] ` <20111221190102.GE13529-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2011-12-21 19:08 ` Frederic Weisbecker 2011-12-21 19:08 ` Frederic Weisbecker 2011-12-21 19:24 ` Mandeep Singh Baines 2011-12-21 19:24 ` Mandeep Singh Baines [not found] ` <20111221192413.GF13529-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2011-12-21 20:04 ` Frederic Weisbecker 2011-12-21 20:04 ` Frederic Weisbecker 2011-12-21 20:04 ` Frederic Weisbecker 2011-12-22 15:30 ` Oleg Nesterov 2011-12-22 15:30 ` Oleg Nesterov [not found] ` <20111222153004.GA30522-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2012-01-04 19:36 ` Mandeep Singh Baines 2012-01-04 19:36 ` Mandeep Singh Baines 2012-01-04 19:36 ` Mandeep Singh Baines [not found] ` <20120104193614.GF9511-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2012-01-06 15:23 ` Oleg Nesterov 2012-01-06 15:23 ` Oleg Nesterov [not found] ` <20120106152356.GA23995-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2012-01-06 18:25 ` Mandeep Singh Baines 2012-01-06 18:25 ` Mandeep Singh Baines 2012-01-06 18:25 ` Mandeep Singh Baines [not found] ` <20120106182535.GJ9511-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2012-01-11 16:07 ` Oleg Nesterov 2012-01-11 16:07 ` Oleg Nesterov [not found] ` <20120111160730.GA24556-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2012-01-12 0:31 ` Mandeep Singh Baines 2012-01-12 0:31 ` Mandeep Singh Baines [not found] ` <20120112003102.GB9511-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2012-01-12 17:07 ` Oleg Nesterov 2012-01-12 17:07 ` Oleg Nesterov 2012-01-12 17:57 ` Mandeep Singh Baines 2012-01-12 17:57 ` Mandeep Singh Baines [not found] ` <20120112175725.GD9511-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2012-01-13 15:20 ` Oleg Nesterov 2012-01-13 15:20 ` Oleg Nesterov [not found] ` <20120113152010.GA19215-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2012-01-13 18:27 ` Mandeep Singh Baines 2012-01-13 18:27 ` Mandeep Singh Baines [not found] ` <20120113182750.GD18166-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2012-01-14 17:36 ` Oleg Nesterov [this message] 2012-01-14 17:36 ` Oleg Nesterov 2012-01-18 23:17 ` Mandeep Singh Baines 2012-01-18 23:17 ` Mandeep Singh Baines [not found] ` <20120118231742.GS18166-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2012-01-19 15:45 ` Oleg Nesterov 2012-01-19 15:45 ` Oleg Nesterov 2012-01-19 18:18 ` Mandeep Singh Baines 2012-01-19 18:18 ` Mandeep Singh Baines [not found] ` <20120119181803.GU18166-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2012-01-20 15:06 ` Oleg Nesterov 2012-01-20 15:06 ` Oleg Nesterov 2012-03-20 19:34 ` Oleg Nesterov 2012-03-20 19:34 ` Oleg Nesterov [not found] ` <20120320193414.GA21277-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2012-03-21 18:59 ` Mandeep Singh Baines 2012-03-21 18:59 ` Mandeep Singh Baines 2012-03-23 17:51 ` Oleg Nesterov 2012-03-23 17:51 ` Oleg Nesterov [not found] ` <20120321185955.GK27051-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2012-03-23 17:51 ` Oleg Nesterov [not found] ` <20120119154522.GA14058-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2012-01-19 18:18 ` Mandeep Singh Baines 2012-03-20 19:34 ` Oleg Nesterov [not found] ` <20120114173648.GA32543-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2012-01-18 23:17 ` Mandeep Singh Baines [not found] ` <20120112170728.GA25717-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2012-01-12 17:57 ` Mandeep Singh Baines 2011-12-22 15:30 ` Oleg Nesterov 2011-12-21 19:01 ` Mandeep Singh Baines 2012-02-01 16:28 ` Frederic Weisbecker 2012-02-01 16:28 ` Frederic Weisbecker [not found] ` <20111221130848.GA19679-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2011-12-21 17:56 ` Frederic Weisbecker 2011-12-21 17:59 ` Frederic Weisbecker 2011-12-21 17:59 ` Frederic Weisbecker 2011-12-21 18:11 ` Oleg Nesterov 2011-12-21 18:11 ` Oleg Nesterov [not found] ` <20111221181101.GA3092-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2011-12-21 18:23 ` Frederic Weisbecker 2011-12-21 18:23 ` Frederic Weisbecker 2011-12-21 18:23 ` Frederic Weisbecker 2012-02-01 16:28 ` Frederic Weisbecker 2011-12-21 13:08 ` Oleg Nesterov -- strict thread matches above, loose matches on Subject: below -- 2011-12-21 3:43 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=20120114173648.GA32543@redhat.com \ --to=oleg-h+wxahxf7alqt0dzr+alfa@public.gmane.org \ --cc=akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org \ --cc=cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org \ --cc=fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \ --cc=paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org \ --cc=paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org \ --cc=tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.