[LTP] [PATCH] ipc/msgstress03: Assume all forks will run concurently

[LTP] [PATCH] ipc/msgstress03: Assume all forks will run concurently

[Teo Couprie Diaz]

It appears that msgstress03 doesn't account for all PIDs that its children
can use, as it expects the tasks will terminate quickly and not reach
the PID limit.
On some systems, this assumption can be invalid and the PID limit
will be hit.
Change the limit to account for all possible children at once, knowning
that each child will fork as well.

Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com>
Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
---
This patch was already reviewed as part of a larger series[0]. The rest of
the series needs a large rework to be merged, but I felt this patch was a
simple and independnt enough to warrant a resend.
No changes were made.

CI Build: https://github.com/Teo-CD/ltp/actions/runs/4502197808

[0]: https://lists.linux.it/pipermail/ltp/2023-February/033007.html

 testcases/kernel/syscalls/ipc/msgstress/msgstress03.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c b/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
index 3cb70ab18..0c46927b8 100644
--- a/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
+++ b/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
@@ -109,7 +109,7 @@ int main(int argc, char **argv)
 		}
 	}
 
-	free_pids = tst_get_free_pids(cleanup);
+	free_pids = tst_get_free_pids(cleanup) / 2;
 	if (nprocs >= free_pids) {
 		tst_resm(TINFO,
 			 "Requested number of processes higher than limit (%d > %d), "
-- 
2.25.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] ipc/msgstress03: Assume all forks will run concurently

[Leo Liang]

On Thu, Mar 23, 2023 at 04:04:42PM +0000, Teo Couprie Diaz wrote:
> It appears that msgstress03 doesn't account for all PIDs that its children
> can use, as it expects the tasks will terminate quickly and not reach
> the PID limit.
> On some systems, this assumption can be invalid and the PID limit
> will be hit.
> Change the limit to account for all possible children at once, knowning
> that each child will fork as well.
> 
> Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com>
> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
> This patch was already reviewed as part of a larger series[0]. The rest of
> the series needs a large rework to be merged, but I felt this patch was a
> simple and independnt enough to warrant a resend.
> No changes were made.
> 
> CI Build: https://github.com/Teo-CD/ltp/actions/runs/4502197808
> 
> [0]: https://lists.linux.it/pipermail/ltp/2023-February/033007.html
> 
>  testcases/kernel/syscalls/ipc/msgstress/msgstress03.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

LGTM!
Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] ipc/msgstress03: Assume all forks will run concurently

[Li Wang]

Reviewed-by: Li Wang <liwang@redhat.com>


-- 
Regards,
Li Wang


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] ipc/msgstress03: Assume all forks will run concurently

[Cyril Hrubis]

Hi!
> It appears that msgstress03 doesn't account for all PIDs that its children
> can use, as it expects the tasks will terminate quickly and not reach
> the PID limit.
> On some systems, this assumption can be invalid and the PID limit
> will be hit.
> Change the limit to account for all possible children at once, knowning
> that each child will fork as well.
> 
> Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com>
> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
> This patch was already reviewed as part of a larger series[0]. The rest of
> the series needs a large rework to be merged, but I felt this patch was a
> simple and independnt enough to warrant a resend.
> No changes were made.
> 
> CI Build: https://github.com/Teo-CD/ltp/actions/runs/4502197808
> 
> [0]: https://lists.linux.it/pipermail/ltp/2023-February/033007.html
> 
>  testcases/kernel/syscalls/ipc/msgstress/msgstress03.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c b/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
> index 3cb70ab18..0c46927b8 100644
> --- a/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
> +++ b/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
> @@ -109,7 +109,7 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	free_pids = tst_get_free_pids(cleanup);
> +	free_pids = tst_get_free_pids(cleanup) / 2;
>  	if (nprocs >= free_pids) {
>  		tst_resm(TINFO,
>  			 "Requested number of processes higher than limit (%d > %d), "

The logic behind the change is good, however I wonder if this would
produce a bit confusing message in case that we are over limit and
setting the value to the current maximum. Can we at least multiply the
values printed in the TINFO message by 2 so that we get real limits
instead of halves?

Generally the nprocs is actually the number of message queues, which is
confusing itself, but that is probably left for a bigger cleanup of the
testcase...

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] ipc/msgstress03: Assume all forks will run concurently

[Teo Couprie Diaz]

Hi Cyril,

Thanks for the review, sorry for the delay in responding.

On 30/03/2023 12:32, Cyril Hrubis wrote:
> Hi!
>> It appears that msgstress03 doesn't account for all PIDs that its children
>> can use, as it expects the tasks will terminate quickly and not reach
>> the PID limit.
>> On some systems, this assumption can be invalid and the PID limit
>> will be hit.
>> Change the limit to account for all possible children at once, knowning
>> that each child will fork as well.
>>
>> Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com>
>> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
>> ---
>> This patch was already reviewed as part of a larger series[0]. The rest of
>> the series needs a large rework to be merged, but I felt this patch was a
>> simple and independnt enough to warrant a resend.
>> No changes were made.
>>
>> CI Build: https://github.com/Teo-CD/ltp/actions/runs/4502197808
>>
>> [0]: https://lists.linux.it/pipermail/ltp/2023-February/033007.html
>>
>>   testcases/kernel/syscalls/ipc/msgstress/msgstress03.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c b/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
>> index 3cb70ab18..0c46927b8 100644
>> --- a/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
>> +++ b/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
>> @@ -109,7 +109,7 @@ int main(int argc, char **argv)
>>   		}
>>   	}
>>   
>> -	free_pids = tst_get_free_pids(cleanup);
>> +	free_pids = tst_get_free_pids(cleanup) / 2;
>>   	if (nprocs >= free_pids) {
>>   		tst_resm(TINFO,
>>   			 "Requested number of processes higher than limit (%d > %d), "
> The logic behind the change is good, however I wonder if this would
> produce a bit confusing message in case that we are over limit and
> setting the value to the current maximum. Can we at least multiply the
> values printed in the TINFO message by 2 so that we get real limits
> instead of halves?
You're right, and without the patch it could be weird as well as the 
maximum could be hit and the warning not appear.
I will send a v2 that does something similar to msgstress04 where I 
don't change the free_pids itself.
>
> Generally the nprocs is actually the number of message queues, which is
> confusing itself, but that is probably left for a bigger cleanup of the
> testcase...
It seems that msgstress04 is a bit clearer on this point, as it has 
nr_msgqs on top of nprocs.
But I agree, if I had the time I would have been happy cleaning up the 
test more.
In the meantime I hope this fix can be useful for some.

Thanks again for the review,
Best regards
Téo-CD

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp