[PATCH] [RFC] fs, proc: don't guard /proc/<pid>/task/<tid>/children on CONFIG_CHECKPOINT_RESTORE

[PATCH] [RFC] fs, proc: don't guard /proc/<pid>/task/<tid>/children on CONFIG_CHECKPOINT_RESTORE

[Alban Crequy]

From: Alban Crequy <alban-973cpzSjLbNWk0Htik3J/w@public.gmane.org>

commit 818411616baf ("fs, proc: introduce
/proc/<pid>/task/<tid>/children entry") introduced the children entry
for checkpoint restore and the file is only available on kernels
configured with CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.

This is available in most distributions (Fedora, Debian, Ubuntu, CoreOS)
because they usually enable CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.
But Arch does not enable CONFIG_EXPERT or CONFIG_CHECKPOINT_RESTORE.

However, the children proc file is useful outside of checkpoint restore.
I would like to use it in rkt. The rkt process exec() another program it
does not control, and that other program will fork()+exec() a child
process. I would like to find the pid of the child process from an
external tool without iterating in /proc over all processes to find
which one has a parent pid equal to rkt.

Since the children proc file is useful outside of checkpoint-restore,
I am removing the guard on CONFIG_CHECKPOINT_RESTORE.

Signed-off-by: Alban Crequy <alban-973cpzSjLbNWk0Htik3J/w@public.gmane.org>
Cc: Iago Lopez Galeiras <iago-973cpzSjLbNWk0Htik3J/w@public.gmane.org>
---
 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 fd02a9e..6edec57 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -569,7 +569,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)
 {
@@ -692,4 +691,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 093ca14..b743007 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2922,9 +2922,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
-- 
2.1.4

Re: [PATCH] [RFC] fs, proc: don't guard /proc/<pid>/task/<tid>/children on CONFIG_CHECKPOINT_RESTORE

[Andrew Morton]

On Thu, 21 May 2015 12:30:21 +0200 Alban Crequy <alban.crequy@gmail.com> wrote:

> commit 818411616baf ("fs, proc: introduce
> /proc/<pid>/task/<tid>/children entry") introduced the children entry
> for checkpoint restore and the file is only available on kernels
> configured with CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.
> 
> This is available in most distributions (Fedora, Debian, Ubuntu, CoreOS)
> because they usually enable CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.
> But Arch does not enable CONFIG_EXPERT or CONFIG_CHECKPOINT_RESTORE.
> 
> However, the children proc file is useful outside of checkpoint restore.
> I would like to use it in rkt. The rkt process exec() another program it
> does not control, and that other program will fork()+exec() a child
> process. I would like to find the pid of the child process from an
> external tool without iterating in /proc over all processes to find
> which one has a parent pid equal to rkt.
> 
> Since the children proc file is useful outside of checkpoint-restore,
> I am removing the guard on CONFIG_CHECKPOINT_RESTORE.

This will add a lump of code to kernels which don't need it.

It's a bit of a pain, but I suppose we should still keep the presence
of get_children_pid() configurable.  That would be by adding a new
CONFIG_PROC_CHILDREN (or similar) and making CONFIG_CHECKPOINT_RESTORE
select that.

[PATCH] fs, proc: introduce CONFIG_PROC_CHILDREN

[Iago López Galeiras]

commit 818411616baf ("fs, proc: introduce
/proc/<pid>/task/<tid>/children entry") introduced the children entry
for checkpoint restore and the file is only available on kernels
configured with CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.

This is available in most distributions (Fedora, Debian, Ubuntu, CoreOS)
because they usually enable CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.
But Arch does not enable CONFIG_EXPERT or CONFIG_CHECKPOINT_RESTORE.

However, the children proc file is useful outside of checkpoint restore.
I would like to use it in rkt. The rkt process exec() another program it
does not control, and that other program will fork()+exec() a child
process. I would like to find the pid of the child process from an
external tool without iterating in /proc over all processes to find
which one has a parent pid equal to rkt.

This commit introduces CONFIG_PROC_CHILDREN and makes
CONFIG_CHECKPOINT_RESTORE depend on it. This allows enabling
/proc/<pid>/task/<tid>/children without needing to enable
CONFIG_CHECKPOINT_RESTORE and CONFIG_EXPERT.

Alban tested that /proc/<pid>/task/<tid>/children is present when the
kernel is configured with CONFIG_PROC_CHILDREN=y but without
CONFIG_CHECKPOINT_RESTORE

Signed-off-by: Iago López Galeiras <iago@endocode.com>
Cc: Alban Crequy <alban@endocode.com>
Tested-by: Alban Crequy <alban@endocode.com>
---
 fs/proc/Kconfig | 4 ++++
 fs/proc/array.c | 4 ++--
 fs/proc/base.c  | 2 +-
 init/Kconfig    | 1 +
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig
index 2183fcf..d751fcb 100644
--- a/fs/proc/Kconfig
+++ b/fs/proc/Kconfig
@@ -71,3 +71,7 @@ config PROC_PAGE_MONITOR
 	  /proc/pid/smaps, /proc/pid/clear_refs, /proc/pid/pagemap,
 	  /proc/kpagecount, and /proc/kpageflags. Disabling these
           interfaces will reduce the size of the kernel by approximately 4kb.
+
+config PROC_CHILDREN
+	bool "Include /proc/<pid>/task/<tid>/children file"
+	default n
diff --git a/fs/proc/array.c b/fs/proc/array.c
index fd02a9e..ddc4b2c 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -569,7 +569,7 @@ int proc_pid_statm(struct seq_file *m, struct pid_namespace *ns,
 	return 0;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
+#ifdef CONFIG_PROC_CHILDREN
 static struct pid *
 get_children_pid(struct inode *inode, struct pid *pid_prev, loff_t pos)
 {
@@ -692,4 +692,4 @@ const struct file_operations proc_tid_children_operations = {
 	.llseek  = seq_lseek,
 	.release = children_seq_release,
 };
-#endif /* CONFIG_CHECKPOINT_RESTORE */
+#endif /* CONFIG_PROC_CHILDREN */
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 093ca14..5477a4a 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2922,7 +2922,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
+#ifdef CONFIG_PROC_CHILDREN
 	REG("children",  S_IRUGO, proc_tid_children_operations),
 #endif
 #ifdef CONFIG_NUMA
diff --git a/init/Kconfig b/init/Kconfig
index dc24dec..daa7941 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1145,6 +1145,7 @@ endif # CGROUPS
 
 config CHECKPOINT_RESTORE
 	bool "Checkpoint/restore support" if EXPERT
+	depends on PROC_CHILDREN
 	default n
 	help
 	  Enables additional kernel features in a sake of checkpoint/restore.
-- 
2.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs, proc: introduce CONFIG_PROC_CHILDREN

[Cyrill Gorcunov]

On Fri, May 22, 2015 at 02:47:39PM +0200, Iago López Galeiras wrote:
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1145,6 +1145,7 @@ endif # CGROUPS
>  
>  config CHECKPOINT_RESTORE
>  	bool "Checkpoint/restore support" if EXPERT
> +	depends on PROC_CHILDREN
>  	default n
>  	help
>  	  Enables additional kernel features in a sake of checkpoint/restore.

Maybe better select PROC_CHILDREN here? Also I must admit I don't understand
why similar patch from Andy has not been merged earlier? Andy, there was
some problem I forgot about?

(to be clear, I don't mind enabling it since we use it in criu alot ;)

Re: [PATCH] fs, proc: introduce CONFIG_PROC_CHILDREN

[Djalal Harouni]

On Fri, May 22, 2015 at 06:37:05PM +0300, Cyrill Gorcunov wrote:
> On Fri, May 22, 2015 at 02:47:39PM +0200, Iago López Galeiras wrote:
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -1145,6 +1145,7 @@ endif # CGROUPS
> >  
> >  config CHECKPOINT_RESTORE
> >  	bool "Checkpoint/restore support" if EXPERT
> > +	depends on PROC_CHILDREN
> >  	default n
> >  	help
> >  	  Enables additional kernel features in a sake of checkpoint/restore.
> 
> Maybe better select PROC_CHILDREN here? Also I must admit I don't understand
> why similar patch from Andy has not been merged earlier? Andy, there was
> some problem I forgot about?
From the linked thread, it seems that it was just lost... The discussion
ended up about task/<tid> and /proc/<pid>... That patch should have been
applied.

BTW this patch adds the CONFIG guard that was requested by Andrew. There
is a v3 coming to make it "select" as suggested.

Thanks!

-- 
Djalal Harouni
http://opendz.org
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] fs, proc: introduce CONFIG_PROC_CHILDREN

[Iago López Galeiras]

commit 818411616baf ("fs, proc: introduce
/proc/<pid>/task/<tid>/children entry") introduced the children entry
for checkpoint restore and the file is only available on kernels
configured with CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.

This is available in most distributions (Fedora, Debian, Ubuntu, CoreOS)
because they usually enable CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.
But Arch does not enable CONFIG_EXPERT or CONFIG_CHECKPOINT_RESTORE.

However, the children proc file is useful outside of checkpoint restore.
I would like to use it in rkt. The rkt process exec() another program it
does not control, and that other program will fork()+exec() a child
process. I would like to find the pid of the child process from an
external tool without iterating in /proc over all processes to find
which one has a parent pid equal to rkt.

This commit introduces CONFIG_PROC_CHILDREN and makes
CONFIG_CHECKPOINT_RESTORE select it. This allows enabling
/proc/<pid>/task/<tid>/children without needing to enable
CONFIG_CHECKPOINT_RESTORE and CONFIG_EXPERT.

Alban tested that /proc/<pid>/task/<tid>/children is present when the
kernel is configured with CONFIG_PROC_CHILDREN=y but without
CONFIG_CHECKPOINT_RESTORE

v3: change depend -> select
v2: introduce CONFIG_PROC_CHILDREN http://marc.info/?l=linux-fsdevel&m=143229890217280&w=2
v1: http://marc.info/?l=linux-api&m=143220431121869&w=2

Signed-off-by: Iago López Galeiras <iago@endocode.com>
Cc: Alban Crequy <alban@endocode.com>
Tested-by: Alban Crequy <alban@endocode.com>
---
 fs/proc/Kconfig | 4 ++++
 fs/proc/array.c | 4 ++--
 fs/proc/base.c  | 2 +-
 init/Kconfig    | 1 +
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig
index 2183fcf..d751fcb 100644
--- a/fs/proc/Kconfig
+++ b/fs/proc/Kconfig
@@ -71,3 +71,7 @@ config PROC_PAGE_MONITOR
 	  /proc/pid/smaps, /proc/pid/clear_refs, /proc/pid/pagemap,
 	  /proc/kpagecount, and /proc/kpageflags. Disabling these
           interfaces will reduce the size of the kernel by approximately 4kb.
+
+config PROC_CHILDREN
+	bool "Include /proc/<pid>/task/<tid>/children file"
+	default n
diff --git a/fs/proc/array.c b/fs/proc/array.c
index fd02a9e..ddc4b2c 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -569,7 +569,7 @@ int proc_pid_statm(struct seq_file *m, struct pid_namespace *ns,
 	return 0;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
+#ifdef CONFIG_PROC_CHILDREN
 static struct pid *
 get_children_pid(struct inode *inode, struct pid *pid_prev, loff_t pos)
 {
@@ -692,4 +692,4 @@ const struct file_operations proc_tid_children_operations = {
 	.llseek  = seq_lseek,
 	.release = children_seq_release,
 };
-#endif /* CONFIG_CHECKPOINT_RESTORE */
+#endif /* CONFIG_PROC_CHILDREN */
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 093ca14..5477a4a 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2922,7 +2922,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
+#ifdef CONFIG_PROC_CHILDREN
 	REG("children",  S_IRUGO, proc_tid_children_operations),
 #endif
 #ifdef CONFIG_NUMA
diff --git a/init/Kconfig b/init/Kconfig
index dc24dec..fa09e5e 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1145,6 +1145,7 @@ endif # CGROUPS
 
 config CHECKPOINT_RESTORE
 	bool "Checkpoint/restore support" if EXPERT
+	select PROC_CHILDREN
 	default n
 	help
 	  Enables additional kernel features in a sake of checkpoint/restore.
-- 
2.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] fs, proc: introduce CONFIG_PROC_CHILDREN

[Cyrill Gorcunov]

On Tue, May 26, 2015 at 05:12:42PM +0200, Iago López Galeiras wrote:
...
> Alban tested that /proc/<pid>/task/<tid>/children is present when the
> kernel is configured with CONFIG_PROC_CHILDREN=y but without
> CONFIG_CHECKPOINT_RESTORE
> 
> v3: change depend -> select
> v2: introduce CONFIG_PROC_CHILDREN http://marc.info/?l=linux-fsdevel&m=143229890217280&w=2
> v1: http://marc.info/?l=linux-api&m=143220431121869&w=2
> 
> Signed-off-by: Iago López Galeiras <iago-973cpzSjLbNWk0Htik3J/w@public.gmane.org>
> Cc: Alban Crequy <alban-973cpzSjLbNWk0Htik3J/w@public.gmane.org>
> Tested-by: Alban Crequy <alban-973cpzSjLbNWk0Htik3J/w@public.gmane.org>

Looks ok to me, thanks.

Reviewed-by: Cyrill Gorcunov <gorcunov-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>

Re: [PATCH] [RFC] fs, proc: don't guard /proc/<pid>/task/<tid>/children on CONFIG_CHECKPOINT_RESTORE

[Andy Lutomirski]

On Thu, May 21, 2015 at 3:30 AM, Alban Crequy <alban.crequy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Alban Crequy <alban-973cpzSjLbNWk0Htik3J/w@public.gmane.org>
>
> commit 818411616baf ("fs, proc: introduce
> /proc/<pid>/task/<tid>/children entry") introduced the children entry
> for checkpoint restore and the file is only available on kernels
> configured with CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.
>
> This is available in most distributions (Fedora, Debian, Ubuntu, CoreOS)
> because they usually enable CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.
> But Arch does not enable CONFIG_EXPERT or CONFIG_CHECKPOINT_RESTORE.
>
> However, the children proc file is useful outside of checkpoint restore.
> I would like to use it in rkt. The rkt process exec() another program it
> does not control, and that other program will fork()+exec() a child
> process. I would like to find the pid of the child process from an
> external tool without iterating in /proc over all processes to find
> which one has a parent pid equal to rkt.
>
> Since the children proc file is useful outside of checkpoint-restore,
> I am removing the guard on CONFIG_CHECKPOINT_RESTORE.

I sent an essentially identical patch a couple years ago, and it got
some interesting comments:

http://lkml.kernel.org/g/0e00e9073855c02a382d49ba1ede9c4fda3451b7.1372189875.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org

See also:

http://lkml.kernel.org/g/5f9a6b3ab75b12f2c5ba61ea1f6f3b08e9952b55.1372280661.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org

--Andy

Re: [PATCH] [RFC] fs, proc: don't guard /proc/<pid>/task/<tid>/children on CONFIG_CHECKPOINT_RESTORE

[Cyrill Gorcunov]

On Thu, May 21, 2015 at 02:47:33PM -0700, Andy Lutomirski wrote:
> >
> > Since the children proc file is useful outside of checkpoint-restore,
> > I am removing the guard on CONFIG_CHECKPOINT_RESTORE.
> 
> I sent an essentially identical patch a couple years ago, and it got
> some interesting comments:
> 

Oh, it is 2k13, I thought it's merged already ;)

> http://lkml.kernel.org/g/0e00e9073855c02a382d49ba1ede9c4fda3451b7.1372189875.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org
> 
> See also:
> 
> http://lkml.kernel.org/g/5f9a6b3ab75b12f2c5ba61ea1f6f3b08e9952b55.1372280661.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org

Re: [PATCH] [RFC] fs, proc: don't guard /proc/<pid>/task/<tid>/children on CONFIG_CHECKPOINT_RESTORE

[Alban Crequy]

On Thu, May 21, 2015 at 11:47 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Thu, May 21, 2015 at 3:30 AM, Alban Crequy <alban.crequy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: Alban Crequy <alban-973cpzSjLbNWk0Htik3J/w@public.gmane.org>
>>
>> commit 818411616baf ("fs, proc: introduce
>> /proc/<pid>/task/<tid>/children entry") introduced the children entry
>> for checkpoint restore and the file is only available on kernels
>> configured with CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.
>>
>> This is available in most distributions (Fedora, Debian, Ubuntu, CoreOS)
>> because they usually enable CONFIG_EXPERT and CONFIG_CHECKPOINT_RESTORE.
>> But Arch does not enable CONFIG_EXPERT or CONFIG_CHECKPOINT_RESTORE.
>>
>> However, the children proc file is useful outside of checkpoint restore.
>> I would like to use it in rkt. The rkt process exec() another program it
>> does not control, and that other program will fork()+exec() a child
>> process. I would like to find the pid of the child process from an
>> external tool without iterating in /proc over all processes to find
>> which one has a parent pid equal to rkt.
>>
>> Since the children proc file is useful outside of checkpoint-restore,
>> I am removing the guard on CONFIG_CHECKPOINT_RESTORE.
>
> I sent an essentially identical patch a couple years ago, and it got
> some interesting comments:
>
> http://lkml.kernel.org/g/0e00e9073855c02a382d49ba1ede9c4fda3451b7.1372189875.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org

Thanks for the pointer, I didn't see it.

> See also:
>
> http://lkml.kernel.org/g/5f9a6b3ab75b12f2c5ba61ea1f6f3b08e9952b55.1372280661.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org

Your paragraph seems good to have in the documentation but I am also
not qualified to comment on the behavior.