Re: [PATCH v3 09/19] perf test: Add named_threads workload

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Data race and signed integer overflow on the global `named_threads_work` variable leading to Undefined Behavior.
- [Medium] Lack of synchronization barriers guarantees sequential execution instead of concurrent thread execution.
- [Medium] Background threads are leaked on the `pthread_create` error path.
--

commit d8bc49305dcacce453a6a52dcb27371073bfc5b0
Author: James Clark <james.clark@linaro.org>

perf test: Add named_threads workload

Add a workload that runs X threads executing a unique function to perform
multiplication in a loop. This is intended to test that processor trace
decoding handles concurrent threads correctly.

> diff --git a/tools/perf/tests/workloads/named_threads.c b/tools/perf/tests/workloads/named_threads.c
> new file mode 100644
> index 0000000000000..dc8070a98df4c
> --- /dev/null
> +++ b/tools/perf/tests/workloads/named_threads.c
> @@ -0,0 +1,109 @@
[ ... ]
> +static int iterations = 500;
> +int named_threads_work = 1234;
> +
> +typedef void *(*thread_fn_t)(void *);
> +
> +#define DEFINE_THREAD(n)						\
> +noinline void *named_threads_thread##n(void *arg __maybe_unused)	\
> +{									\
> +	pthread_setname_np(pthread_self(), "thread" #n);		\
> +	for (int i = 0; i < iterations; i++)				\
> +		named_threads_work *= 3;				\

[Severity: Medium]
Does this loop trigger undefined behavior due to data races and signed
integer overflow? 

Multiple threads concurrently modify the global named_threads_work without 
synchronization. Additionally, multiplying 1234 by 3 for 500 iterations 
will overflow the signed integer. 

Could modern C compilers optimize this loop out entirely due to the UB, 
preventing the generation of the processor trace samples this test relies 
on?

> +									\
> +	return NULL;							\
> +}
[ ... ]
> +static int named_threads(int argc, const char **argv)
> +{
[ ... ]
> +	for (int i = 0; i < nr_threads; i++) {
> +		int ret;
> +
> +		ret = pthread_create(&threads[i], NULL, thread_fns[i], NULL);

[Severity: Medium]
Will these threads actually run concurrently as intended?

The overhead of pthread_create is typically longer than the time it takes
for the thread to execute its workload, especially since the loop only 
executes 500 simple multiplications. 

Without a synchronization barrier (like pthread_barrier_wait), earlier 
threads might complete their loops and exit before the main thread finishes 
spawning the remaining threads, resulting in sequential execution rather 
than overlapping concurrent tasks.

> +		if (ret) {
> +			fprintf(stderr, "Error: failed to create thread%d: %s\n",
> +				i + 1, strerror(ret));
> +			return 1;

[Severity: Medium]
Does this error path leak previously created background threads?

If pthread_create fails for any thread index greater than 0, the function
returns 1 immediately. It doesn't join the successfully created threads from
earlier iterations. 

If perf test is run in non-forking mode (e.g., with --dont-fork), these 
leaked background threads could persist and interfere with subsequent tests.

> +		}
> +	}
> +
> +	for (int i = 0; i < nr_threads; i++)
> +		pthread_join(threads[i], NULL);
> +
> +	return err;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-james-cs-context-tracking-fix-v3-0-c392945d9ed5@linaro.org?part=9