[PATCH] proc: Expose /proc/<pid>/task/<tid>/children unconditionally

[PATCH] proc: Expose /proc/<pid>/task/<tid>/children unconditionally

[Andy Lutomirski]

This is currently only available if CONFIG_CHECKPOINT_RESTORE, which
is hidden under CONFIG_EXPERT.  It's generally useful functionality,
though, so expose it unconditionally.

Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 fs/proc/array.c | 2 --
 fs/proc/base.c  | 2 --
 2 files changed, 4 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index cbd0f1b..96d3e3e 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -598,7 +598,6 @@ int proc_pid_statm(struct seq_file *m, struct pid_namespace *ns,
 	return 0;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
 static struct pid *
 get_children_pid(struct inode *inode, struct pid *pid_prev, loff_t pos)
 {
@@ -719,4 +718,3 @@ const struct file_operations proc_tid_children_operations = {
 	.llseek  = seq_lseek,
 	.release = children_seq_release,
 };
-#endif /* CONFIG_CHECKPOINT_RESTORE */
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 69078c7..34396a9 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2866,9 +2866,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	ONE("stat",      S_IRUGO, proc_tid_stat),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
 	REG("maps",      S_IRUGO, proc_tid_maps_operations),
-#ifdef CONFIG_CHECKPOINT_RESTORE
 	REG("children",  S_IRUGO, proc_tid_children_operations),
-#endif
 #ifdef CONFIG_NUMA
 	REG("numa_maps", S_IRUGO, proc_tid_numa_maps_operations),
 #endif
-- 
1.8.1.4

Re: [PATCH] proc: Expose /proc/<pid>/task/<tid>/children unconditionally

[Cyrill Gorcunov]

On Tue, Jun 25, 2013 at 12:51:45PM -0700, Andy Lutomirski wrote:
> This is currently only available if CONFIG_CHECKPOINT_RESTORE, which
> is hidden under CONFIG_EXPERT.  It's generally useful functionality,
> though, so expose it unconditionally.
> 
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>

Re: [PATCH] proc: Expose /proc/<pid>/task/<tid>/children unconditionally

[Oleg Nesterov]

On 06/26, Cyrill Gorcunov wrote:
>
> On Tue, Jun 25, 2013 at 12:51:45PM -0700, Andy Lutomirski wrote:
> > This is currently only available if CONFIG_CHECKPOINT_RESTORE, which
> > is hidden under CONFIG_EXPERT.  It's generally useful functionality,
> > though, so expose it unconditionally.
> >
> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>

I didn't see the patch but I guess it is trivial and I agree with intent ;)

Oleg.

Re: [PATCH] proc: Expose /proc/<pid>/task/<tid>/children unconditionally

[Andy Lutomirski]

On Tue, Jun 25, 2013 at 1:17 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/26, Cyrill Gorcunov wrote:
>>
>> On Tue, Jun 25, 2013 at 12:51:45PM -0700, Andy Lutomirski wrote:
>> > This is currently only available if CONFIG_CHECKPOINT_RESTORE, which
>> > is hidden under CONFIG_EXPERT.  It's generally useful functionality,
>> > though, so expose it unconditionally.
>> >
>> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
>> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
>
> I didn't see the patch but I guess it is trivial and I agree with intent ;)

The patch works, but "children" is only listed under task/<tid>, not
under /proc/<pid>.  Is that intentional?  Fixing it would be a
one-liner.

--Andy

Re: [PATCH] proc: Expose /proc/<pid>/task/<tid>/children unconditionally

[Cyrill Gorcunov]

On Tue, Jun 25, 2013 at 02:36:31PM -0700, Andy Lutomirski wrote:
> On Tue, Jun 25, 2013 at 1:17 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 06/26, Cyrill Gorcunov wrote:
> >>
> >> On Tue, Jun 25, 2013 at 12:51:45PM -0700, Andy Lutomirski wrote:
> >> > This is currently only available if CONFIG_CHECKPOINT_RESTORE, which
> >> > is hidden under CONFIG_EXPERT.  It's generally useful functionality,
> >> > though, so expose it unconditionally.
> >> >
> >> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> >> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> >> Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
> >
> > I didn't see the patch but I guess it is trivial and I agree with intent ;)
> 
> The patch works, but "children" is only listed under task/<tid>, not
> under /proc/<pid>.  Is that intentional?  Fixing it would be a
> one-liner.

Yeah, it's intentional. Here some explanations from Oleg (check out
the whole thread, it's not that big https://lkml.org/lkml/2011/12/9/220)
in short this might require some more code, but i'll re-check tomorrow.

Re: [PATCH] proc: Expose /proc/<pid>/task/<tid>/children unconditionally

[Andy Lutomirski]

On Tue, Jun 25, 2013 at 2:52 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Tue, Jun 25, 2013 at 02:36:31PM -0700, Andy Lutomirski wrote:
>> On Tue, Jun 25, 2013 at 1:17 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > On 06/26, Cyrill Gorcunov wrote:
>> >>
>> >> On Tue, Jun 25, 2013 at 12:51:45PM -0700, Andy Lutomirski wrote:
>> >> > This is currently only available if CONFIG_CHECKPOINT_RESTORE, which
>> >> > is hidden under CONFIG_EXPERT.  It's generally useful functionality,
>> >> > though, so expose it unconditionally.
>> >> >
>> >> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
>> >> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> >> Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
>> >
>> > I didn't see the patch but I guess it is trivial and I agree with intent ;)
>>
>> The patch works, but "children" is only listed under task/<tid>, not
>> under /proc/<pid>.  Is that intentional?  Fixing it would be a
>> one-liner.
>
> Yeah, it's intentional. Here some explanations from Oleg (check out
> the whole thread, it's not that big https://lkml.org/lkml/2011/12/9/220)
> in short this might require some more code, but i'll re-check tomorrow.

This is a little strange.  It looks like ppid (in status) shows the
tgid, but the actual real_parent can refer to a thread (as opposed to
a thread group leader), and task/tid/children respects that.  So the
tree that you get by following task/tid/ children won't be quite the
same as the tree you get by following ppid.

I wonder if the ptid should be added to status.  Is there anything
(other than task/tid/children) that cased which thread is the parent
of a given task?

--Andy

Re: [PATCH] proc: Expose /proc/<pid>/task/<tid>/children unconditionally

[Cyrill Gorcunov]

On Tue, Jun 25, 2013 at 03:01:35PM -0700, Andy Lutomirski wrote:
> 
> This is a little strange.  It looks like ppid (in status) shows the
> tgid, but the actual real_parent can refer to a thread (as opposed to
> a thread group leader), and task/tid/children respects that.  So the
> tree that you get by following task/tid/ children won't be quite the
> same as the tree you get by following ppid.
> 
> I wonder if the ptid should be added to status.  Is there anything
> (other than task/tid/children) that cased which thread is the parent
> of a given task?

None I know of. As to ptid -- sounds reasonable to me. Oleg?

Re: [PATCH] proc: Expose /proc/<pid>/task/<tid>/children unconditionally

[Oleg Nesterov]

On 06/25, Andy Lutomirski wrote:
>
> On Tue, Jun 25, 2013 at 2:52 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > On Tue, Jun 25, 2013 at 02:36:31PM -0700, Andy Lutomirski wrote:
> >> On Tue, Jun 25, 2013 at 1:17 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >> > On 06/26, Cyrill Gorcunov wrote:
> >> >>
> >> >> On Tue, Jun 25, 2013 at 12:51:45PM -0700, Andy Lutomirski wrote:
> >> >> > This is currently only available if CONFIG_CHECKPOINT_RESTORE, which
> >> >> > is hidden under CONFIG_EXPERT.  It's generally useful functionality,
> >> >> > though, so expose it unconditionally.
> >> >> >
> >> >> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> >> >> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> >> >> Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
> >> >
> >> > I didn't see the patch but I guess it is trivial and I agree with intent ;)
> >>
> >> The patch works, but "children" is only listed under task/<tid>, not
> >> under /proc/<pid>.  Is that intentional?  Fixing it would be a
> >> one-liner.
> >
> > Yeah, it's intentional. Here some explanations from Oleg (check out
> > the whole thread, it's not that big https://lkml.org/lkml/2011/12/9/220)
> > in short this might require some more code, but i'll re-check tomorrow.
>
> This is a little strange.  It looks like ppid (in status) shows the
> tgid,

Yes,

> but the actual real_parent can refer to a thread (as opposed to
> a thread group leader),

Yes.

This is mostly the internal implementation detail. And probably it
would be nice to move ->children from task_struct to signal_struct.

However. See __WNOTHREAD in man waitpid. I think this is the only
(historical) reason. Otherwise the real_parent's tid doesn't matter,
the parent is always the whole process, not sub-thread.

> and task/tid/children respects that.

Well, it should respect if you want to restart and keep __WNOTHREAD
working.

But there is another reason. It is not trivial to list all children
under /proc/<pid>, and the fix would not be a one-liner ;) You need
to fight with the exiting sub-threads and reparenting. 

> So the
> tree that you get by following task/tid/ children won't be quite the
> same as the tree you get by following ppid.

Not sure I understand... but in any case, yes you need to read
/proc/pid/task/*/children to construct the tree.

> I wonder if the ptid should be added to status.  Is there anything
> (other than task/tid/children)

Perhaps, I dunno.

Better yet, we should kill __WNOTHREAD ;) I am wondering if it is still
used.

Oleg.

Re: [PATCH] proc: Expose /proc/<pid>/task/<tid>/children unconditionally

[Andy Lutomirski]

On Wed, Jun 26, 2013 at 8:57 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/25, Andy Lutomirski wrote:
>>
>> On Tue, Jun 25, 2013 at 2:52 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> > On Tue, Jun 25, 2013 at 02:36:31PM -0700, Andy Lutomirski wrote:
>> >> On Tue, Jun 25, 2013 at 1:17 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >> > On 06/26, Cyrill Gorcunov wrote:
>> >> >>
>> >> >> On Tue, Jun 25, 2013 at 12:51:45PM -0700, Andy Lutomirski wrote:
>> >> >> > This is currently only available if CONFIG_CHECKPOINT_RESTORE, which
>> >> >> > is hidden under CONFIG_EXPERT.  It's generally useful functionality,
>> >> >> > though, so expose it unconditionally.
>> >> >> >
>> >> >> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
>> >> >> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> >> >> Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
>> >> >
>> >> > I didn't see the patch but I guess it is trivial and I agree with intent ;)
>> >>
>> >> The patch works, but "children" is only listed under task/<tid>, not
>> >> under /proc/<pid>.  Is that intentional?  Fixing it would be a
>> >> one-liner.
>> >
>> > Yeah, it's intentional. Here some explanations from Oleg (check out
>> > the whole thread, it's not that big https://lkml.org/lkml/2011/12/9/220)
>> > in short this might require some more code, but i'll re-check tomorrow.
>>
>> This is a little strange.  It looks like ppid (in status) shows the
>> tgid,
>
> Yes,
>
>> but the actual real_parent can refer to a thread (as opposed to
>> a thread group leader),
>
> Yes.
>
> This is mostly the internal implementation detail. And probably it
> would be nice to move ->children from task_struct to signal_struct.
>
> However. See __WNOTHREAD in man waitpid. I think this is the only
> (historical) reason. Otherwise the real_parent's tid doesn't matter,
> the parent is always the whole process, not sub-thread.
>
>> and task/tid/children respects that.
>
> Well, it should respect if you want to restart and keep __WNOTHREAD
> working.
>
> But there is another reason. It is not trivial to list all children
> under /proc/<pid>, and the fix would not be a one-liner ;) You need
> to fight with the exiting sub-threads and reparenting.
>
>> So the
>> tree that you get by following task/tid/ children won't be quite the
>> same as the tree you get by following ppid.
>
> Not sure I understand... but in any case, yes you need to read
> /proc/pid/task/*/children to construct the tree.
>
>> I wonder if the ptid should be added to status.  Is there anything
>> (other than task/tid/children)
>
> Perhaps, I dunno.
>
> Better yet, we should kill __WNOTHREAD ;) I am wondering if it is still
> used.

Ugh.  I'll submit a follow-up patch to
Documentation/filesystems/proc.txt to explain the situation.  All I
really care about is being able to iterate the children of a
single-threaded process, and the current semantics are fine for that.

--Andy

[PATCH] proc: Document that /proc/<pid>/task/<tid>/children really is per-thread

[Andy Lutomirski]

I was surprised to discover that a process can have a parent that isn't
a thread group leader.  (The usual ppid interfaces hide this, but the
children list exposes it.)

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 Documentation/filesystems/proc.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index fd8d0d5..205796a 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -1623,6 +1623,12 @@ 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.
 
+This really is a per-thread list.  If a process's parent is a thread,
+then that process will appear in that thread's children list.  (This
+means that, for any pid, /proc/pid/task/*/children are disjoint lists.)
+This may be surprising, as /proc/pid/status's PPid field is parent's
+tgid as opposed to the parent's tid.
+
 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.
-- 
1.8.1.4

Re: [PATCH] proc: Document that /proc/<pid>/task/<tid>/children really is per-thread

[Cyrill Gorcunov]

On Wed, Jun 26, 2013 at 02:05:01PM -0700, Andy Lutomirski wrote:
> I was surprised to discover that a process can have a parent that isn't
> a thread group leader.  (The usual ppid interfaces hide this, but the
> children list exposes it.)
> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>

Looks good to me, thanks!

Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>

Re: [PATCH] proc: Document that /proc/<pid>/task/<tid>/children really is per-thread

[Rob Landley]

On 06/26/2013 04:05:01 PM, Andy Lutomirski wrote:
> I was surprised to discover that a process can have a parent that  
> isn't
> a thread group leader.  (The usual ppid interfaces hide this, but the
> children list exposes it.)
> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> Cc: Cyrill Gorcunov <gorcunov@gmail.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> ---
>  Documentation/filesystems/proc.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/filesystems/proc.txt  
> b/Documentation/filesystems/proc.txt
> index fd8d0d5..205796a 100644
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -1623,6 +1623,12 @@ 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.
> 
> +This really is a per-thread list.  If a process's parent is a thread,
> +then that process will appear in that thread's children list.  (This
> +means that, for any pid, /proc/pid/task/*/children are disjoint  
> lists.)
> +This may be surprising, as /proc/pid/status's PPid field is parent's
> +tgid as opposed to the parent's tid.

I've read this twice and still don't quite understand what it's saying.  
(Possibly I need more than 3 hours of sleep.)

It sounds like you're saying a thread can fork() and exec() and this  
makes proc look weird, because the ppid pooints to the thread group  
leader and not the thread, but the proc info listing us as a child  
belongs to the thread?

Is this a bug that should be fixed rather than documented? (Showing the  
right ppid for this corner case?)

Rob

Re: [PATCH] proc: Document that /proc/<pid>/task/<tid>/children really is per-thread

[Andy Lutomirski]

On Mon, Jul 1, 2013 at 9:49 AM, Rob Landley <rob@landley.net> wrote:
> On 06/26/2013 04:05:01 PM, Andy Lutomirski wrote:
>>
>> I was surprised to discover that a process can have a parent that isn't
>> a thread group leader.  (The usual ppid interfaces hide this, but the
>> children list exposes it.)
>>
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> Cc: Cyrill Gorcunov <gorcunov@gmail.com>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> ---
>>  Documentation/filesystems/proc.txt | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/filesystems/proc.txt
>> b/Documentation/filesystems/proc.txt
>> index fd8d0d5..205796a 100644
>> --- a/Documentation/filesystems/proc.txt
>> +++ b/Documentation/filesystems/proc.txt
>> @@ -1623,6 +1623,12 @@ 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.
>>
>> +This really is a per-thread list.  If a process's parent is a thread,
>> +then that process will appear in that thread's children list.  (This
>> +means that, for any pid, /proc/pid/task/*/children are disjoint lists.)
>> +This may be surprising, as /proc/pid/status's PPid field is parent's
>> +tgid as opposed to the parent's tid.
>
>
> I've read this twice and still don't quite understand what it's saying.
> (Possibly I need more than 3 hours of sleep.)
>
> It sounds like you're saying a thread can fork() and exec() and this makes
> proc look weird, because the ppid pooints to the thread group leader and not
> the thread, but the proc info listing us as a child belongs to the thread?

That is exactly right.  Is it unclear because I wrote it badly, or is
it unclear because it's really weird?

Perhaps I'll add something like:

For example, if thread group 100 has threads 100 (the leader) and 101,
and thread 101 forks and produces a child with tgid 102, then
/proc/100/task/100/children will be empty, /proc/100/task/101/children
will contain 102, and /proc/102/status will show 'Ppid: 100'.

>
> Is this a bug that should be fixed rather than documented? (Showing the
> right ppid for this corner case?)
>

I'm not qualified to say whether it's a bug or even, if it is a bug,
what the preferred behavior would be.

--Andy