[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZqgHTKOciVShT8IN@ghost>
Date: Mon, 29 Jul 2024 14:19:08 -0700
From: Charlie Jenkins <charlie@...osinc.com>
To: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
Adrian Hunter <adrian.hunter@...el.com>,
Atish Patra <atishp@...osinc.com>,
Al Viro <viro@...iv.linux.org.uk>
Cc: linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] perf: Add PERF_EVENT_IOC_SET_EVENT_LIMIT
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@...osinc.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
>
Powered by blists - more mailing lists