Re: [PATCH V2] testing/selftests/mqueue: Fix mq_perf_tests to free the allocated cpu set

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