| Message ID | 20230323160442.671164-1-teo.coupriediaz@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ipc/msgstress03: Assume all forks will run concurently | expand |
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>
Reviewed-by: Li Wang <liwang@redhat.com>
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...
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 >
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), "