> On 08-Apr-2022, at 12:31 AM, Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> >> --- >> 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