fs, proc: Introduce the /proc/<pid>/children entry v2

There is no easy way to make a reverse parent->children chain from the task status, in turn children->parent provided with "PPid" field.

So instead of walking over all pids in system to figure out what children the task have -- we add explicit /proc/<pid>/children entry, since kernel already knows this kind of information but it was not yet exported.

...

fs/proc/array.c +15 -0

+static void *children_seq_start(struct seq_file *seq, loff_t *pos)
+{
+	struct task_struct *task;
+
+	rcu_read_lock();
+	task = seq->private;
+	if (task)
+		return seq_list_start(&task->children, *pos);
+
+	return NULL;
+}
+
+static void *children_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+	struct task_struct *task = seq->private;

It's also worth adding a warning about this into the documentation for this procfs file. Documentation which this patch forgot to provide, btw!

On Tue, Dec 06, 2011 at 02:33:13PM -0800, Andrew Morton wrote:
...

Quoted 6 lines

It's also worth adding a warning about this into the documentation for this procfs file. Documentation which this patch forgot to provide, btw!

Yes, thanks for comments Andrew, I already got the feeling that I've missed documentation update from the start_brk /proc patch ;)

I'll address all issues, thanks!

Hello,

fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 581

Quoted 5 lines · Cyrill Gorcunov

fs/proc/array.c +5 -0

+static void children_seq_stop(struct seq_file *seq, void *v)
+{
+	rcu_read_unlock();
+	seq_printf(seq, "\n");
+}

Wouldn't this add newline on page boundary? Newline should be printed at the end of the whole sequence but seq_stop() is called at buffer boundaries.

Hi Cyrill,

Sorry, I didn't read this patch yet, but

fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 564

Quoted 9 lines · Cyrill Gorcunov

fs/proc/array.c +8 -0

+static void *children_seq_start(struct seq_file *seq, loff_t *pos)
+{
+	struct task_struct *task;
+
+	rcu_read_lock();
+	task = seq->private;
+	if (task)
+		return seq_list_start(&task->children, *pos);

This looks "obviously wrong".

We can not trust ->children->next after rcu_read_unlock(). Another rcu_read_lock() can't help.

Once again, I can be easily wrong, need to read the patch first.

Quoted 17 lines · Oleg Nesterov

Hi Cyrill,

Sorry, I didn't read this patch yet, but

fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 564

Quoted 9 lines · Cyrill Gorcunov

fs/proc/array.c +8 -0

+static void *children_seq_start(struct seq_file *seq, loff_t *pos)
+{
+	struct task_struct *task;
+
+	rcu_read_lock();
+	task = seq->private;
+	if (task)
+		return seq_list_start(&task->children, *pos);

This looks "obviously wrong".

Quoted 5 lines

We can not trust ->children->next after rcu_read_unlock(). Another rcu_read_lock() can't help.

Once again, I can be easily wrong, need to read the patch first.

Wait, Oleg, I might be wrong as well, but it's now as

children_seq_open
get_proc_task (so ref to task increased)

the children_seq_start/children_seq_stop works in iteration and every new iteration seq_list_next walks over the whole children list from the list head under rcu lock, so even if task is removed or added the link should exsist until rcu is unlocked and sync'ed no?

Quoted 14 lines

Quoted 8 lines

This looks "obviously wrong".

We can not trust ->children->next after rcu_read_unlock(). Another rcu_read_lock() can't help.

Once again, I can be easily wrong, need to read the patch first.

Wait, Oleg, I might be wrong as well, but it's now as

children_seq_open
get_proc_task (so ref to task increased)

Quoted 7 lines

the children_seq_start/children_seq_stop works in iteration and every new iteration seq_list_next walks over the whole children list from the list head under rcu lock, so even if task is removed or added the link should exsist until rcu is unlocked and sync'ed no?

On the other hands some if (task) tests are redundant and might be dropped since we have a reference to a task until seq-file is not released. I'll update it and shrink a patch some more.

So while adding new task to a list is not a problem (the reader will simply not notice it) removing task from a list is a bit tricky, but as far as I see switch_task_namespaces (from exit_notify) uses synchronize_rcu as well as release_task calls for call_rcu for put_task_struct) so I think we will not get any wrong dereference in

static int children_seq_show(struct seq_file *seq, void *v)

{
	struct task_struct *task = container_of(v, struct task_struct, sibling);
	return seq_printf(seq, " %lu", (unsigned long)pid_vnr(task_pid(task)));
}

while we're in rcu reader section. I might be wrong of course, so please verify this claim.

Quoted 20 lines · Oleg Nesterov

Hi Cyrill,

Sorry, I didn't read this patch yet, but

fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 564

Quoted 9 lines · Cyrill Gorcunov

fs/proc/array.c +8 -0

+static void *children_seq_start(struct seq_file *seq, loff_t *pos)
+{
+	struct task_struct *task;
+
+	rcu_read_lock();
+	task = seq->private;
+	if (task)
+		return seq_list_start(&task->children, *pos);

This looks "obviously wrong".

We can not trust ->children->next after rcu_read_unlock(). Another rcu_read_lock() can't help.

Quoted 2 lines

Once again, I can be easily wrong, need to read the patch first.

Wait, Oleg, I might be wrong as well, but it's now a

children_seq_open
get_proc_task (so ref to task increased)

the children_seq_start/children_seq_stop work in iteration and every new iteration seq_list_next walks over the whole children list from the list head under rcu lock,

Quoted 5 lines

Wait, Oleg, I might be wrong as well, but it's now a

children_seq_open
get_proc_task (so ref to task increased)

Yes. task_struct itself can't go away.

Quoted 4 lines

the children_seq_start/children_seq_stop work in iteration and every new iteration seq_list_next walks over the whole children list from the list head under rcu lock,

Yep, I misread this code, I though it does _next.

However, ->children list is not rcu-safe, this means that even list_for_each() itself is not safe. Either you need tasklist or we can probably make it rcu-safe...

As for /proc/pid/children, personally I think it is very useful. But note that it obviously reports the children per-thread, while in general this is the per-process thing. Not sure this really

makes sense, but perhaps /proc/pid/children and /proc/pid/task/tid/children should act differently. Like, say, proc_tid_stat/proc_tgid_stat. I won't insist.

However, ->children list is not rcu-safe, this means that even list_for_each() itself is not safe. Either you need tasklist or we can probably make it rcu-safe...

On Thu, Dec 08, 2011 at 05:35:35PM +0100, Oleg Nesterov wrote:
...

Quoted 5 lines

However, ->children list is not rcu-safe, this means that even list_for_each() itself is not safe. Either you need tasklist or we can probably make it rcu-safe...

Andrew, Oleg, does the below one look more less fine? Note the tasklist_lock is back and it worries me a bit since I imagine one could be endlessly reading some /proc/<pid>/children file increasing contention over this lock on the whole system (regardless the fact that it's take for read only).

---
From: Cyrill Gorcunov <[email protected]>
Subject: [PATCH] fs, proc: Introduce the /proc/<pid>/children entry v4

There is no easy way to make a reverse parent->children chain from arbitrary <pid> (while parent pid is provided in "PPid" field of /proc/<pid>/status).

So instead of walking over all pids in the system to figure out which children a task have -- we add explicit /proc/<pid>/children entry, because kernel already has this kind of information but it is not yet exported. This is a first level children, not the whole process tree, neither the process threads are identified with this interface.

Quoted 8 lines · On Fri, 9 Dec 2011 01:28:53 +0400 Cyrill Gorcunov <[email protected]> wrote:

On Thu, Dec 08, 2011 at 05:35:35PM +0100, Oleg Nesterov wrote:
...

Quoted 5 lines

However, ->children list is not rcu-safe, this means that even list_for_each() itself is not safe. Either you need tasklist or we can probably make it rcu-safe...

Quoted 5 lines

Andrew, Oleg, does the below one look more less fine? Note the tasklist_lock is back and it worries me a bit since I imagine one could be endlessly reading some /proc/<pid>/children file increasing contention over this lock on the whole system (regardless the fact that it's take for read only).

It is a potential problem, from the lock-hold point of view and also it can cause large scheduling latencies. What's involved in making ->children an rcu-protected list?

Quoted 8 lines

---
From: Cyrill Gorcunov <[email protected]>
Subject: [PATCH] fs, proc: Introduce the /proc/<pid>/children entry v4

There is no easy way to make a reverse parent->children chain from arbitrary <pid> (while parent pid is provided in "PPid" field of /proc/<pid>/status).

Quoted 5 lines

So instead of walking over all pids in the system to figure out which children a task have -- we add explicit /proc/<pid>/children entry, because kernel already has this kind of information but it is not yet exported. This is a first level children, not the whole process tree, neither the process threads are identified with this interface.

The changelog doesn't explain why we want the patch, so there's no reason to merge it! Something to do with c/r, yes?

If so, I guess the feature could/should be configurable. Probably with a CONFIG_PROC_CHILDREN which is selected by CONFIG_CHECKPOINT_RESTORE. Which is all getting a bit over the top, but I suppose we must do it.

Also, neither the changelog not the documentation mention the loss-of-data problem which might occur if/when the lock is dropped.

The code now appears to be kinda-duplicating functionality which the seq_file library provides. Shouldn't this have been changelogged/commented? If it was, I wouldn't need to ask the next question.

Why is it kinda-duplicating seq_file functionality? Can we strengthen the seq_file code so this is unnecessary?

Quoted 8 lines · On Fri, 9 Dec 2011 01:28:53 +0400 Cyrill Gorcunov <[email protected]> wrote:

On Thu, Dec 08, 2011 at 05:35:35PM +0100, Oleg Nesterov wrote:
...

Quoted 5 lines

However, ->children list is not rcu-safe, this means that even list_for_each() itself is not safe. Either you need tasklist or we can probably make it rcu-safe...

Quoted 5 lines

Andrew, Oleg, does the below one look more less fine? Note the tasklist_lock is back and it worries me a bit since I imagine one could be endlessly reading some /proc/<pid>/children file increasing contention over this lock on the whole system (regardless the fact that it's take for read only).

It is a potential problem, from the lock-hold point of view and also it can cause large scheduling latencies. What's involved in making ->children an rcu-protected list?

Quoted 7 lines

From: Cyrill Gorcunov <[email protected]>
Subject: [PATCH] fs, proc: Introduce the /proc/<pid>/children entry v4

There is no easy way to make a reverse parent->children chain from arbitrary <pid> (while parent pid is provided in "PPid" field of /proc/<pid>/status).

Quoted 5 lines

So instead of walking over all pids in the system to figure out which children a task have -- we add explicit /proc/<pid>/children entry, because kernel already has this kind of information but it is not yet exported. This is a first level children, not the whole process tree, neither the process threads are identified with this interface.

The changelog doesn't explain why we want the patch, so there's no reason to merge it! Something to do with c/r, yes?

If so, I guess the feature could/should be configurable. Probably with a CONFIG_PROC_CHILDREN which is selected by CONFIG_CHECKPOINT_RESTORE. Which is all getting a bit over the top, but I suppose we must do it.

Quoted 4 lines

It is a potential problem, from the lock-hold point of view and also it can cause large scheduling latencies. What's involved in making ->children an rcu-protected list?

At first glance, this doesn't look trivial... forget_original_parent() abuses ->sibling.

But yes, it is not really nice to hold tasklist_lock here. May be we can change this code so that every iteration records the reported task_struct and then tries to continue. This means we should verify that ->real_parent is still the same under tasklist, but at least this way we do not hold it throughout.

Quoted 3 lines

If so, I guess the feature could/should be configurable. Probably with a CONFIG_PROC_CHILDREN which is selected by CONFIG_CHECKPOINT_RESTORE. Which is all getting a bit over the top, but I suppose we must do it.

Heh. This is the rare case when I personally like the new feature ;) I mean, it looks "obviously useful" to me. If nothing else, it can help to debug the problems. Probably the tools like pstree can use it.

Personally I'd even prefer /proc/pid/children/ directory (like /proc/pid/task), but I guess this needs much more complications.

On Fri, Dec 09, 2011 at 04:30:09PM +0100, Oleg Nesterov wrote:
...

Quoted 8 lines

Quoted 4 lines

It is a potential problem, from the lock-hold point of view and also it can cause large scheduling latencies. What's involved in making ->children an rcu-protected list?

At first glance, this doesn't look trivial... forget_original_parent() abuses ->sibling.

But wait, forget_original_parent may move task out of the original list and put it in dead_children list, right? Then release_task may call delayed_put_task_struct under as call_rcu which should wait until we finish reading in our rcu section, isn't it?

Even if we get inconsistent picture of children (ie we get pid of shildren which just getting dead) I think it's fine.

Quoted 6 lines

But yes, it is not really nice to hold tasklist_lock here. May be we can change this code so that every iteration records the reported task_struct and then tries to continue. This means we should verify that ->real_parent is still the same under tasklist, but at least this way we do not hold it throughout.

And if real_parent is changed,

I should simply skip such task and
continue, right?

Quoted 3 lines

Personally I'd even prefer /proc/pid/children/ directory (like /proc/pid/task), but I guess this needs much more complications.

I fear so. Oleg, if it possible I would like to bring in as minimum code as I can ;)

To simplify, let's talk about children_seq_start() your patch adds. Suppose that we make ->children/sibling "rcu-safe" (we replace __unhash_process()->list_del_init() with list_del_rcu, and so on).

Now, children_seq_start() does:

	rcu_read_lock();

	list_for_each(child, task->children)
		...;

Suppose that this task exits and reparents the child we are looking at. Once reparent_leader() moves it into another list, list_for_each() can never stop.

In short. forget_original_parent() changes the _head_ of the list, in some sense.