config PROC_CHILDREN

config PROC_CHILDREN

[Jean Delvare]

Hi Iago,

You just introduced a Linux kernel configuration option named
PROC_CHILDREN. It is user-visible but has no help. This is bad.

As this option appears to be selected automatically as needed, I'm not
sure why you made it visible?

Please either hide the option, or add a help text to let the user make
a sane decision.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: config PROC_CHILDREN

[Iago López Galeiras]

Hi Jean,

The purpose of this option is enabling /proc/<pid>/task/<tid>/children without
having to enable CHECKPOINT_RESTORE, which is hidden behind EXPERT.

Regarding its lack of help, documentation is in already in place[1] but perhaps
that's not clear for the user because as you say the Kconfig help text is missing.

I suggest adding something like:

    Provides a fast way to retrieve first level children pids of a task. See
    <file:Documentation/filesystems/proc.txt> for more information.

Do you think that's enough?

Thanks.

[1]: https://www.kernel.org/doc/Documentation/filesystems/proc.txt

On 07/03/2015 09:39 AM, Jean Delvare wrote:
> Hi Iago,
> 
> You just introduced a Linux kernel configuration option named
> PROC_CHILDREN. It is user-visible but has no help. This is bad.
> 
> As this option appears to be selected automatically as needed, I'm not
> sure why you made it visible?
> 
> Please either hide the option, or add a help text to let the user make
> a sane decision.
> 
> Thanks,
> 

-- 

Iago López Galeiras
Software developer @ Endocode AG
iago@endocode.com

Re: config PROC_CHILDREN

[Jean Delvare]

Hi Iago,

Please don't top-post.

On Fri, 3 Jul 2015 11:10:45 +0200, Iago López Galeiras wrote:
> Hi Jean,
> 
> The purpose of this option is enabling /proc/<pid>/task/<tid>/children without
> having to enable CHECKPOINT_RESTORE, which is hidden behind EXPERT.
> 
> Regarding its lack of help, documentation is in already in place[1] but perhaps
> that's not clear for the user because as you say the Kconfig help text is missing.
> 
> I suggest adding something like:
> 
>     Provides a fast way to retrieve first level children pids of a task. See
>     <file:Documentation/filesystems/proc.txt> for more information.
> 
> Do you think that's enough?

That's a start, the reference to Documentation/filesystems/proc.txt is
good but I think we can do better. You need to help the user make the
decision. Why should he/she say Y or N? The user should NOT have to look
at an external documentation file if the answer is N. I would suggest
the following:

Say Y if running any user-space software which takes benefit from this
interface. For example, rkt is such a piece of software.

That being said, I am curious... Is this interface so expensive that it
really deserves a separate option, instead of always enabling it? This
seems to be a fairly generic feature that a lot of scripts and tools
could benefit from (starting with pstree I suppose.)

-- 
Jean Delvare
SUSE L3 Support

Re: config PROC_CHILDREN

[Iago López Galeiras]

On 07/04/2015 07:07 PM, Jean Delvare wrote:
> Hi Iago,
> 
> Please don't top-post.
> 
> On Fri, 3 Jul 2015 11:10:45 +0200, Iago López Galeiras wrote:
>> Hi Jean,
>>
>> The purpose of this option is enabling /proc/<pid>/task/<tid>/children without
>> having to enable CHECKPOINT_RESTORE, which is hidden behind EXPERT.
>>
>> Regarding its lack of help, documentation is in already in place[1] but perhaps
>> that's not clear for the user because as you say the Kconfig help text is missing.
>>
>> I suggest adding something like:
>>
>>     Provides a fast way to retrieve first level children pids of a task. See
>>     <file:Documentation/filesystems/proc.txt> for more information.
>>
>> Do you think that's enough?
> 
> That's a start, the reference to Documentation/filesystems/proc.txt is
> good but I think we can do better. You need to help the user make the
> decision. Why should he/she say Y or N? The user should NOT have to look
> at an external documentation file if the answer is N. I would suggest
> the following:
> 
> Say Y if running any user-space software which takes benefit from this
> interface. For example, rkt is such a piece of software.

That makes it more clear for the user. Thanks!

> That being said, I am curious... Is this interface so expensive that it
> really deserves a separate option, instead of always enabling it? This
> seems to be a fairly generic feature that a lot of scripts and tools
> could benefit from (starting with pstree I suppose.)

I don't think I have enough information to answer that question. I'll CC Cyrill
Gorcunov and Andrew Morton.

-- 

Iago López Galeiras
Software developer @ Endocode AG
iago@endocode.com

Re: config PROC_CHILDREN

[Cyrill Gorcunov]

On Wed, Jul 08, 2015 at 04:18:28PM +0200, Iago López Galeiras wrote:
...
> 
> > That being said, I am curious... Is this interface so expensive that it
> > really deserves a separate option, instead of always enabling it? This
> > seems to be a fairly generic feature that a lot of scripts and tools
> > could benefit from (starting with pstree I suppose.)
> 
> I don't think I have enough information to answer that question. I'll CC Cyrill
> Gorcunov and Andrew Morton.

The interface is not expensive per-se but it was developed for
checkpoint/restore in first place so we've been wrapping any
new code created for this sake with CONFIG_. And we (criu
team) were the only users of this interface at those days
so it had lot of sence to keep it conditionally enabled.

Re: config PROC_CHILDREN

[Djalal Harouni]

On Wed, Jul 08, 2015 at 04:18:28PM +0200, Iago López Galeiras wrote:
> On 07/04/2015 07:07 PM, Jean Delvare wrote:
> > Hi Iago,
> > 
> > Please don't top-post.
> > 
> > On Fri, 3 Jul 2015 11:10:45 +0200, Iago López Galeiras wrote:
> >> Hi Jean,
> >>
> >> The purpose of this option is enabling /proc/<pid>/task/<tid>/children without
> >> having to enable CHECKPOINT_RESTORE, which is hidden behind EXPERT.
> >>
> >> Regarding its lack of help, documentation is in already in place[1] but perhaps
> >> that's not clear for the user because as you say the Kconfig help text is missing.
> >>
> >> I suggest adding something like:
> >>
> >>     Provides a fast way to retrieve first level children pids of a task. See
> >>     <file:Documentation/filesystems/proc.txt> for more information.
> >>
> >> Do you think that's enough?
> > 
> > That's a start, the reference to Documentation/filesystems/proc.txt is
> > good but I think we can do better. You need to help the user make the
> > decision. Why should he/she say Y or N? The user should NOT have to look
> > at an external documentation file if the answer is N. I would suggest
> > the following:
> > 
> > Say Y if running any user-space software which takes benefit from this
> > interface. For example, rkt is such a piece of software.
> 
> That makes it more clear for the user. Thanks!
> 
> > That being said, I am curious... Is this interface so expensive that it
> > really deserves a separate option, instead of always enabling it? This
> > seems to be a fairly generic feature that a lot of scripts and tools
> > could benefit from (starting with pstree I suppose.)
> 
> I don't think I have enough information to answer that question. I'll CC Cyrill
> Gorcunov and Andrew Morton.
IIRC, this was suggested by Andrew Morton, and it results on this patch.

http://www.spinics.net/lists/linux-fsdevel/msg86491.html


> -- 
> 
> Iago López Galeiras
> Software developer @ Endocode AG
> iago@endocode.com

-- 
Djalal Harouni
http://opendz.org

Re: config PROC_CHILDREN

[Jean Delvare]

Le Wednesday 08 July 2015 à 15:50 +0100, Djalal Harouni a écrit :
> On Wed, Jul 08, 2015 at 04:18:28PM +0200, Iago López Galeiras wrote:
> > On 07/04/2015 07:07 PM, Jean Delvare wrote:
> > > That being said, I am curious... Is this interface so expensive that it
> > > really deserves a separate option, instead of always enabling it? This
> > > seems to be a fairly generic feature that a lot of scripts and tools
> > > could benefit from (starting with pstree I suppose.)
> > 
> > I don't think I have enough information to answer that question. I'll CC Cyrill
> > Gorcunov and Andrew Morton.
> IIRC, this was suggested by Andrew Morton, and it results on this patch.
> 
> http://www.spinics.net/lists/linux-fsdevel/msg86491.html

OK, if Andrew said so... :-D

-- 
Jean Delvare
SUSE L3 Support

Re: config PROC_CHILDREN

[Jean Delvare]

Le Wednesday 08 July 2015 à 16:18 +0200, Iago López Galeiras a écrit :
> On 07/04/2015 07:07 PM, Jean Delvare wrote:
> > On Fri, 3 Jul 2015 11:10:45 +0200, Iago López Galeiras wrote:
> >> The purpose of this option is enabling /proc/<pid>/task/<tid>/children without
> >> having to enable CHECKPOINT_RESTORE, which is hidden behind EXPERT.
> >>
> >> Regarding its lack of help, documentation is in already in place[1] but perhaps
> >> that's not clear for the user because as you say the Kconfig help text is missing.
> >>
> >> I suggest adding something like:
> >>
> >>     Provides a fast way to retrieve first level children pids of a task. See
> >>     <file:Documentation/filesystems/proc.txt> for more information.
> >>
> >> Do you think that's enough?
> > 
> > That's a start, the reference to Documentation/filesystems/proc.txt is
> > good but I think we can do better. You need to help the user make the
> > decision. Why should he/she say Y or N? The user should NOT have to look
> > at an external documentation file if the answer is N. I would suggest
> > the following:
> > 
> > Say Y if running any user-space software which takes benefit from this
> > interface. For example, rkt is such a piece of software.
> 
> That makes it more clear for the user. Thanks!

You're welcome. Will you submit a patch, or should I take care of it?

-- 
Jean Delvare
SUSE L3 Support