Re: [PATCH] perf bench: Add atomic CAS benchmark

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf bench: 32-bit integer overflow computing runtime in worker()
- [Low] perf bench: missing stdbool.h inclusion for false in worker()
--

commit 37bbeabb11e8b0698696e276ba97886862aa6887
Author: Changbin Du <changbin.du@intel.com>

perf bench: Add atomic CAS benchmark

This commit introduces a new atomic collection to perf bench for measuring
the performance of compare-and-swap (CAS) operations. It evaluates
multi-threaded contention to help identify latency and scalability patterns.

> diff --git a/tools/perf/bench/atomic.c b/tools/perf/bench/atomic.c
> new file mode 100644
> index 0000000000000..ce97025018ab3
> --- /dev/null
> +++ b/tools/perf/bench/atomic.c

[ ... ]

> +static void *worker(void *arg)
> +{
> +	struct worker_stats *ws = (struct worker_stats *)arg;
> +	struct timespec tstart, tend;
> +	u64 i;
> +
> +	pthread_barrier_wait(&start_barrier);
> +
> +	clock_gettime(CLOCK_MONOTONIC, &tstart);
> +	for (i = 0; i < iterations; i++) {
> +		u64 old_val, new_val;
> +
> +		do {
> +			old_val = __atomic_load_n(&shared_counter,
> +						  __ATOMIC_RELAXED);
> +			new_val = old_val + 1;
> +		} while (!__atomic_compare_exchange_n(&shared_counter,
> +						      &old_val, new_val,
> +						      false,

[Severity: Low]
Does this file need to explicitly include <stdbool.h>?

The POSIX guidelines for perf tool compatibility with musl require files using
POSIX constants like false to directly include the headers where they are
declared. Relying on implicit inclusion from other headers can cause
compilation failures.

> +						      __ATOMIC_RELAXED,
> +						      __ATOMIC_RELAXED));
> +	}
> +	clock_gettime(CLOCK_MONOTONIC, &tend);
> +
> +	ws->runtime_ns	= (tend.tv_sec - tstart.tv_sec) * NSEC_PER_SEC +

[Severity: Medium]
Can this runtime calculation overflow on 32-bit architectures?

When compiling for 32-bit platforms where time_t is a 32-bit signed integer,
tv_sec is also a 32-bit signed integer. Since NSEC_PER_SEC is defined as
1000000000L (which is 32-bit signed on 32-bit platforms), the multiplication
is performed as a 32-bit signed integer operation.

If the time difference exceeds 2.14 seconds, the result wraps around before
being converted and assigned to the u64 runtime_ns field. Given the default
iterations take around 7.3 seconds based on the commit message, wouldn't this
guarantee an overflow during a native 32-bit run, leading to incorrect
benchmark results?

> +			  (tend.tv_nsec - tstart.tv_nsec);
> +
> +	return NULL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930091617.4189736-1-changbin.du@gmail.com?part=1