| Message ID | 20220407184008.93879-1-atrajeev@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] testing/selftests/mqueue: Fix mq_perf_tests to free the allocated cpu set | expand |
On 4/7/22 12:40 PM, Athira Rajeev wrote: > The selftest "mqueue/mq_perf_tests.c" use CPU_ALLOC to allocate > CPU set. This cpu set is used further in pthread_attr_setaffinity_np > and by pthread_create in the code. But in current code, allocated > cpu set is not freed. > > Fix this issue by adding CPU_FREE in the "shutdown" function which > is called in most of the error/exit path for the cleanup. Also add > CPU_FREE in some of the error paths where shutdown is not called. > > Fixes: 7820b0715b6f ("tools/selftests: add mq_perf_tests") > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> > --- > Changelog: > From v1 -> v2: > Addressed review comment from Shuah Khan to add > CPU_FREE in other exit paths where it is needed > Thank you. I have a couple of comments on making the error paths simpler. Please see below. > tools/testing/selftests/mqueue/mq_perf_tests.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/mqueue/mq_perf_tests.c b/tools/testing/selftests/mqueue/mq_perf_tests.c > index b019e0b8221c..182434c7898d 100644 > --- a/tools/testing/selftests/mqueue/mq_perf_tests.c > +++ b/tools/testing/selftests/mqueue/mq_perf_tests.c > @@ -180,6 +180,9 @@ void shutdown(int exit_val, char *err_cause, int line_no) > if (in_shutdown++) > return; > > + /* Free the cpu_set allocated using CPU_ALLOC in main function */ > + CPU_FREE(cpu_set); > + > for (i = 0; i < num_cpus_to_pin; i++) > if (cpu_threads[i]) { > pthread_kill(cpu_threads[i], SIGUSR1); > @@ -589,6 +592,7 @@ int main(int argc, char *argv[]) > cpu_set)) { > fprintf(stderr, "Any given CPU may " > "only be given once.\n"); > + CPU_FREE(cpu_set); This could be done in a common error path handling. > exit(1); > } else > CPU_SET_S(cpus_to_pin[cpu], > @@ -607,6 +611,7 @@ int main(int argc, char *argv[]) > queue_path = malloc(strlen(option) + 2); > if (!queue_path) { > perror("malloc()"); > + CPU_FREE(cpu_set); This could be done in a common error path handling. > exit(1); > } > queue_path[0] = '/'; > @@ -619,6 +624,7 @@ int main(int argc, char *argv[]) > } > > if (continuous_mode && num_cpus_to_pin == 0) { > + CPU_FREE(cpu_set); This could be done in a common error path handling. > fprintf(stderr, "Must pass at least one CPU to continuous " > "mode.\n"); > poptPrintUsage(popt_context, stderr, 0); > @@ -628,10 +634,12 @@ int main(int argc, char *argv[]) > cpus_to_pin[0] = cpus_online - 1; > } > > - if (getuid() != 0) > + if (getuid() != 0) { > + CPU_FREE(cpu_set); > ksft_exit_skip("Not running as root, but almost all tests " > "require root in order to modify\nsystem settings. " > "Exiting.\n"); > + } > Why not move this check before CPU_ALLOC and make this the very first check in main()? With this change the other places where CPU_FREE is added right before exit(1). Something like this: err_code: CPU_FREE(cpu_set); exit(code) > max_msgs = fopen(MAX_MSGS, "r+"); > max_msgsize = fopen(MAX_MSGSIZE, "r+"); > thanks, -- Shuah
> On 08-Apr-2022, at 12:31 AM, Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 4/7/22 12:40 PM, Athira Rajeev wrote: >> The selftest "mqueue/mq_perf_tests.c" use CPU_ALLOC to allocate >> CPU set. This cpu set is used further in pthread_attr_setaffinity_np >> and by pthread_create in the code. But in current code, allocated >> cpu set is not freed. >> Fix this issue by adding CPU_FREE in the "shutdown" function which >> is called in most of the error/exit path for the cleanup. Also add >> CPU_FREE in some of the error paths where shutdown is not called. >> Fixes: 7820b0715b6f ("tools/selftests: add mq_perf_tests") >> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> >> --- >> Changelog: >> From v1 -> v2: >> Addressed review comment from Shuah Khan to add >> CPU_FREE in other exit paths where it is needed > > Thank you. I have a couple of comments on making the error > paths simpler. Please see below. > >> tools/testing/selftests/mqueue/mq_perf_tests.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> diff --git a/tools/testing/selftests/mqueue/mq_perf_tests.c b/tools/testing/selftests/mqueue/mq_perf_tests.c >> index b019e0b8221c..182434c7898d 100644 >> --- a/tools/testing/selftests/mqueue/mq_perf_tests.c >> +++ b/tools/testing/selftests/mqueue/mq_perf_tests.c >> @@ -180,6 +180,9 @@ void shutdown(int exit_val, char *err_cause, int line_no) >> if (in_shutdown++) >> return; >> + /* Free the cpu_set allocated using CPU_ALLOC in main function */ >> + CPU_FREE(cpu_set); >> + >> for (i = 0; i < num_cpus_to_pin; i++) >> if (cpu_threads[i]) { >> pthread_kill(cpu_threads[i], SIGUSR1); >> @@ -589,6 +592,7 @@ int main(int argc, char *argv[]) >> cpu_set)) { >> fprintf(stderr, "Any given CPU may " >> "only be given once.\n"); >> + CPU_FREE(cpu_set); > > This could be done in a common error path handling. > >> exit(1); >> } else >> CPU_SET_S(cpus_to_pin[cpu], >> @@ -607,6 +611,7 @@ int main(int argc, char *argv[]) >> queue_path = malloc(strlen(option) + 2); >> if (!queue_path) { >> perror("malloc()"); >> + CPU_FREE(cpu_set); > > This could be done in a common error path handling. > >> exit(1); >> } >> queue_path[0] = '/'; >> @@ -619,6 +624,7 @@ int main(int argc, char *argv[]) >> } >> if (continuous_mode && num_cpus_to_pin == 0) { >> + CPU_FREE(cpu_set); > > This could be done in a common error path handling. > >> fprintf(stderr, "Must pass at least one CPU to continuous " >> "mode.\n"); >> poptPrintUsage(popt_context, stderr, 0); >> @@ -628,10 +634,12 @@ int main(int argc, char *argv[]) >> cpus_to_pin[0] = cpus_online - 1; >> } >> - if (getuid() != 0) >> + if (getuid() != 0) { >> + CPU_FREE(cpu_set); >> ksft_exit_skip("Not running as root, but almost all tests " >> "require root in order to modify\nsystem settings. " >> "Exiting.\n"); >> + } >> > > Why not move this check before CPU_ALLOC and make this the very first > check in main()? > > With this change the other places where CPU_FREE is added right before > exit(1). Something like this: > > err_code: > CPU_FREE(cpu_set); > exit(code) Hi Shuah Thanks for the comments. Addressed these in V3 Athira > >> max_msgs = fopen(MAX_MSGS, "r+"); >> max_msgsize = fopen(MAX_MSGSIZE, "r+"); > > thanks, > -- Shuah
diff --git a/tools/testing/selftests/mqueue/mq_perf_tests.c b/tools/testing/selftests/mqueue/mq_perf_tests.c index b019e0b8221c..182434c7898d 100644 --- a/tools/testing/selftests/mqueue/mq_perf_tests.c +++ b/tools/testing/selftests/mqueue/mq_perf_tests.c @@ -180,6 +180,9 @@ void shutdown(int exit_val, char *err_cause, int line_no) if (in_shutdown++) return; + /* Free the cpu_set allocated using CPU_ALLOC in main function */ + CPU_FREE(cpu_set); + for (i = 0; i < num_cpus_to_pin; i++) if (cpu_threads[i]) { pthread_kill(cpu_threads[i], SIGUSR1); @@ -589,6 +592,7 @@ int main(int argc, char *argv[]) cpu_set)) { fprintf(stderr, "Any given CPU may " "only be given once.\n"); + CPU_FREE(cpu_set); exit(1); } else CPU_SET_S(cpus_to_pin[cpu], @@ -607,6 +611,7 @@ int main(int argc, char *argv[]) queue_path = malloc(strlen(option) + 2); if (!queue_path) { perror("malloc()"); + CPU_FREE(cpu_set); exit(1); } queue_path[0] = '/'; @@ -619,6 +624,7 @@ int main(int argc, char *argv[]) } if (continuous_mode && num_cpus_to_pin == 0) { + CPU_FREE(cpu_set); fprintf(stderr, "Must pass at least one CPU to continuous " "mode.\n"); poptPrintUsage(popt_context, stderr, 0); @@ -628,10 +634,12 @@ int main(int argc, char *argv[]) cpus_to_pin[0] = cpus_online - 1; } - if (getuid() != 0) + if (getuid() != 0) { + CPU_FREE(cpu_set); ksft_exit_skip("Not running as root, but almost all tests " "require root in order to modify\nsystem settings. " "Exiting.\n"); + } max_msgs = fopen(MAX_MSGS, "r+"); max_msgsize = fopen(MAX_MSGSIZE, "r+");
The selftest "mqueue/mq_perf_tests.c" use CPU_ALLOC to allocate CPU set. This cpu set is used further in pthread_attr_setaffinity_np and by pthread_create in the code. But in current code, allocated cpu set is not freed. Fix this issue by adding CPU_FREE in the "shutdown" function which is called in most of the error/exit path for the cleanup. Also add CPU_FREE in some of the error paths where shutdown is not called. Fixes: 7820b0715b6f ("tools/selftests: add mq_perf_tests") Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> --- Changelog: From v1 -> v2: Addressed review comment from Shuah Khan to add CPU_FREE in other exit paths where it is needed tools/testing/selftests/mqueue/mq_perf_tests.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)