Re: [PATCH v2 1/2] selftests/perf_events: Add test for refresh limit signals

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Missing <sys/syscall.h> include for syscall() and __NR_perf_event_open
- [Low] FIXTURE_TEARDOWN unconditionally operates on potentially invalid file descriptor
- [Low] Using EXPECT_EQ for sigaction may cause an ungraceful test crash on setup failure
--

commit 3f198b4c551c8f0caf35b8f6bc25feee2904073b
Author: Leo Yan <leo.yan@arm.com>

selftests/perf_events: Add test for refresh limit signals

This commit adds a test to verify PERF_EVENT_IOC_REFRESH with a
task-clock software event. It uses a real-time signal to queue
notifications and verifies that exactly one POLL_HUP notification is
delivered per iteration when the refresh count reaches zero.

> diff --git a/tools/testing/selftests/perf_events/refresh_signal.c b/tools/testing/selftests/perf_events/refresh_signal.c
> new file mode 100644
> index 0000000000000..5519ea7b56f9d
> --- /dev/null
> +++ b/tools/testing/selftests/perf_events/refresh_signal.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +
> +#include <fcntl.h>
> +#include <linux/perf_event.h>
> +#include <poll.h>
> +#include <signal.h>
> +#include <sys/ioctl.h>
> +#include <time.h>
> +
> +#include "../kselftest_harness.h"

Is it necessary to explicitly include <sys/syscall.h> and <unistd.h>
here?

The test uses syscall() and __NR_perf_event_open, and relying on
implicit transitive includes from kselftest_harness.h might lead to
compilation failures on some architectures or libc implementations.

[ ... ]

> +FIXTURE_SETUP(refresh_signal)
> +{
> +	struct sigaction sa = { 0 };
> +	struct perf_event_attr attr = { 0 };
> +
> +	sa.sa_sigaction = sigio_handler;
> +	sa.sa_flags = SA_SIGINFO;
> +	sigemptyset(&sa.sa_mask);
> +
> +	/* Use a real-time signal so notifications are reliably queued */
> +	EXPECT_EQ(sigaction(RT_SIG, &sa, &self->old_sa), 0);

If sigaction() fails, EXPECT_EQ logs a failure but allows the setup to
continue.

When the perf event triggers the first RT_SIG, the process will lack a
custom handler and the default action will terminate the test program.

Would it be safer to use ASSERT_EQ here so the test aborts cleanly on
setup failure?

[ ... ]

> +	self->fd = syscall(__NR_perf_event_open, &attr, 0, -1, -1, 0);
> +	ASSERT_NE(self->fd, -1);
> +
> +	/* Enable async notification */
> +	ASSERT_EQ(fcntl(self->fd, F_SETFL, fcntl(self->fd, F_GETFL) | O_ASYNC), 0);
> +
> +	/* Receive the signal for current process */
> +	ASSERT_EQ(fcntl(self->fd, F_SETOWN, getpid()), 0);
> +
> +	/* Use signo instead of the default SIGIO */
> +	ASSERT_EQ(fcntl(self->fd, F_SETSIG, RT_SIG), 0);
> +}
> +
> +FIXTURE_TEARDOWN(refresh_signal)
> +{
> +	EXPECT_EQ(ioctl(self->fd, PERF_EVENT_IOC_DISABLE, 0), 0);
> +
> +	close(self->fd);
> +	sigaction(RT_SIG, &self->old_sa, NULL);
> +}

If syscall(__NR_perf_event_open) fails during FIXTURE_SETUP, self->fd is
assigned -1 and the setup aborts via ASSERT_NE.

However, the kselftest framework still executes FIXTURE_TEARDOWN, which
will unconditionally call ioctl() and close() on -1. Could this generate
additional and confusing test failure logs?

Should we verify if self->fd >= 0 before attempting to operate on it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-arm_cs_clean_perf_handle-v2-0-cbb29c3b3661@arm.com?part=1