fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v6

RFC In review Cyrill Gorcunov 4 participants 20 messages Jan 16, 2012 – Jan 19, 2012

Subsystems (2)

Documentation Proc filesystem

lore

Share & tools

lore

Cyrill Gorcunov

author

2012-01-16 15:32 UTC #0 lore raw

RFC 1/1 fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v6

When we do checkpoint of a task we need to know the list of children the task, has but there is no easy and fast way to generate reverse parent->children chain from arbitrary <pid> (while a parent pid is provided in "PPid" field of /proc/<pid>/status).

So instead of walking over all pids in the system (creating one big process tree in memory, just to figure out which children a task has) -- we add explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has this kind of information but it is not yet exported.

This is a first level children, not the whole process tree.

v2:
- Kame suggested to use a separated /proc/<pid>/children entry instead of poking /proc/<pid>/status
- Andew suggested to use rcu facility instead of locking tasklist_lock
- Tejun pointed that non-seekable seq file might not be enough for tasks with large number of children

v3:
- To be on a safe side use %lu format for pid_t printing

v4:
- New line get printed when sequence ends not at seq->stop, a nit pointed by Tejun
- Documentation update
- tasklist_lock is back, Oleg pointed that ->children list is actually not rcu-safe

v5:
- Oleg suggested to make /proc/<pid>/task/<tid>/children instead of global /proc/<pid>/children, which eliminates hardness related to threads and children migration, and allows patch to be a way simplier.

v6:
- Drop ptrace_may_access tests, pids are can be found anyway so nothing to protect here.

Signed-off-by Cyrill Gorcunov <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Oleg Nesterov <[email protected]>

---
 Documentation/filesystems/proc.txt |   18 ++++
 fs/proc/array.c                    |  146 +++++++++++++++++++++++++++++++++++++
 fs/proc/base.c                     |    1 
 fs/proc/internal.h                 |    1 
 4 files changed, 166 insertions(+)

Documentation/filesystems/proc.txt +18 -0

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
@@ -40,6 +40,7 @@ Table of Contents
   3.4	/proc/<pid>/coredump_filter - Core dump filtering settings
   3.5	/proc/<pid>/mountinfo - Information about mounts
   3.6	/proc/<pid>/comm  & /proc/<pid>/task/<tid>/comm
+  3.7   /proc/<pid>/task/<tid>/children - Information about task children
 
   4	Configuring procfs
   4.1	Mount options

@@ -1549,6 +1550,23 @@ then the kernel's TASK_COMM_LEN (current
 comm value.
 
 
+3.7	/proc/<pid>/task/<tid>/children - Information about task children
+-------------------------------------------------------------------------
+This file provides a fast way to retrieve first level children pids
+of a task pointed by <pid>/<tid> pair. The format is a space separated
+stream of pids.
+
+Note the "first level" here -- if a child has own children they will
+not be listed here, one needs to read /proc/<children-pid>/task/<tid>/children
+to obtain the descendants.
+
+Since this interface is intended to be fast and cheap it doesn't
+guarantee to provide precise results, which means if there a new child
+created while we read the "children" file -- it might or might not be
+counted. So one need to either stop or freeze processes if precise
+results are needed.
+
+
 ------------------------------------------------------------------------------
 Configuring procfs
 ------------------------------------------------------------------------------

fs/proc/array.c +146 -0

149 lines not on this page · raw message

fs/proc/base.c +1 -0

Index: linux-2.6.git/fs/proc/base.c
===================================================================
@@ -3454,6 +3454,7 @@ static const struct pid_entry tid_base_s
 	ONE("stat",      S_IRUGO, proc_tid_stat),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
 	REG("maps",      S_IRUGO, proc_maps_operations),
+	REG("children",  S_IRUGO, proc_tid_children_operations),
 #ifdef CONFIG_NUMA
 	REG("numa_maps", S_IRUGO, proc_numa_maps_operations),
 #endif

fs/proc/internal.h +1 -0

Index: linux-2.6.git/fs/proc/internal.h
===================================================================
@@ -53,6 +53,7 @@ extern int proc_pid_statm(struct seq_fil
 				struct pid *pid, struct task_struct *task);
 extern loff_t mem_lseek(struct file *file, loff_t offset, int orig);
 
+extern const struct file_operations proc_tid_children_operations;
 extern const struct file_operations proc_maps_operations;
 extern const struct file_operations proc_numa_maps_operations;
 extern const struct file_operations proc_smaps_operations;

Oleg Nesterov

2012-01-16 16:17 UTC #1 lore raw

Quoted 10 lines · Cyrill Gorcunov

fs/proc/array.c +9 -0

+get_children_pid(struct proc_pid_children_iter *iter, struct pid *pid_prev, loff_t pos)
+{
+	struct task_struct *start, *task;
+	struct pid *pid = NULL;
+
+	read_lock(&tasklist_lock);
+
+	start = pid_task(iter->pid_start, PIDTYPE_PID);
+	if (!start)

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

Quoted 10 lines

fs/proc/array.c +10 -0

+		goto out;
+
+	/*
+	 * Lets try to continue searching first, this gives
+	 * us significant speedup on children-rich processes.
+	 */
+	if (pid_prev) {
+		task = pid_task(pid_prev, PIDTYPE_PID);
+		if (task && task->real_parent == start &&
+		    !(list_empty(&task->sibling))) {

Damn. No, this is wrong.

Damn! Yes, it was we who told you to check list_empty(sibling) ;)

But this is not enough. exit_ptrace() can do list_move() without changing ->real_parent.

I'll try to think. At first glance we can rely on EXIT_DEAD, but I'd like to avoid this, I think EXIT_DEAD should die.

Oleg.

Cyrill Gorcunov

2012-01-16 16:20 UTC #2 lore raw

Quoted 24 lines · Oleg Nesterov

Quoted 10 lines · Cyrill Gorcunov

fs/proc/array.c +9 -0

+get_children_pid(struct proc_pid_children_iter *iter, struct pid *pid_prev, loff_t pos)
+{
+	struct task_struct *start, *task;
+	struct pid *pid = NULL;
+
+	read_lock(&tasklist_lock);
+
+	start = pid_task(iter->pid_start, PIDTYPE_PID);
+	if (!start)

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

Quoted 10 lines

fs/proc/array.c +10 -0

+		goto out;
+
+	/*
+	 * Lets try to continue searching first, this gives
+	 * us significant speedup on children-rich processes.
+	 */
+	if (pid_prev) {
+		task = pid_task(pid_prev, PIDTYPE_PID);
+		if (task && task->real_parent == start &&
+		    !(list_empty(&task->sibling))) {

Damn. No, this is wrong.

Quoted 8 lines

Damn! Yes, it was we who told you to check list_empty(sibling) ;)
But this is not enough. exit_ptrace() can do list_move() without changing ->real_parent.
I'll try to think. At first glance we can rely on EXIT_DEAD, but I'd like to avoid this, I think EXIT_DEAD should die.

Ouch! Thanks for catching this Oleg. I'll try to come with something to show as well.

Cyrill

Oleg Nesterov

2012-01-17 17:47 UTC #3 lore raw

Quoted 28 lines · Cyrill Gorcunov

Quoted 26 lines · Oleg Nesterov
Quoted 10 lines · Cyrill Gorcunov
fs/proc/array.c +9 -0
+get_children_pid(struct proc_pid_children_iter *iter, struct pid *pid_prev, loff_t pos)
+{
+	struct task_struct *start, *task;
+	struct pid *pid = NULL;
+
+	read_lock(&tasklist_lock);
+
+	start = pid_task(iter->pid_start, PIDTYPE_PID);
+	if (!start)
fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 575
Quoted 10 lines
fs/proc/array.c +10 -0
+		goto out;
+
+	/*
+	 * Lets try to continue searching first, this gives
+	 * us significant speedup on children-rich processes.
+	 */
+	if (pid_prev) {
+		task = pid_task(pid_prev, PIDTYPE_PID);
+		if (task && task->real_parent == start &&
+		    !(list_empty(&task->sibling))) {
Damn. No, this is wrong.
Damn! Yes, it was we who told you to check list_empty(sibling) ;)

Quoted 8 lines

Quoted 5 lines
But this is not enough. exit_ptrace() can do list_move() without changing ->real_parent.
I'll try to think. At first glance we can rely on EXIT_DEAD, but I'd like to avoid this, I think EXIT_DEAD should die.
Ouch! Thanks for catching this Oleg. I'll try to come with something to show as well.

Do you see another approach? I don't, so I'd suggest to check "task->exit_state != EXIT_DEAD" instead of !list_empty().

Just in case, we can also check "start->exit_state == 0" instead of "task->real_parent == start" with the same effect, up to you.

It would be nice to add the comment explaining these checks...

And I forgot to mention, the comment below

	> +			/*
	> +			 * We might miss some freshly created children
	> +			 * here, but it was never promised to be
	> +			 * accurate.
	> +			 */
	> +			if (list_is_last(&task->sibling, &start->children))
	> +				goto out;

looks misleading. Contrary to the slow path, we can't miss the freshly forked child here, copy_process() does list_add_tail().

But the slow path obviously can skip much more than needed and miss children (freshly forked or not), probably it would be better to move the comment down and remove the "freshly created" part.

What do you think?

Oleg.

Cyrill Gorcunov

2012-01-17 17:57 UTC #4 lore raw

On Tue, Jan 17, 2012 at 06:40:49PM +0100, Oleg Nesterov wrote:
...

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
But this is not enough. exit_ptrace() can do list_move() without changing ->real_parent.

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
I'll try to think. At first glance we can rely on EXIT_DEAD, but I'd like to avoid this, I think EXIT_DEAD should die.
Ouch! Thanks for catching this Oleg. I'll try to come with something to show as well.
Do you see another approach? I don't, so I'd suggest to check "task->exit_state != EXIT_DEAD" instead of !list_empty().

Well, I thought what if I can find another way without EXIT_DEAD but seems there is no luck.

Quoted 3 lines

Just in case, we can also check "start->exit_state == 0" instead of "task->real_parent == start" with the same effect, up to you.

real_parent == start somehow more informative for me so if you allow (and noone against) I would leave the current form.

Quoted 2 lines

It would be nice to add the comment explaining these checks...

Yeah, I'll add ones. Sure.

Quoted 10 lines

And I forgot to mention, the comment below

	> +			/*
	> +			 * We might miss some freshly created children
	> +			 * here, but it was never promised to be
	> +			 * accurate.
	> +			 */
	> +			if (list_is_last(&task->sibling, &start->children))
	> +				goto out;

Quoted 3 lines

looks misleading. Contrary to the slow path, we can't miss the freshly forked child here, copy_process() does list_add_tail().

Ah, crap, indeed!

Quoted 7 lines

But the slow path obviously can skip much more than needed and miss children (freshly forked or not), probably it would be better to move the comment down and remove the "freshly created" part.
What do you think?

Yeah, thanks a lot, Oleg. I'll update it an post for review (I hope to finish it tonight ;)

Cyrill

Oleg Nesterov

2012-01-17 18:20 UTC #5 lore raw

Quoted 13 lines · Cyrill Gorcunov

On Tue, Jan 17, 2012 at 06:40:49PM +0100, Oleg Nesterov wrote:
...
Quoted 10 lines
Quoted 9 lines
Quoted 6 lines
But this is not enough. exit_ptrace() can do list_move() without changing ->real_parent.
I'll try to think. At first glance we can rely on EXIT_DEAD, but I'd like to avoid this, I think EXIT_DEAD should die.
Ouch! Thanks for catching this Oleg. I'll try to come with something to show as well.

Quoted 6 lines

Quoted 3 lines
Do you see another approach? I don't, so I'd suggest to check "task->exit_state != EXIT_DEAD" instead of !list_empty().
Well, I thought what if I can find another way without EXIT_DEAD but seems there is no luck.

Oooooh. Cyrill, it seems I managed to confuse you. And myself.

exit_ptrace() doesn't use ->sibling, it uses ->ptrace_entry!.

Sorry!

Oleg.

Cyrill Gorcunov

2012-01-17 18:30 UTC #6 lore raw

On Tue, Jan 17, 2012 at 07:14:36PM +0100, Oleg Nesterov wrote:
...

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
Do you see another approach? I don't, so I'd suggest to check "task->exit_state != EXIT_DEAD" instead of !list_empty().

Quoted 9 lines

Quoted 2 lines
Well, I thought what if I can find another way without EXIT_DEAD but seems there is no luck.
Oooooh. Cyrill, it seems I managed to confuse you. And myself.
exit_ptrace() doesn't use ->sibling, it uses ->ptrace_entry!.
Sorry!

Heh ;) No worries, I seem to not understand this code enough anyway, so I'll re-check it again and post an updated version of the patch which should be reviewed with great caution.

Cyrill

KOSAKI Motohiro

2012-01-17 21:38 UTC #7 lore raw

In reply to Cyrill Gorcunov

Quoted 9 lines · Cyrill Gorcunov

When we do checkpoint of a task we need to know the list of children the task, has but there is no easy and fast way to generate reverse parent->children chain from arbitrary<pid> (while a parent pid is provided in "PPid" field of /proc/<pid>/status).
So instead of walking over all pids in the system (creating one big process tree in memory, just to figure out which children a task has) -- we add explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has this kind of information but it is not yet exported.

I doubt this is good idea. It move some complexity to userland, but not reduce. Again, if we add this interface, it should help pstree like process traversal tools. Bare task hierarchy shouldn't be exposed userland. I believe users need sub process, not sub threads.

Cyrill Gorcunov

2012-01-18 09:43 UTC #8 lore raw

Quoted 11 lines · KOSAKI Motohiro

Quoted 9 lines · Cyrill Gorcunov
When we do checkpoint of a task we need to know the list of children the task, has but there is no easy and fast way to generate reverse parent->children chain from arbitrary<pid> (while a parent pid is provided in "PPid" field of /proc/<pid>/status).
So instead of walking over all pids in the system (creating one big process tree in memory, just to figure out which children a task has) -- we add explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has this kind of information but it is not yet exported.

Quoted 5 lines

I doubt this is good idea. It move some complexity to userland, but not reduce. Again, if we add this interface, it should help pstree like process traversal tools. Bare task hierarchy shouldn't be exposed userland. I believe users need sub process, not sub threads.

Which exactly complexity it moves to user-space? You have some task, and now you can find all children easily, what the complexity you're talking about? pstree is building the whole process tree going through all entries in /proc/<pid>, reading PPid field and then forming the topology. I would like to be able to find children faster. So I readdir a /proc/<pid>/task/ and the walk over every /proc/<pid>/task/<tid>/children. This helps alot.

Cyrill

Oleg Nesterov

2012-01-18 14:04 UTC #9 lore raw

In reply to KOSAKI Motohiro

Quoted 12 lines · KOSAKI Motohiro

Quoted 9 lines · Cyrill Gorcunov
When we do checkpoint of a task we need to know the list of children the task, has but there is no easy and fast way to generate reverse parent->children chain from arbitrary<pid> (while a parent pid is provided in "PPid" field of /proc/<pid>/status).
So instead of walking over all pids in the system (creating one big process tree in memory, just to figure out which children a task has) -- we add explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has this kind of information but it is not yet exported.

Quoted 4 lines

I doubt this is good idea. It move some complexity to userland, but not reduce. Again, if we add this interface, it should help pstree like process traversal tools. Bare task hierarchy shouldn't be exposed userland. I believe users need sub process, not sub threads.

IOW, you mean that the reading from 'children' should list all children of the whole thread group. Yes, we discussed this before a bit.

In this case this file should live in /proc/pid/, not in /proc/pid/task/tid. But this doesn't allow to restore the hierarchy correctly, you need to know the parent thread. So I think /proc/pid/tasks/tid/children is still needed. And I think it does help pstree-like apps, just they need to look into task/.

As for /proc/pid/children, may be we can add it later. But this is not that simple. The problem is the threaded reparenting, we do not want to list the same child twice.

Oleg.

Cyrill Gorcunov

2012-01-18 14:22 UTC #10 lore raw

Quoted 21 lines · Oleg Nesterov

Quoted 12 lines · KOSAKI Motohiro
Quoted 9 lines · Cyrill Gorcunov
When we do checkpoint of a task we need to know the list of children the task, has but there is no easy and fast way to generate reverse parent->children chain from arbitrary<pid> (while a parent pid is provided in "PPid" field of /proc/<pid>/status).
So instead of walking over all pids in the system (creating one big process tree in memory, just to figure out which children a task has) -- we add explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has this kind of information but it is not yet exported.
Quoted 4 lines
I doubt this is good idea. It move some complexity to userland, but not reduce. Again, if we add this interface, it should help pstree like process traversal tools. Bare task hierarchy shouldn't be exposed userland. I believe users need sub process, not sub threads.
IOW, you mean that the reading from 'children' should list all children of the whole thread group. Yes, we discussed this before a bit.

Quoted 9 lines

In this case this file should live in /proc/pid/, not in /proc/pid/task/tid. But this doesn't allow to restore the hierarchy correctly, you need to know the parent thread. So I think /proc/pid/tasks/tid/children is still needed. And I think it does help pstree-like apps, just they need to look into task/.
As for /proc/pid/children, may be we can add it later. But this is not that simple. The problem is the threaded reparenting, we do not want to list the same child twice.

Yes, /proc/pid/children was initial target but then I fall back to hardness and though implementing more simplier solution is better approach.

So Oleg, I think you meant something like below? Comment is moved down an list_empty over siblings remans, right?

Btw, should it be under CONFIG_CHECKPOINT_RESTORE or we could live with it in general?

Cyrill
---
From: Cyrill Gorcunov <[email protected]>
Subject: fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v6

This is a first level children, not the whole process tree.

v3:
- To be on a safe side use %lu format for pid_t printing

v4:
- New line get printed when sequence ends not at seq->stop, a nit pointed by Tejun
- Documentation update
- tasklist_lock is back, Oleg pointed that ->children list is actually not rcu-safe

v6:
- Drop ptrace_may_access tests, pids are can be found anyway so nothing to protect here.

Signed-off-by Cyrill Gorcunov <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Oleg Nesterov <[email protected]>

---
 Documentation/filesystems/proc.txt |   18 ++++
 fs/proc/array.c                    |  147 +++++++++++++++++++++++++++++++++++++
 fs/proc/base.c                     |    1 
 fs/proc/internal.h                 |    1 
 4 files changed, 167 insertions(+)

Documentation/filesystems/proc.txt +18 -0

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
@@ -40,6 +40,7 @@ Table of Contents
   3.4	/proc/<pid>/coredump_filter - Core dump filtering settings
   3.5	/proc/<pid>/mountinfo - Information about mounts
   3.6	/proc/<pid>/comm  & /proc/<pid>/task/<tid>/comm
+  3.7   /proc/<pid>/task/<tid>/children - Information about task children
 
   4	Configuring procfs
   4.1	Mount options

@@ -1549,6 +1550,23 @@ then the kernel's TASK_COMM_LEN (current
 comm value.
 
 
+3.7	/proc/<pid>/task/<tid>/children - Information about task children
+-------------------------------------------------------------------------
+This file provides a fast way to retrieve first level children pids
+of a task pointed by <pid>/<tid> pair. The format is a space separated
+stream of pids.
+
+Note the "first level" here -- if a child has own children they will
+not be listed here, one needs to read /proc/<children-pid>/task/<tid>/children
+to obtain the descendants.
+
+Since this interface is intended to be fast and cheap it doesn't
+guarantee to provide precise results, which means if there a new child
+created while we read the "children" file -- it might or might not be
+counted. So one need to either stop or freeze processes if precise
+results are needed.
+
+
 ------------------------------------------------------------------------------
 Configuring procfs
 ------------------------------------------------------------------------------

fs/proc/array.c +147 -0

150 lines not on this page · raw message

fs/proc/base.c +1 -0

Index: linux-2.6.git/fs/proc/base.c
===================================================================
@@ -3454,6 +3454,7 @@ static const struct pid_entry tid_base_s
 	ONE("stat",      S_IRUGO, proc_tid_stat),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
 	REG("maps",      S_IRUGO, proc_maps_operations),
+	REG("children",  S_IRUGO, proc_tid_children_operations),
 #ifdef CONFIG_NUMA
 	REG("numa_maps", S_IRUGO, proc_numa_maps_operations),
 #endif

fs/proc/internal.h +1 -0

Index: linux-2.6.git/fs/proc/internal.h
===================================================================
@@ -53,6 +53,7 @@ extern int proc_pid_statm(struct seq_fil
 				struct pid *pid, struct task_struct *task);
 extern loff_t mem_lseek(struct file *file, loff_t offset, int orig);
 
+extern const struct file_operations proc_tid_children_operations;
 extern const struct file_operations proc_maps_operations;
 extern const struct file_operations proc_numa_maps_operations;
 extern const struct file_operations proc_smaps_operations;

Oleg Nesterov

2012-01-18 14:42 UTC #11 lore raw

Quoted 3 lines · Cyrill Gorcunov

So Oleg, I think you meant something like below? Comment is moved down an list_empty over siblings remans, right?

Yes, except the comment still looks misleading to me.

Otherwise looks correct, but I'll try to re-check once again with the fresh head. Although I think you should remove me from CC: after I found the nonexistent bug ;)

Quoted 6 lines

fs/proc/array.c +5 -0

+get_children_pid(struct proc_pid_children_iter *iter, struct pid *pid_prev, loff_t pos)
+{
...
+	/*
+	 * Slow search case
+	 *

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

Quoted 10 lines

fs/proc/array.c +10 -0

+	 * We might miss some freshly created children
+	 * here, but it was never promised to be
+	 * accurate.
+	 */
+	list_for_each_entry(task, &start->children, sibling) {
+		if (pos-- == 0) {
+			pid = get_pid(task_pid(task));
+			break;
+		}
+	}

This is minor, but "freshly created" looks very confusing to me. What does it mean? We hold tasklist, we can't race with fork().

Yes we can miss a child, but this has nothing to do with "freshly". Just suppose that the parent sleeps, but N children exit after we printed their tids. Now the slow paths skips N extra children, we miss N tasks.

Oleg.

Cyrill Gorcunov

2012-01-18 18:23 UTC #12 lore raw

Quoted 11 lines · Oleg Nesterov

Quoted 3 lines · Cyrill Gorcunov
So Oleg, I think you meant something like below? Comment is moved down an list_empty over siblings remans, right?
Yes, except the comment still looks misleading to me.
Otherwise looks correct, but I'll try to re-check once again with the fresh head. Although I think you should remove me from CC: after I found the nonexistent bug ;)

There is no way back from CC ;)

Quoted 3 lines

This is minor, but "freshly created" looks very confusing to me. What does it mean? We hold tasklist, we can't race with fork().

Hmm. Sure we keep a lock here, but changes might happen between reads If only I'm not missing something again.

Look which scenario I've had in mind. We have a task A and children B,C,D,E. ... Here my scenario ended and I realised that you're right. I'll update the comment.

Quoted 7 lines

Yes we can miss a child, but this has nothing to do with "freshly". Just suppose that the parent sleeps, but N children exit after we printed their tids. Now the slow paths skips N extra children, we miss N tasks.
Oleg.

Cyrill

Cyrill Gorcunov

2012-01-18 19:07 UTC #13 lore raw

Quoted 31 lines · Cyrill Gorcunov

Quoted 11 lines · Oleg Nesterov
Quoted 3 lines · Cyrill Gorcunov
So Oleg, I think you meant something like below? Comment is moved down an list_empty over siblings remans, right?
Yes, except the comment still looks misleading to me.
Otherwise looks correct, but I'll try to re-check once again with the fresh head. Although I think you should remove me from CC: after I found the nonexistent bug ;)
There is no way back from CC ;)
Quoted 3 lines
This is minor, but "freshly created" looks very confusing to me. What does it mean? We hold tasklist, we can't race with fork().
Hmm. Sure we keep a lock here, but changes might happen between reads If only I'm not missing something again.
Look which scenario I've had in mind. We have a task A and children B,C,D,E. ... Here my scenario ended and I realised that you're right. I'll update the comment.
Quoted 5 lines
Yes we can miss a child, but this has nothing to do with "freshly". Just suppose that the parent sleeps, but N children exit after we printed their tids. Now the slow paths skips N extra children, we miss N tasks.

Quoted 4 lines

Quoted 2 lines
Oleg.
Cyrill

I suppose it might be something like below. I've updated comment and quoted your comment there just I wont forget this next time I'll be reading the source. Thanks!

Cyrill
---
From: Cyrill Gorcunov <[email protected]>
Subject: fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v6

This is a first level children, not the whole process tree.

v3:
- To be on a safe side use %lu format for pid_t printing

v4:
- New line get printed when sequence ends not at seq->stop, a nit pointed by Tejun
- Documentation update
- tasklist_lock is back, Oleg pointed that ->children list is actually not rcu-safe

v6:
- Drop ptrace_may_access tests, pids are can be found anyway so nothing to protect here.
- Update comments and docs, pointed by Oleg.

Signed-off-by Cyrill Gorcunov <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Oleg Nesterov <[email protected]>

---
 Documentation/filesystems/proc.txt |   18 ++++
 fs/proc/array.c                    |  155 +++++++++++++++++++++++++++++++++++++
 fs/proc/base.c                     |    1 
 fs/proc/internal.h                 |    1 
 4 files changed, 175 insertions(+)

Documentation/filesystems/proc.txt +18 -0

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
@@ -40,6 +40,7 @@ Table of Contents
   3.4	/proc/<pid>/coredump_filter - Core dump filtering settings
   3.5	/proc/<pid>/mountinfo - Information about mounts
   3.6	/proc/<pid>/comm  & /proc/<pid>/task/<tid>/comm
+  3.7   /proc/<pid>/task/<tid>/children - Information about task children
 
   4	Configuring procfs
   4.1	Mount options

@@ -1549,6 +1550,23 @@ then the kernel's TASK_COMM_LEN (current
 comm value.
 
 
+3.7	/proc/<pid>/task/<tid>/children - Information about task children
+-------------------------------------------------------------------------
+This file provides a fast way to retrieve first level children pids
+of a task pointed by <pid>/<tid> pair. The format is a space separated
+stream of pids.
+
+Note the "first level" here -- if a child has own children they will
+not be listed here, one needs to read /proc/<children-pid>/task/<tid>/children
+to obtain the descendants.
+
+Since this interface is intended to be fast and cheap it doesn't
+guarantee to provide precise results and some children might be
+skipped, especially if they've exited right after we printed their
+pids, so one need to either stop or freeze processes being inspected
+if precise results are needed.
+
+
 ------------------------------------------------------------------------------
 Configuring procfs
 ------------------------------------------------------------------------------

fs/proc/array.c +155 -0

158 lines not on this page · raw message

fs/proc/base.c +1 -0

Index: linux-2.6.git/fs/proc/base.c
===================================================================
@@ -3454,6 +3454,7 @@ static const struct pid_entry tid_base_s
 	ONE("stat",      S_IRUGO, proc_tid_stat),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
 	REG("maps",      S_IRUGO, proc_maps_operations),
+	REG("children",  S_IRUGO, proc_tid_children_operations),
 #ifdef CONFIG_NUMA
 	REG("numa_maps", S_IRUGO, proc_numa_maps_operations),
 #endif

fs/proc/internal.h +1 -0

Index: linux-2.6.git/fs/proc/internal.h
===================================================================
@@ -53,6 +53,7 @@ extern int proc_pid_statm(struct seq_fil
 				struct pid *pid, struct task_struct *task);
 extern loff_t mem_lseek(struct file *file, loff_t offset, int orig);
 
+extern const struct file_operations proc_tid_children_operations;
 extern const struct file_operations proc_maps_operations;
 extern const struct file_operations proc_numa_maps_operations;
 extern const struct file_operations proc_smaps_operations;

Oleg Nesterov

2012-01-19 14:17 UTC #14 lore raw

Quoted 4 lines · Cyrill Gorcunov

I suppose it might be something like below. I've updated comment and quoted your comment there just I wont forget this next time I'll be reading the source. Thanks!

I believe the patrch is correct.

But... Cyrill, I am wondering how much will you hate me if I make yet another attempt to delay this patch.

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

Quoted 11 lines

fs/proc/array.c +11 -0

+static int children_seq_open(struct inode *inode, struct file *file)
+{
+	struct proc_pid_children_iter *iter = NULL;
+	struct task_struct *task = NULL;
+	int ret = 0;
+
+	task = get_proc_task(inode);
+	if (!task) {
+		ret = -ENOENT;
+		goto err;
+	}

For what??

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

Quoted 5 lines

fs/proc/array.c +5 -0

+	if (!ret) {
+		struct seq_file *m = file->private_data;
+		m->private = iter;
+
+		iter->pid_start = get_pid(task_pid(task));

This is what we need, right? So can't we remove this "task_struct *task" and simply do

		iter->pid_start = get_ppid(proc_pid(inode));

And while this is absolutely cosmetic probably ->parent_pid is a bit better name, but this is up to you.

Oleg.

Cyrill Gorcunov

2012-01-19 14:47 UTC #15 lore raw

Quoted 11 lines · Oleg Nesterov

Quoted 4 lines · Cyrill Gorcunov
I suppose it might be something like below. I've updated comment and quoted your comment there just I wont forget this next time I'll be reading the source. Thanks!
I believe the patrch is correct.
But... Cyrill, I am wondering how much will you hate me if I make yet another attempt to delay this patch.

I'm ready to do any iterations until patch become correct. It's better to spend some time now than later.

Quoted 23 lines

fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 661
Quoted 11 lines
fs/proc/array.c +11 -0
+static int children_seq_open(struct inode *inode, struct file *file)
+{
+	struct proc_pid_children_iter *iter = NULL;
+	struct task_struct *task = NULL;
+	int ret = 0;
+
+	task = get_proc_task(inode);
+	if (!task) {
+		ret = -ENOENT;
+		goto err;
+	}
For what??
fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 674
Quoted 5 lines
fs/proc/array.c +5 -0
+	if (!ret) {
+		struct seq_file *m = file->private_data;
+		m->private = iter;
+
+		iter->pid_start = get_pid(task_pid(task));
This is what we need, right? So can't we remove this "task_struct *task" and simply do

Quoted 2 lines

		iter->pid_start = get_ppid(proc_pid(inode));

Yeah, this one will be cleaner. Thanks!

Quoted 5 lines

?
And while this is absolutely cosmetic probably ->parent_pid is a bit better name, but this is up to you.

Yeah, I'll change the name, I liked this one more.

Cyrill

Oleg Nesterov

2012-01-19 16:01 UTC #16 lore raw

In reply to Oleg Nesterov

Quoted 11 lines · Oleg Nesterov

Quoted 4 lines · Cyrill Gorcunov
I suppose it might be something like below. I've updated comment and quoted your comment there just I wont forget this next time I'll be reading the source. Thanks!
I believe the patrch is correct.
But... Cyrill, I am wondering how much will you hate me if I make yet another attempt to delay this patch.

Cough... and another attempt...

Quoted 23 lines

fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 661
Quoted 11 lines
fs/proc/array.c +11 -0
+static int children_seq_open(struct inode *inode, struct file *file)
+{
+	struct proc_pid_children_iter *iter = NULL;
+	struct task_struct *task = NULL;
+	int ret = 0;
+
+	task = get_proc_task(inode);
+	if (!task) {
+		ret = -ENOENT;
+		goto err;
+	}
For what??
fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 674
Quoted 5 lines
fs/proc/array.c +5 -0
+	if (!ret) {
+		struct seq_file *m = file->private_data;
+		m->private = iter;
+
+		iter->pid_start = get_pid(task_pid(task));
This is what we need, right? So can't we remove this "task_struct *task" and simply do

Quoted 6 lines

		iter->pid_start = get_ppid(proc_pid(inode));
?
And while this is absolutely cosmetic probably ->parent_pid is a bit better name, but this is up to you.

Thinking more... I am not sure, but do we really need proc_pid_children_iter at all??

It is very possibly I missed something, but we can get both parent_pid and pid_ns from inode, right? so can't we just remember inode in seq->private?

Oleg.

Cyrill Gorcunov

2012-01-19 16:27 UTC #17 lore raw

Quoted 8 lines · Oleg Nesterov

Thinking more... I am not sure, but do we really need proc_pid_children_iter at all??
It is very possibly I missed something, but we can get both parent_pid and pid_ns from inode, right? so can't we just remember inode in seq->private?

Good point. Letme check if we will need to call for ihold then... (/me scratchig the head)

Cyrill

Oleg Nesterov

2012-01-19 16:50 UTC #18 lore raw

Quoted 6 lines · Cyrill Gorcunov

Quoted 4 lines · Oleg Nesterov
Thinking more... I am not sure, but do we really need proc_pid_children_iter at all??

Quoted 6 lines

Quoted 3 lines
It is very possibly I missed something, but we can get both parent_pid and pid_ns from inode, right? so can't we just remember inode in seq->private?
Good point. Letme check if we will need to call for ihold then... (/me scratchig the head)

Why? file/inode can't go away, at least until this fd is closed.

And just in case, get_pid(proc_pid(inode)) is not needed, even in v6/v7. I didn't realize this.

Cyrill, I won't argue if you prefer to make this in a separate patch (of course, assuming you are agree) on top of v7, to me it looks "good enough".

Oleg.

Cyrill Gorcunov

2012-01-19 17:08 UTC #19 lore raw

Quoted 11 lines · Oleg Nesterov

Quoted 10 lines · Cyrill Gorcunov
Quoted 7 lines · Oleg Nesterov
Thinking more... I am not sure, but do we really need proc_pid_children_iter at all??
It is very possibly I missed something, but we can get both parent_pid and pid_ns from inode, right? so can't we just remember inode in seq->private?

Quoted 8 lines

Quoted 2 lines
Good point. Letme check if we will need to call for ihold then... (/me scratchig the head)
Why? file/inode can't go away, at least until this fd is closed.
And just in case, get_pid(proc_pid(inode)) is not needed, even in v6/v7. I didn't realize this.

Grr, indeed.

Quoted 4 lines

Cyrill, I won't argue if you prefer to make this in a separate patch (of course, assuming you are agree) on top of v7, to me it looks "good enough".

I think better to make completely updated version then, than pushing changes on top. it'll be better to put anything in one patch I think.

Thanks!

Cyrill