Q: cgroup: Questions about possible issues in cgroup locking

Hi,

Starring at some parts of cgroups, I have a few questions:

- Is cgroup_enable_task_cg_list()'s while_each_thread() safe against concurrent exec()? The leader may change in de_thread() and invalidate the test done in while_each_thread().

- do_each_thread() also needs RCU and cgroup_enable_task_cg_list()
seems to remind it. But it seems there is at least one caller that
doesn't call rcu_read_lock(): update_cpu_mask() -> update_tasks_cpumask() -> cgroup_scan_tasks()

- By the time we call cgroup_post_fork(), it is ready to be woken up and usable by the scheduler.

- Is the check for use_task_css_set_links in cgroup_post_fork() safe? given it is checked outside css_set_lock?

Imagine this:

CPU 0 CPU 1
---- -----

cgroup_enable_task_cg() {
	uset_tasks_css_set_links = 1
	for_each_thread() {
		add tasks in the list
	}
}

do_fork() {
cgroup_post_fork() {
use_tasks_css_set_links appears to be equal to 0 due to write/read
not flushed. New task won't appear to the list.

Quoted 7 lines · Frederic Weisbecker

Hi,

Starring at some parts of cgroups, I have a few questions:

- Is cgroup_enable_task_cg_list()'s while_each_thread() safe against concurrent exec()? The leader may change in de_thread() and invalidate the test done in while_each_thread().

Yes. Oh, we need to do something with while_each_thread.

Quoted 10 lines · Oleg Nesterov

Quoted 7 lines · Frederic Weisbecker

Hi,

Starring at some parts of cgroups, I have a few questions:

- Is cgroup_enable_task_cg_list()'s while_each_thread() safe against concurrent exec()? The leader may change in de_thread() and invalidate the test done in while_each_thread().

Yes. Oh, we need to do something with while_each_thread.

Would something like this work?

include/linux/sched.h +0 -0

index c0c5876..e002a00 100644

include/linux/sched.h +1 -0

+	}))
 
 static inline int get_nr_threads(struct task_struct *tsk)
 {

Quoted 19 lines · Frederic Weisbecker

Quoted 10 lines · Oleg Nesterov

Quoted 7 lines · Frederic Weisbecker

Hi,

Starring at some parts of cgroups, I have a few questions:

- Is cgroup_enable_task_cg_list()'s while_each_thread() safe against concurrent exec()? The leader may change in de_thread() and invalidate the test done in while_each_thread().

Yes. Oh, we need to do something with while_each_thread.

Would something like this work?

include/linux/sched.h +0 -0

index c0c5876..e002a00 100644

include/linux/sched.h extern bool current_is_single_threaded(void); · line 2300

Quoted 10 lines

include/linux/sched.h +5 -2

@@ -2293,8 +2293,12 @@ 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
 
-#define while_each_thread(g, t) \
-	while ((t = next_thread(t)) != g)
+#define while_each_thread(g, t)					\
+	while (({						\
+		struct task_struct *__prev = t;			\
+		t = next_thread(t);				\
+		t != __prev && t != g;				\

Hi,

Don't you still have an (highly unlikely) race if you exec and then pthread_create()?

Instead of:

t != __prev && t != g;

How about:

t != t->group_leader;

Quoted 33 lines · Mandeep Singh Baines

Quoted 19 lines · Frederic Weisbecker

Quoted 10 lines · Oleg Nesterov

Quoted 7 lines · Frederic Weisbecker

Hi,

Starring at some parts of cgroups, I have a few questions:

- Is cgroup_enable_task_cg_list()'s while_each_thread() safe against concurrent exec()? The leader may change in de_thread() and invalidate the test done in while_each_thread().

Yes. Oh, we need to do something with while_each_thread.

Would something like this work?

include/linux/sched.h +0 -0

index c0c5876..e002a00 100644

include/linux/sched.h extern bool current_is_single_threaded(void); · line 2300

Quoted 10 lines

include/linux/sched.h +5 -2

@@ -2293,8 +2293,12 @@ 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
 
-#define while_each_thread(g, t) \
-	while ((t = next_thread(t)) != g)
+#define while_each_thread(g, t)					\
+	while (({						\
+		struct task_struct *__prev = t;			\
+		t = next_thread(t);				\
+		t != __prev && t != g;				\

Hi,

Quoted 2 lines

Don't you still have an (highly unlikely) race if you exec and then pthread_create()?

I'm not sure what you mean.

Quoted 9 lines

Instead of:

t != __prev && t != g;

How about:

t != t->group_leader;

That might work too but we need a pair of memory barriers.

Quoted 37 lines · Frederic Weisbecker

Quoted 35 lines · Mandeep Singh Baines

Quoted 19 lines · Frederic Weisbecker

Quoted 10 lines · Oleg Nesterov

Quoted 7 lines · Frederic Weisbecker

Hi,

Starring at some parts of cgroups, I have a few questions:

- Is cgroup_enable_task_cg_list()'s while_each_thread() safe against concurrent exec()? The leader may change in de_thread() and invalidate the test done in while_each_thread().

Yes. Oh, we need to do something with while_each_thread.

Would something like this work?

include/linux/sched.h +0 -0

index c0c5876..e002a00 100644

include/linux/sched.h extern bool current_is_single_threaded(void); · line 2300

Quoted 10 lines

include/linux/sched.h +5 -2

@@ -2293,8 +2293,12 @@ 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
 
-#define while_each_thread(g, t) \
-	while ((t = next_thread(t)) != g)
+#define while_each_thread(g, t)					\
+	while (({						\
+		struct task_struct *__prev = t;			\
+		t = next_thread(t);				\
+		t != __prev && t != g;				\

Hi,

Don't you still have an (highly unlikely) race if you exec and then pthread_create()?

Quoted 1 line

I'm not sure what you mean.

Here is what I'm thinking:

If you call exec from a thread other than g, g is now unlinked. So "t != g" will always be true. If you then pthread_create, you now have two threads so "t != __prev" will also always be true. So you now have an infinite loop.

Quoted 5 lines

Quoted 5 lines

Instead of:

Quoted 7 lines

Quoted 5 lines

t != __prev && t != g;

How about:

t != t->group_leader;

That might work too but we need a pair of memory barriers.

next_thread() calls list_entry_rcu. Shouldn't that protect against a dereference? You don't need to synchronize group_leader since you are only using it as a value. You don't dereference it.

Regards,
Mandeep

Quoted 5 lines · Mandeep Singh Baines

If you call exec from a thread other than g, g is now unlinked. So "t != g" will always be true. If you then pthread_create, you now have two threads so "t != __prev" will also always be true. So you now have an infinite loop.

Oh you're right.

But then we can't use t != t->group_leader because that assumes while_each_thread() started on the leader.

Quoted 10 lines · Frederic Weisbecker

Quoted 5 lines · Mandeep Singh Baines

If you call exec from a thread other than g, g is now unlinked. So "t != g" will always be true. If you then pthread_create, you now have two threads so "t != __prev" will also always be true. So you now have an infinite loop.

Oh you're right.

Quoted 2 lines

But then we can't use t != t->group_leader because that assumes while_each_thread() started on the leader.

Yes, this can't work.

Besides, we need more burriers to rely on the ->group_leader check.

See http://marc.info/?t=127688987300002

in particular, http://marc.info/?l=linux-kernel&m=127714242731448
I think this should work, but then we should do something with the users like zap_threads().

Oleg.

Quoted 11 lines · Oleg Nesterov

Quoted 10 lines · Frederic Weisbecker

Quoted 5 lines · Mandeep Singh Baines

If you call exec from a thread other than g, g is now unlinked. So "t != g" will always be true. If you then pthread_create, you now have two threads so "t != __prev" will also always be true. So you now have an infinite loop.

Oh you're right.

Quoted 9 lines

Quoted 2 lines

But then we can't use t != t->group_leader because that assumes while_each_thread() started on the leader.

Yes, this can't work.

Besides, we need more burriers to rely on the ->group_leader check.

See http://marc.info/?t=127688987300002

I went through the thread. Were there any other concerns other than requiring that you start with the group_leader and the barrier?

You could modify zap_other_threads to start with the group leader by skipping p:

if (p == t)
continue;

Quoted 4 lines

in particular, http://marc.info/?l=linux-kernel&m=127714242731448
I think this should work, but then we should do something with the users like zap_threads().

With that patch, won't you potentially miss the exec thread if an exec occurs while you're iterating over the list? Is that OK?

Quoted 23 lines · Mandeep Singh Baines

Quoted 11 lines · Oleg Nesterov

Quoted 10 lines · Frederic Weisbecker

Quoted 5 lines · Mandeep Singh Baines

If you call exec from a thread other than g, g is now unlinked. So "t != g" will always be true. If you then pthread_create, you now have two threads so "t != __prev" will also always be true. So you now have an infinite loop.

Oh you're right.

Quoted 9 lines

Quoted 2 lines

But then we can't use t != t->group_leader because that assumes while_each_thread() started on the leader.

Yes, this can't work.

Besides, we need more burriers to rely on the ->group_leader check.

See http://marc.info/?t=127688987300002

Quoted 8 lines

I went through the thread. Were there any other concerns other than requiring that you start with the group_leader and the barrier?

You could modify zap_other_threads to start with the group leader by skipping p:

if (p == t)
continue;

Yes, we can but there are other while_each_thread(nonleader) users. Yes we can fix them too but this looks a bit ugly and we need to change while_each_thread() anyway. And I do not see why this change will be simpler if we restrict it to group_leader.

And note that zap_other_threads() is fine in any case, it is called under ->siglock.

Quoted 5 lines

Quoted 4 lines

in particular, http://marc.info/?l=linux-kernel&m=127714242731448
I think this should work, but then we should do something with the users like zap_threads().

Quoted 2 lines

With that patch, won't you potentially miss the exec thread if an exec occurs while you're iterating over the list? Is that OK?

Of course it is not OK ;) Note the "we should do something with" above.

Oleg.

Quoted 6 lines · Oleg Nesterov

Quoted 6 lines

Quoted 5 lines

in particular, http://marc.info/?l=linux-kernel&m=127714242731448
I think this should work, but then we should do something with the users like zap_threads().

Quoted 5 lines

Quoted 2 lines

With that patch, won't you potentially miss the exec thread if an exec occurs while you're iterating over the list? Is that OK?

Of course it is not OK ;) Note the "we should do something with" above.

So requirements should be something like this:

* Any task alive for the duration of the iteration MUST be visited
* No task should be visited more than once
* Any task born or exiting after starting the iteration MAY be skipped
* You can start at any task in the thread group

Would something like this work:

#define while_each_thread(g, t, o) \
	while (t->group_leader == o && (t = next_thread(t)) != g)

Where o should have the value of g->group_leader.

Quoted 11 lines · Mandeep Singh Baines

Quoted 9 lines · Oleg Nesterov

Quoted 8 lines

Quoted 5 lines

in particular, http://marc.info/?l=linux-kernel&m=127714242731448
I think this should work, but then we should do something with the users like zap_threads().

With that patch, won't you potentially miss the exec thread if an exec occurs while you're iterating over the list? Is that OK?

Quoted 4 lines

Quoted 2 lines

Of course it is not OK ;) Note the "we should do something with" above.

So requirements should be something like this:

(I assume, you mean the lockless case)

Quoted 4 lines

* Any task alive for the duration of the iteration MUST be visited
* No task should be visited more than once
* Any task born or exiting after starting the iteration MAY be skipped
* You can start at any task in the thread group

Well yes, but it is not easy to exactly define what after/before means in this case.

Quoted 6 lines

Would something like this work:

#define while_each_thread(g, t, o) \
	while (t->group_leader == o && (t = next_thread(t)) != g)

Where o should have the value of g->group_leader.

I don't understand how this helps... and how this can work even ignoring the barriers.

OK, we have the main thream M and the sub-thread T, we are doing

	do {
		do_something(t);
	} while_each_thread(M, t, M);

why we can't miss T if it does exec?

Oleg.

Quoted 15 lines

Quoted 5 lines

#define while_each_thread(g, t, o) \
	while (t->group_leader == o && (t = next_thread(t)) != g)

Where o should have the value of g->group_leader.

I don't understand how this helps... and how this can work even ignoring the barriers.

OK, we have the main thream M and the sub-thread T, we are doing

	do {
		do_something(t);
	} while_each_thread(M, t, M);

Quoted 2 lines

why we can't miss T if it does exec?

So for:

struct task *M; /* assuming this is passed in to us */ struct task *L = M->group_leader;

do {
	do_something(T);
} while_each_thread(M, T, L);

Here is my thinking.

If some thread K does exec, you won't miss it because:

1) Ignoring the group_leader check, you'll visit K just by following next_thread(). That's the case today and is what you except when iterating over an rcu_list.
2) (t->group_leader == o) will fail iff t is the exec thread. Since we test t->group_leader before re-assigning it (t=next_thread()), the test will fail only after visiting the exec thread. So you'll visit the exec thread and then terminate the loop.

Hi Mandeep,

Quoted 15 lines · Mandeep Singh Baines

Quoted 15 lines

Quoted 5 lines

#define while_each_thread(g, t, o) \
	while (t->group_leader == o && (t = next_thread(t)) != g)

Where o should have the value of g->group_leader.

I don't understand how this helps... and how this can work even ignoring the barriers.

OK, we have the main thream M and the sub-thread T, we are doing

	do {
		do_something(t);
	} while_each_thread(M, t, M);

Quoted 7 lines

Quoted 2 lines

why we can't miss T if it does exec?

So for:

struct task *M; /* assuming this is passed in to us */ struct task *L = M->group_leader;

L == M

Quoted 8 lines

do {
	do_something(T);
} while_each_thread(M, T, L);

Here is my thinking.

If some thread K does exec, you won't miss it because:

Quoted 7 lines

1) Ignoring the group_leader check, you'll visit K just by following next_thread(). That's the case today and is what you except when iterating over an rcu_list.
2) (t->group_leader == o) will fail iff t is the exec thread. Since we test t->group_leader before re-assigning it (t=next_thread()), the test will fail only after visiting the exec thread. So you'll visit the exec thread and then terminate the loop.

Still can't understand... Lets look at this trivial example again.

We start from the main thread M, it is ->group_leader. There is another thread T in this thread group. We are doing

	OLD = M;

	t = M;
	do {
		do_smth(t);
	}
	while (t->group_leader == OLD && ((t = next_thread(t)) != M);

The first iteration does do_smth(M).

T calls de_thread() and, in particular, it does M->group_leader = T (see "leader->group_leader = tsk" in de_thread).

after that t->group_leader == OLD fails. t == M, its group_leader == T. do_smth(T) won't be called.

No?

Oleg.

Quoted 16 lines · Oleg Nesterov

Still can't understand... Lets look at this trivial example again.

We start from the main thread M, it is ->group_leader. There is another thread T in this thread group. We are doing

	OLD = M;

	t = M;
	do {
		do_smth(t);
	}
	while (t->group_leader == OLD && ((t = next_thread(t)) != M);

The first iteration does do_smth(M).

Quoted 8 lines

T calls de_thread() and, in particular, it does M->group_leader = T (see "leader->group_leader = tsk" in de_thread).

after that t->group_leader == OLD fails. t == M, its group_leader == T. do_smth(T) won't be called.

No?

I think we can handle this by removing the assignment. So in de_thread():

Quoted 27 lines · Mandeep Singh Baines

Quoted 24 lines · Oleg Nesterov

Still can't understand... Lets look at this trivial example again.

We start from the main thread M, it is ->group_leader. There is another thread T in this thread group. We are doing

	OLD = M;

	t = M;
	do {
		do_smth(t);
	}
	while (t->group_leader == OLD && ((t = next_thread(t)) != M);

The first iteration does do_smth(M).

T calls de_thread() and, in particular, it does M->group_leader = T (see "leader->group_leader = tsk" in de_thread).

after that t->group_leader == OLD fails. t == M, its group_leader == T. do_smth(T) won't be called.

No?

Review

Quoted 3 lines

I think we can handle this by removing the assignment. So in de_thread():

(quoted diff) +0 -1

-		leader->group_leader = tsk;

Ah, so that was you plan. I was confused by the 3rd argument, why it is needed?

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).

But this can _only_ help if we start at the group leader!

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); }))

The definition above was one of the possibilities I considered, but I wasn't able to convince myself this is the best option.

See? Or do you think I missed something?

Just in case... note that while_each_thread_rcu() doesn't use 'g' at all. May be it makes sense to keep the old "t != g &&", but this is minor.

Oleg.

Quoted 8 lines · Oleg Nesterov

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.

Quoted 1 line

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

Quoted 7 lines

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?

Quoted 26 lines · Mandeep Singh Baines

Quoted 8 lines · Oleg Nesterov

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.

Quoted 1 line

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

Quoted 1 line

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?

Quoted 9 lines

Quoted 7 lines

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.

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines · Mandeep Singh Baines

Case 3: g is some other thread

In this case, g MUST be current

Why? This is not true.

Here is my thinking:

The terminating condition, t != g, assumes that you can get back to g. If g is unhashed, there is no guarantee you'll ever get back to it. Holding a reference does not prevent unhashing.

for_each_process avoids unhashing by starting and ending at init_task (which can never be unhashed).

As you pointed out a while back, this doesn't work for:

do_each_thread(g, t){
do_something(t);
} while_each_thread(g, t)

because g can be unhashed.

However, you should be able to use while_each_thread if you are current. Being current would prevent 'g' from being unhashed.

other than, do_each_thread/while_each_thread, all other callers of while_each_thread() are starting at current. Otherwise, how do you guarantee that it terminates.

I see at least one example, coredump_wait() that uses while_each_thread starting at current. I didn't find any cases where while_each_thread starts anywhere other than current or group_leader.

Quoted 8 lines

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.

Quoted 9 lines

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.

I think we are on the same page. Your explanation is consistent with my understanding.

Some other thoughts:

I suspect that other than do_each_thread()/while_each_thread() or for_each_thread()/while_each_thread() where 'g' is the group_leader, 'g' MUST be current. So the only cases to consider are:

Quoted 2 lines

I'll try to recheck my thinking once again, what do you think? Anything else we could miss?

Yeah, the ->group_leader solution seems the most promising. It seems correct (ignoring barriers) and should work for all supported cases:

1) when g is group_leader
2) when g is current

Quoted 10 lines · Mandeep Singh Baines

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines · Mandeep Singh Baines

Case 3: g is some other thread

In this case, g MUST be current

Why? This is not true.

Quoted 8 lines

Here is my thinking:

The terminating condition, t != g, assumes that you can get back to g. If g is unhashed, there is no guarantee you'll ever get back to it. Holding a reference does not prevent unhashing.

for_each_process avoids unhashing by starting and ending at init_task (which can never be unhashed).

Yes,

Quoted 10 lines

As you pointed out a while back, this doesn't work for:

do_each_thread(g, t){
do_something(t);
} while_each_thread(g, t)

because g can be unhashed.

However, you should be able to use while_each_thread if you are current. Being current would prevent 'g' from being unhashed.

Ah, I misunderstood you. Yes, sure.

Quoted 3 lines

other than, do_each_thread/while_each_thread, all other callers of while_each_thread() are starting at current. Otherwise, how do you guarantee that it terminates.

Hm, still can't understand...

Quoted 3 lines

I see at least one example, coredump_wait() that uses while_each_thread starting at current. I didn't find any cases where while_each_thread starts anywhere other than current or group_leader.

Probably you meant zap_threads/zap_process, not coredump_wait?

zap_process() is fine, we hold ->siglock. But zap_threads does _not_

start at current, may be you misread the g == tsk->group_leader check in the main for_each_process() loop ? But most probably we simply misunderstand each other a bit, see below.

However it starts at ->group_leader, so it won't suffer if we restrict the lockless while_each_thread_rcu().

Quoted 21 lines

Quoted 8 lines

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.

Quoted 9 lines

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.

I think we are on the same page. Your explanation is consistent with my understanding.

Quoted 5 lines

Some other thoughts:

I suspect that other than do_each_thread()/while_each_thread() or for_each_thread()/while_each_thread() where 'g' is the group_leader, 'g' MUST be current. So the only cases to consider are:

I didn't try to grep, but I do not know any example of the lockless while_each_thread() which starts at current.

I guess this is the source of confusion.

Quoted 8 lines

Quoted 2 lines

I'll try to recheck my thinking once again, what do you think? Anything else we could miss?

Yeah, the ->group_leader solution seems the most promising. It seems correct (ignoring barriers) and should work for all supported cases:

1) when g is group_leader
2) when g is current

OK, thanks.

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.

Oleg.

Quoted 3 lines · Oleg Nesterov

But we did
have a reason (reasons?). Hopefully, the only really important reason was already removed by 087806b1.

Ah. So the leader->group_leader may have been necessary earlier in order to prevent two tasks, old leader and new leader from both returning true for thread_group_leader(tsk).

Quoted 10 lines · Mandeep Singh Baines

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines · Mandeep Singh Baines

Case 3: g is some other thread

In this case, g MUST be current

Why? This is not true.

Quoted 8 lines

Here is my thinking:

The terminating condition, t != g, assumes that you can get back to g. If g is unhashed, there is no guarantee you'll ever get back to it. Holding a reference does not prevent unhashing.

for_each_process avoids unhashing by starting and ending at init_task (which can never be unhashed).

Yes,

Quoted 10 lines

As you pointed out a while back, this doesn't work for:

do_each_thread(g, t){
do_something(t);
} while_each_thread(g, t)

because g can be unhashed.

However, you should be able to use while_each_thread if you are current. Being current would prevent 'g' from being unhashed.

Ah, I misunderstood you. Yes, sure.

Quoted 3 lines

other than, do_each_thread/while_each_thread, all other callers of while_each_thread() are starting at current. Otherwise, how do you guarantee that it terminates.

Hm, still can't understand...

Quoted 3 lines

I see at least one example, coredump_wait() that uses while_each_thread starting at current. I didn't find any cases where while_each_thread starts anywhere other than current or group_leader.

Probably you meant zap_threads/zap_process, not coredump_wait?

zap_process() is fine, we hold ->siglock. But zap_threads does _not_

start at current, may be you misread the g == tsk->group_leader check in the main for_each_process() loop ? But most probably we simply misunderstand each other a bit, see below.

However it starts at ->group_leader, so it won't suffer if we restrict the lockless while_each_thread_rcu().

Quoted 21 lines

Quoted 8 lines

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.

Quoted 9 lines

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.

I think we are on the same page. Your explanation is consistent with my understanding.

Quoted 5 lines

Some other thoughts:

I suspect that other than do_each_thread()/while_each_thread() or for_each_thread()/while_each_thread() where 'g' is the group_leader, 'g' MUST be current. So the only cases to consider are:

I didn't try to grep, but I do not know any example of the lockless while_each_thread() which starts at current.

I guess this is the source of confusion.

Quoted 8 lines

Quoted 2 lines

I'll try to recheck my thinking once again, what do you think? Anything else we could miss?

Yeah, the ->group_leader solution seems the most promising. It seems correct (ignoring barriers) and should work for all supported cases:

1) when g is group_leader
2) when g is current

OK, thanks.

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.

Oleg.

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.

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.

OK, finally we should do something with this problem ;)

Quoted 10 lines · Oleg Nesterov

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.
---

include/linux/sched.h +5 -0

index 7d379a6..f169bfd 100644
@@ -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)
 
+/*

include/linux/sched.h +0 -0

 static inline int get_nr_threads(struct task_struct *tsk)
 {
 	return tsk->signal->nr_threads;

include/linux/sched.h extern bool current_is_single_threaded(void); · line 2343

Quoted 11 lines · Oleg Nesterov

include/linux/sched.h +11 -0

+/*
+ * 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)

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)

include/linux/sched.h extern bool current_is_single_threaded(void); · line 2343

Quoted 11 lines · Oleg Nesterov

include/linux/sched.h +11 -0

+/*
+ * 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)

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)

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines · Mandeep Singh Baines

Case 3: g is some other thread

In this case, g MUST be current

Why? This is not true.

Here is my thinking:

The terminating condition, t != g, assumes that you can get back to g. If g is unhashed, there is no guarantee you'll ever get back to it. Holding a reference does not prevent unhashing.

for_each_process avoids unhashing by starting and ending at init_task (which can never be unhashed).

As you pointed out a while back, this doesn't work for:

do_each_thread(g, t){
do_something(t);
} while_each_thread(g, t)

because g can be unhashed.

However, you should be able to use while_each_thread if you are current. Being current would prevent 'g' from being unhashed.

other than, do_each_thread/while_each_thread, all other callers of while_each_thread() are starting at current. Otherwise, how do you guarantee that it terminates.

I see at least one example, coredump_wait() that uses while_each_thread starting at current. I didn't find any cases where while_each_thread starts anywhere other than current or group_leader.

Quoted 8 lines

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.

Quoted 9 lines

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.

I think we are on the same page. Your explanation is consistent with my understanding.

Some other thoughts:

I suspect that other than do_each_thread()/while_each_thread() or for_each_thread()/while_each_thread() where 'g' is the group_leader, 'g' MUST be current. So the only cases to consider are:

Quoted 2 lines

I'll try to recheck my thinking once again, what do you think? Anything else we could miss?

Yeah, the ->group_leader solution seems the most promising. It seems correct (ignoring barriers) and should work for all supported cases:

1) when g is group_leader
2) when g is current

Quoted 26 lines · Mandeep Singh Baines

Quoted 8 lines · Oleg Nesterov

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.

Quoted 1 line

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

Quoted 1 line

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?

Quoted 9 lines

Quoted 7 lines

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.

Hi Mandeep,

Quoted 15 lines · Mandeep Singh Baines

Quoted 15 lines

Quoted 5 lines

#define while_each_thread(g, t, o) \
	while (t->group_leader == o && (t = next_thread(t)) != g)

Where o should have the value of g->group_leader.

I don't understand how this helps... and how this can work even ignoring the barriers.

OK, we have the main thream M and the sub-thread T, we are doing

	do {
		do_something(t);
	} while_each_thread(M, t, M);

Quoted 7 lines

Quoted 2 lines

why we can't miss T if it does exec?

So for:

struct task *M; /* assuming this is passed in to us */ struct task *L = M->group_leader;

L == M

Quoted 8 lines

do {
	do_something(T);
} while_each_thread(M, T, L);

Here is my thinking.

If some thread K does exec, you won't miss it because:

Quoted 7 lines

1) Ignoring the group_leader check, you'll visit K just by following next_thread(). That's the case today and is what you except when iterating over an rcu_list.
2) (t->group_leader == o) will fail iff t is the exec thread. Since we test t->group_leader before re-assigning it (t=next_thread()), the test will fail only after visiting the exec thread. So you'll visit the exec thread and then terminate the loop.

Still can't understand... Lets look at this trivial example again.

We start from the main thread M, it is ->group_leader. There is another thread T in this thread group. We are doing

	OLD = M;

	t = M;
	do {
		do_smth(t);
	}
	while (t->group_leader == OLD && ((t = next_thread(t)) != M);

The first iteration does do_smth(M).

T calls de_thread() and, in particular, it does M->group_leader = T (see "leader->group_leader = tsk" in de_thread).

after that t->group_leader == OLD fails. t == M, its group_leader == T. do_smth(T) won't be called.

No?

Oleg.

Quoted 23 lines · Mandeep Singh Baines

Quoted 11 lines · Oleg Nesterov

Quoted 10 lines · Frederic Weisbecker

Quoted 5 lines · Mandeep Singh Baines

If you call exec from a thread other than g, g is now unlinked. So "t != g" will always be true. If you then pthread_create, you now have two threads so "t != __prev" will also always be true. So you now have an infinite loop.

Oh you're right.

Quoted 9 lines

Quoted 2 lines

But then we can't use t != t->group_leader because that assumes while_each_thread() started on the leader.

Yes, this can't work.

Besides, we need more burriers to rely on the ->group_leader check.

See http://marc.info/?t=127688987300002

Quoted 8 lines

I went through the thread. Were there any other concerns other than requiring that you start with the group_leader and the barrier?

You could modify zap_other_threads to start with the group leader by skipping p:

if (p == t)
continue;

Yes, we can but there are other while_each_thread(nonleader) users. Yes we can fix them too but this looks a bit ugly and we need to change while_each_thread() anyway. And I do not see why this change will be simpler if we restrict it to group_leader.

And note that zap_other_threads() is fine in any case, it is called under ->siglock.

Quoted 5 lines

Quoted 4 lines

in particular, http://marc.info/?l=linux-kernel&m=127714242731448
I think this should work, but then we should do something with the users like zap_threads().

Quoted 2 lines

With that patch, won't you potentially miss the exec thread if an exec occurs while you're iterating over the list? Is that OK?

Of course it is not OK ;) Note the "we should do something with" above.

Oleg.

Quoted 10 lines · Frederic Weisbecker

Quoted 5 lines · Mandeep Singh Baines

If you call exec from a thread other than g, g is now unlinked. So "t != g" will always be true. If you then pthread_create, you now have two threads so "t != __prev" will also always be true. So you now have an infinite loop.

Oh you're right.

Quoted 2 lines

But then we can't use t != t->group_leader because that assumes while_each_thread() started on the leader.

Yes, this can't work.

Besides, we need more burriers to rely on the ->group_leader check.

See http://marc.info/?t=127688987300002

in particular, http://marc.info/?l=linux-kernel&m=127714242731448
I think this should work, but then we should do something with the users like zap_threads().

Oleg.

Quoted 5 lines · Mandeep Singh Baines

If you call exec from a thread other than g, g is now unlinked. So "t != g" will always be true. If you then pthread_create, you now have two threads so "t != __prev" will also always be true. So you now have an infinite loop.

Oh you're right.

But then we can't use t != t->group_leader because that assumes while_each_thread() started on the leader.

Quoted 37 lines · Frederic Weisbecker

Quoted 35 lines · Mandeep Singh Baines

Quoted 19 lines · Frederic Weisbecker

Quoted 10 lines · Oleg Nesterov

Quoted 7 lines · Frederic Weisbecker

Hi,

Starring at some parts of cgroups, I have a few questions:

- Is cgroup_enable_task_cg_list()'s while_each_thread() safe against concurrent exec()? The leader may change in de_thread() and invalidate the test done in while_each_thread().

Yes. Oh, we need to do something with while_each_thread.

Would something like this work?

include/linux/sched.h +0 -0

index c0c5876..e002a00 100644

include/linux/sched.h extern bool current_is_single_threaded(void); · line 2300

Quoted 10 lines

include/linux/sched.h +5 -2

@@ -2293,8 +2293,12 @@ 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
 
-#define while_each_thread(g, t) \
-	while ((t = next_thread(t)) != g)
+#define while_each_thread(g, t)					\
+	while (({						\
+		struct task_struct *__prev = t;			\
+		t = next_thread(t);				\
+		t != __prev && t != g;				\

Hi,

Don't you still have an (highly unlikely) race if you exec and then pthread_create()?

Quoted 1 line

I'm not sure what you mean.

Here is what I'm thinking:

If you call exec from a thread other than g, g is now unlinked. So "t != g" will always be true. If you then pthread_create, you now have two threads so "t != __prev" will also always be true. So you now have an infinite loop.

Quoted 5 lines

Quoted 5 lines

Instead of:

Quoted 7 lines

Quoted 5 lines

t != __prev && t != g;

How about:

t != t->group_leader;

That might work too but we need a pair of memory barriers.

next_thread() calls list_entry_rcu. Shouldn't that protect against a dereference? You don't need to synchronize group_leader since you are only using it as a value. You don't dereference it.

Regards,
Mandeep

Quoted 10 lines · Oleg Nesterov

Quoted 7 lines · Frederic Weisbecker

Hi,

Starring at some parts of cgroups, I have a few questions:

- Is cgroup_enable_task_cg_list()'s while_each_thread() safe against concurrent exec()? The leader may change in de_thread() and invalidate the test done in while_each_thread().

Yes. Oh, we need to do something with while_each_thread.

Would something like this work?

include/linux/sched.h +0 -0

index c0c5876..e002a00 100644

include/linux/sched.h +1 -0

+	}))
 
 static inline int get_nr_threads(struct task_struct *tsk)
 {

Quoted 7 lines · Frederic Weisbecker

Hi,

Starring at some parts of cgroups, I have a few questions:

- Is cgroup_enable_task_cg_list()'s while_each_thread() safe against concurrent exec()? The leader may change in de_thread() and invalidate the test done in while_each_thread().

Yes. Oh, we need to do something with while_each_thread.

Quoted 14 lines · Frederic Weisbecker

- Is the check for use_task_css_set_links in cgroup_post_fork() safe? given it is checked outside css_set_lock?

Imagine this:

CPU 0 CPU 1
---- -----

cgroup_enable_task_cg() {
	uset_tasks_css_set_links = 1
	for_each_thread() {
		add tasks in the list
	}
}

Quoted 6 lines

do_fork() {
cgroup_post_fork() {
use_tasks_css_set_links appears to be equal to 0 due to write/read
not flushed. New task won't appear to the list.

Yes, I was thinking about this too.

Or (I think) they can race "contrariwise". CPU_1 creates the new child, then CPU_0 sets uset_tasks_css_set_links = 1. But afaics there is no any guarantee that CPU_0 sees the result of list_add_tail_rcu().

Quoted 2 lines · Frederic Weisbecker

- By the time we call cgroup_post_fork(), it is ready to be woken up and usable by the scheduler.

No, the new child can't run until do_fork()->wake_up_new_task().

Quoted 5 lines · Oleg Nesterov

Quoted 2 lines · Frederic Weisbecker

- By the time we call cgroup_post_fork(), it is ready to be woken up and usable by the scheduler.

No, the new child can't run until do_fork()->wake_up_new_task().

Out of curiosity, why is it not possible for a task to kill and wake up the child before that happens?

Quoted 10 lines · Frederic Weisbecker

Quoted 5 lines · Oleg Nesterov

Quoted 2 lines · Frederic Weisbecker

- By the time we call cgroup_post_fork(), it is ready to be woken up and usable by the scheduler.

No, the new child can't run until do_fork()->wake_up_new_task().

Out of curiosity, why is it not possible for a task to kill and wake up the child before that happens?

Because it is not possible to wake it up.

Please note that copy_process() creates the "deactivated" child, iow it is not on rq.

But, at the same time its ->state == TASK_RUNNING. This "fools" try_to_wake_up() or anything else which in theory could place it on the runqueue.

Except, of course, wake_up_new_task() does activate_task(). And note that it does this unconditionally, exactly because we know that this task can't be woken.

Quoted 10 lines · Frederic Weisbecker

Quoted 5 lines · Oleg Nesterov

Quoted 2 lines · Frederic Weisbecker

- By the time we call cgroup_post_fork(), it is ready to be woken up and usable by the scheduler.

No, the new child can't run until do_fork()->wake_up_new_task().

Out of curiosity, why is it not possible for a task to kill and wake up the child before that happens?

Because it is not possible to wake it up.

Please note that copy_process() creates the "deactivated" child, iow it is not on rq.

But, at the same time its ->state == TASK_RUNNING. This "fools" try_to_wake_up() or anything else which in theory could place it on the runqueue.

Except, of course, wake_up_new_task() does activate_task(). And note that it does this unconditionally, exactly because we know that this task can't be woken.

Quoted 14 lines · Frederic Weisbecker

- Is the check for use_task_css_set_links in cgroup_post_fork() safe? given it is checked outside css_set_lock?

Imagine this:

CPU 0 CPU 1
---- -----

cgroup_enable_task_cg() {
	uset_tasks_css_set_links = 1
	for_each_thread() {
		add tasks in the list
	}
}

Quoted 6 lines

do_fork() {
cgroup_post_fork() {
use_tasks_css_set_links appears to be equal to 0 due to write/read
not flushed. New task won't appear to the list.

Yes, I was thinking about this too.

Or (I think) they can race "contrariwise". CPU_1 creates the new child, then CPU_0 sets uset_tasks_css_set_links = 1. But afaics there is no any guarantee that CPU_0 sees the result of list_add_tail_rcu().

Hi,

Starring at some parts of cgroups, I have a few questions:

- Is cgroup_enable_task_cg_list()'s while_each_thread() safe against concurrent exec()? The leader may change in de_thread() and invalidate the test done in while_each_thread().

- do_each_thread() also needs RCU and cgroup_enable_task_cg_list()
seems to remind it. But it seems there is at least one caller that
doesn't call rcu_read_lock(): update_cpu_mask() -> update_tasks_cpumask() -> cgroup_scan_tasks()

- By the time we call cgroup_post_fork(), it is ready to be woken up and usable by the scheduler.

- Is the check for use_task_css_set_links in cgroup_post_fork() safe? given it is checked outside css_set_lock?

Imagine this:

CPU 0 CPU 1
---- -----

cgroup_enable_task_cg() {
	uset_tasks_css_set_links = 1
	for_each_thread() {
		add tasks in the list
	}
}

do_fork() {
cgroup_post_fork() {
use_tasks_css_set_links appears to be equal to 0 due to write/read
not flushed. New task won't appear to the list.