[v2,2/2] lib/tst_pid.c: Increase PIDS_RESERVED to avoid fork failure.

After Adjusting how we count used pid, we increase
the number of PIDS_RESERVED to void fork failure.

Suggested-by: Petr Vorel <pvorel@suse.cz>
Signed-off-by: Leo Yu-Chi Liang <ycliang@andestech.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
Changes v1 -> v2
* Split into two patches
---
 lib/tst_pid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Leo,

> After Adjusting how we count used pid, we increase
> the number of PIDS_RESERVED to void fork failure.
nit: in this case I'd actually keep changes in single commit
(otherwise first commit alone would break tests),

Kind regards,
Petr

> Suggested-by: Petr Vorel <pvorel@suse.cz>
> Signed-off-by: Leo Yu-Chi Liang <ycliang@andestech.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> ---
> Changes v1 -> v2
> * Split into two patches
> ---
>  lib/tst_pid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/lib/tst_pid.c b/lib/tst_pid.c
> index a282f8cc9..7582e4828 100644
> --- a/lib/tst_pid.c
> +++ b/lib/tst_pid.c
> @@ -36,7 +36,7 @@
>  #define CGROUPS_V1_SLICE_FMT "/sys/fs/cgroup/pids/user.slice/user-%d.slice/pids.max"
>  #define CGROUPS_V2_SLICE_FMT "/sys/fs/cgroup/user.slice/user-%d.slice/pids.max"
>  /* Leave some available processes for the OS */
> -#define PIDS_RESERVE 50
> +#define PIDS_RESERVE 200

>  pid_t tst_get_unused_pid_(void (*cleanup_fn) (void))
>  {

Hi!
> > After Adjusting how we count used pid, we increase
> > the number of PIDS_RESERVED to void fork failure.
> nit: in this case I'd actually keep changes in single commit
> (otherwise first commit alone would break tests),

Do we get a different result from ps and parsing /proc? That sounds
strange...

Hi Petr,

On Tue, Feb 14, 2023 at 02:37:14PM +0100, Petr Vorel wrote:
> Hi Leo,
> 
> > After Adjusting how we count used pid, we increase
> > the number of PIDS_RESERVED to void fork failure.
> nit: in this case I'd actually keep changes in single commit
> (otherwise first commit alone would break tests),
> 

That makes sense!
Then we could probably drop this v2 patch
and stay with the v1?

Best regards,
Leo

> Kind regards,
> Petr
> 
> > Suggested-by: Petr Vorel <pvorel@suse.cz>
> > Signed-off-by: Leo Yu-Chi Liang <ycliang@andestech.com>
> > Reviewed-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > Changes v1 -> v2
> > * Split into two patches
> > ---
> >  lib/tst_pid.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> > diff --git a/lib/tst_pid.c b/lib/tst_pid.c
> > index a282f8cc9..7582e4828 100644
> > --- a/lib/tst_pid.c
> > +++ b/lib/tst_pid.c
> > @@ -36,7 +36,7 @@
> >  #define CGROUPS_V1_SLICE_FMT "/sys/fs/cgroup/pids/user.slice/user-%d.slice/pids.max"
> >  #define CGROUPS_V2_SLICE_FMT "/sys/fs/cgroup/user.slice/user-%d.slice/pids.max"
> >  /* Leave some available processes for the OS */
> > -#define PIDS_RESERVE 50
> > +#define PIDS_RESERVE 200
> 
> >  pid_t tst_get_unused_pid_(void (*cleanup_fn) (void))
> >  {

Hi Cyril,

On Tue, Feb 14, 2023 at 02:43:35PM +0100, Cyril Hrubis wrote:
> Hi!
> > > After Adjusting how we count used pid, we increase
> > > the number of PIDS_RESERVED to void fork failure.
> > nit: in this case I'd actually keep changes in single commit
> > (otherwise first commit alone would break tests),
> 
> Do we get a different result from ps and parsing /proc? That sounds
> strange...

I think that's because "ps -eT" would list threads with the same PID
but with different SPID.

I get the following output on my VM.

ycliang@ubuntu01:~$ ps -eT | wc -l
170
ycliang@ubuntu01:~$ ls -d /proc/[0-9]* | xargs -n1 | wc -l
127

Best regards,
Leo

> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz

Hi!
> > > > After Adjusting how we count used pid, we increase
> > > > the number of PIDS_RESERVED to void fork failure.
> > > nit: in this case I'd actually keep changes in single commit
> > > (otherwise first commit alone would break tests),
> > 
> > Do we get a different result from ps and parsing /proc? That sounds
> > strange...
> 
> I think that's because "ps -eT" would list threads with the same PID
> but with different SPID.
> 
> I get the following output on my VM.
> 
> ycliang@ubuntu01:~$ ps -eT | wc -l
> 170
> ycliang@ubuntu01:~$ ls -d /proc/[0-9]* | xargs -n1 | wc -l
> 127

Adjusting the RESERVED constant is then a lousy workaround that wouldn't
work for systems with many threads per process.

One alternative would be to open /proc/$PID/status and read the number
of threads from there. Should be as easy as one call to
SAFE_FILE_LINES_SCANF().

Hi!
> > > > > After Adjusting how we count used pid, we increase
> > > > > the number of PIDS_RESERVED to void fork failure.
> > > > nit: in this case I'd actually keep changes in single commit
> > > > (otherwise first commit alone would break tests),
> > > 
> > > Do we get a different result from ps and parsing /proc? That sounds
> > > strange...
> > 
> > I think that's because "ps -eT" would list threads with the same PID
> > but with different SPID.
> > 
> > I get the following output on my VM.
> > 
> > ycliang@ubuntu01:~$ ps -eT | wc -l
> > 170
> > ycliang@ubuntu01:~$ ls -d /proc/[0-9]* | xargs -n1 | wc -l
> > 127
> 
> Adjusting the RESERVED constant is then a lousy workaround that wouldn't
> work for systems with many threads per process.
> 
> One alternative would be to open /proc/$PID/status and read the number
> of threads from there. Should be as easy as one call to
> SAFE_FILE_LINES_SCANF().

Thinking of it again using SAFE_FILE_LINES_SCANF() may be prone to a
race where the process exits and the file disappears between the call to
the readdir() and the open in the SAFE_FILE_LINES_SCANF() so I suppose
that we should use just the FILE_LINES_SCANF() instead and add the
threads value only if the call succeeded.

Hi Cyril,

On Tue, Feb 14, 2023 at 04:09:52PM +0100, Cyril Hrubis wrote:
> Hi!
> > > > > > After Adjusting how we count used pid, we increase
> > > > > > the number of PIDS_RESERVED to void fork failure.
> > > > > nit: in this case I'd actually keep changes in single commit
> > > > > (otherwise first commit alone would break tests),
> > > > 
> > > > Do we get a different result from ps and parsing /proc? That sounds
> > > > strange...
> > > 
> > > I think that's because "ps -eT" would list threads with the same PID
> > > but with different SPID.
> > > 
> > > I get the following output on my VM.
> > > 
> > > ycliang@ubuntu01:~$ ps -eT | wc -l
> > > 170
> > > ycliang@ubuntu01:~$ ls -d /proc/[0-9]* | xargs -n1 | wc -l
> > > 127
> > 
> > Adjusting the RESERVED constant is then a lousy workaround that wouldn't
> > work for systems with many threads per process.
> > 
> > One alternative would be to open /proc/$PID/status and read the number
> > of threads from there. Should be as easy as one call to
> > SAFE_FILE_LINES_SCANF().
> 
> Thinking of it again using SAFE_FILE_LINES_SCANF() may be prone to a
> race where the process exits and the file disappears between the call to
> the readdir() and the open in the SAFE_FILE_LINES_SCANF() so I suppose
> that we should use just the FILE_LINES_SCANF() instead and add the
> threads value only if the call succeeded.

Thanks for the advice!
Will send a v3.

Just out of curiosity, is there any reason that we should do this in plain C ?
(Otherwise, we could drop this patchset and stay with the current implementation)

Best regards,
Leo

> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz

Hi!
> Just out of curiosity, is there any reason that we should do this in plain C ?
> (Otherwise, we could drop this patchset and stay with the current implementation)

There are a few, calling random scripts from C is a bad practice
overall.

Portabilitity may be one of the problems, there are several
iimplementations of the basic UNIX utilities for Linux eg. coreutils,
busybox, toybox, etc. These implemtations are subtly incompatible, not
all commandline options are supported and so on. And for the busybox and
toybox some options can be disabled at a compile time. We leaned that
sometimes you have to double check if the functionality available and
most of the time the end result is that it's just easier to rewrite the
code in C.

We also have rule to make tests as self contained as possible, which
simplifies debugging. One of the problems is that we do not have the
environment the shell code runs in under control, we had a few test
failing for non-standard settings of the LANG variables.

In this case the code is reasonably simple, so it will be less likely to
be problematic, however I would stil lean towards replacing it with C
code.

tl;dr Calling shell code from C programs makes things less predictable
      and possibly unstable.

Hi Cyril,

On Thu, Feb 16, 2023 at 09:46:59AM +0100, Cyril Hrubis wrote:
> Hi!
> > Just out of curiosity, is there any reason that we should do this in plain C ?
> > (Otherwise, we could drop this patchset and stay with the current implementation)
> 
> There are a few, calling random scripts from C is a bad practice
> overall.
> 
> Portabilitity may be one of the problems, there are several
> iimplementations of the basic UNIX utilities for Linux eg. coreutils,
> busybox, toybox, etc. These implemtations are subtly incompatible, not
> all commandline options are supported and so on. And for the busybox and
> toybox some options can be disabled at a compile time. We leaned that
> sometimes you have to double check if the functionality available and
> most of the time the end result is that it's just easier to rewrite the
> code in C.
> 
> We also have rule to make tests as self contained as possible, which
> simplifies debugging. One of the problems is that we do not have the
> environment the shell code runs in under control, we had a few test
> failing for non-standard settings of the LANG variables.
> 
> In this case the code is reasonably simple, so it will be less likely to
> be problematic, however I would stil lean towards replacing it with C
> code.
> 
> tl;dr Calling shell code from C programs makes things less predictable
>       and possibly unstable.
> 

Understood! Thank you for the detailed explanation!!
Will send a v3 patch ASAP in accordance with your advice!

Best regards,
Leo

> -- 
> Cyril Hrubis
> chrubis@suse.cz

...
> Understood! Thank you for the detailed explanation!!
> Will send a v3 patch ASAP in accordance with your advice!

Leo, thanks a lot for investing your time in LTP cleanup!

Kind regards,
Petr