Re: [PATCH 0/2] perf: Add PERF_EVENT_IOC_SET_EVENT_LIMIT

[Charlie Jenkins <charlie@rivosinc.com>]

On Wed, Jul 24, 2024 at 03:54:10PM -0700, Charlie Jenkins wrote:
> My motivation here is to have a program that is able to detect when an
> arbitrary other program has reviewed some number of PMU overflows.
> PERF_EVENT_IOC_REFRESH is not useful in the scenario since that ioctl
> enables the event immediately. This new ioctl flag
> PERF_EVENT_IOC_SET_EVENT_LIMIT allows a program to setup a event_limit
> and then enable the event when ready at some later time.
> 
> This solves the first problem. The second problem that I am debating a
> solution for is that the refresh SIGIO signals are marked as duplicate
> signals and not always delivered to userspace. I will attach a simple
> program below that can be ran a handful of times to catch this behavior.
> REFRESH is supposed to send a SIGIO for every overflow, with an si_code
> of POLL_IN for each signal except the last one which has a si_code of
> POLL_HUP. However, __send_signal_locked() in kernel/signal.c will
> occasionally catch this POLL_HUP signal before all POLL_IN signals have
> been processed, thus treating this signal as a duplicate and dropping
> with this logic:
> 
> 	pending = (type != PIDTYPE_PID) ? &t->signal->shared_pending : &t->pending;
> 	/*
> 	 * Short-circuit ignored signals and support queuing
> 	 * exactly one non-rt signal, so that we can get more
> 	 * detailed information about the cause of the signal.
> 	 */
> 	result = TRACE_SIGNAL_ALREADY_PENDING;
> 	if (legacy_queue(pending, sig))
> 		goto ret;
> 

+ Al Viro

Do you have any input into why signals with the same signal number but
different si_code are marked as duplicates of each other? Does it need
to work this way?

- Charlie

> I am sending this patch to begin a conversation on this issue with
> REFRESH. One solution could be to only treat a signal as a duplicate in
> __send_signal_locked() if the si_code is the same. Another solution is
> to only use event_limits with a value of one, as any other number will
> run into this race condition. If consistent signals are desired, sigtrap
> can be used, but that also doesn't support enable_on_exec.
> 
> Here is a minimal test program for dropped signals. Can be ran on a loop until
> program halts waiting for signal that will never arrive.
> 
> while ./signal 1 1; do sleep .01; done
> 
> // signal.c
> 
> int fd;
> pid_t pid;
> 
> struct read_format {
> 	unsigned long long value;
> 	unsigned long long time_enabled;
> 	unsigned long long time_running;
> };
> 
> int counter;
> 
> static inline int sys_perf_event_open(struct perf_event_attr *attr, int pid, int cpu, int group_fd,
> 		        	      unsigned long flags)
> {
> 	return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd, flags);
> }
> 
> static void sig_handler(int signum, siginfo_t *siginfo, void *user_context)
> {
> 	struct read_format rd;
> 	counter++;
> 	read(fd, &rd, sizeof(rd));
> 	printf("fd: %d, signum: %d, si_code: %d, counter: %llu, time to overflow: %llu. time enabled: %llu\n", signum, siginfo->si_fd, siginfo->si_code, rd.value, rd.time_running, rd.time_enabled);
> 	if (counter == REFRESH_COUNT) {
> 		printf("Hit refresh count\n");
> 		close(fd);
> 		kill(pid, SIGKILL);
> 		if (siginfo->si_code == POLL_HUP) {
> 			exit(EXIT_SUCCESS); 
> 		} else {
> 			printf("INCORRECT COUNT! Expected POLL_HUP on second signal.\n");
> 			exit(EXIT_FAILURE); 
> 		}
> 	}
> }
> 
> int main(int argc, char *argv[])
> {
> 	char sbuf[128];
> 
> 	if (argc < 3)
> 		goto invalid_args;
> 
> 	unsigned long long num_instructions = atoll(argv[1]);
> 	unsigned int num_samples = atoi(argv[2]);
> 	unsigned long long sample_periods = num_instructions / num_samples;
> 
> 	pid = fork();
> 
> 	if (pid) {
> 		struct sigaction sa = {
> 			.sa_sigaction = (void *) sig_handler,
> 			.sa_flags = SA_SIGINFO
> 		};
> 		struct perf_event_attr pe = {
> 			.type = PERF_TYPE_HARDWARE,
> 			.size = sizeof(pe),
> 			.config = PERF_COUNT_HW_INSTRUCTIONS,
> 			.sample_period = sample_periods,
> 			.wakeup_events = 1,
> 			.disabled = 1,
> 			.sample_type = PERF_SAMPLE_IP | PERF_SAMPLE_TID | PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD,
> 			.read_format = PERF_FORMAT_TOTAL_TIME_RUNNING | PERF_FORMAT_TOTAL_TIME_ENABLED,
> 			.exclude_kernel = 1,
> 			.exclude_idle = 1,
> 			.exclude_hv = 1
> 		};
> 
> 		if (sigaction(SIGIO, &sa, NULL) < 0) {
> 			fprintf(stderr, "FAILED setting up signal handler\n");
> 			exit(EXIT_FAILURE);
> 		}
> 
> 		fd = sys_perf_event_open(&pe, pid, -1, -1, PERF_FLAG_FD_CLOEXEC);
> 
> 		if (fd == -1) {
> 			fprintf(stderr, "FAILED opening perf: %s\n", strerror_r(errno, sbuf, sizeof(sbuf)));
> 			exit(EXIT_FAILURE);
> 		}
> 
> 		ioctl(fd, PERF_EVENT_IOC_RESET, 0);
> 		ioctl(fd, PERF_EVENT_IOC_REFRESH, REFRESH_COUNT);
> 
> 		fcntl(fd, F_SETFL, O_RDWR | O_NONBLOCK | O_ASYNC);
> 		fcntl(fd, F_SETSIG, SIGIO);
> 		fcntl(fd, F_SETOWN, getpid());
> 
> 		while (1);
> 	} else {
> 		while (1);
> 	}
> 
> invalid_args:
> 	printf("FAILED: Please provide at least 2 args: number of instructions, number of samples. eg: %s 10000 5\n", argv[0]);
> 	exit(EXIT_FAILURE);
> }
> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
> Charlie Jenkins (2):
>       perf: Add PERF_EVENT_IOC_INC_EVENT_LIMIT
>       perf: Document PERF_EVENT_IOC_SET_EVENT_LIMIT
> 
>  include/linux/perf_event.h            |  4 ++--
>  include/uapi/linux/perf_event.h       |  1 +
>  kernel/events/core.c                  | 17 +++++++++++------
>  tools/include/uapi/linux/perf_event.h |  1 +
>  tools/perf/design.txt                 |  5 +++++
>  5 files changed, 20 insertions(+), 8 deletions(-)
> ---
> base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd
> change-id: 20240724-perf_set_event_limit-079f1b996376
> -- 
> - Charlie
>