Re: [PATCH 3/7] libperf: Add support for overflow handling of sampling events

[Namhyung Kim <namhyung@kernel.org>]

On Fri, Apr 22, 2022 at 2:44 AM Shunsuke Nakamura
<nakamura.shun@fujitsu.com> wrote:
>
> Extend the fields of the opts structure to set up overflow handling
> for sampling events. Also, add processing to set signal handlers in
> perf_evsel__open_opts.
>
> Signed-off-by: Shunsuke Nakamura <nakamura.shun@fujitsu.com>
> ---
>  tools/lib/perf/Documentation/libperf.txt |  6 +-
>  tools/lib/perf/Makefile                  |  1 +
>  tools/lib/perf/evsel.c                   | 79 ++++++++++++++++++++++++
>  tools/lib/perf/include/perf/evsel.h      |  6 +-
>  tools/lib/perf/tests/test-evlist.c       |  1 -
>  5 files changed, 90 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/perf/Documentation/libperf.txt b/tools/lib/perf/Documentation/libperf.txt
> index 6ca91ca94e01..ec93b1c75ebe 100644
> --- a/tools/lib/perf/Documentation/libperf.txt
> +++ b/tools/lib/perf/Documentation/libperf.txt
> @@ -137,8 +137,12 @@ SYNOPSIS
>            size_t sz;
>
>            unsigned long open_flags;       /* perf_event_open flags */
> +          int flags;                      /* fcntl flags */

What about just rename it to fcntl_flags ?

> +          unsigned int signal;

int ?

> +          int owner_type;

Please add a comment that it's for F_SETOWN_EX.

> +          struct sigaction *sig;
>    };
> -  #define perf_evsel_open_opts__last_field open_flags
> +  #define perf_evsel_open_opts__last_field sig
>
>    #define LIBPERF_OPTS(TYPE, NAME, ...)
>
> diff --git a/tools/lib/perf/Makefile b/tools/lib/perf/Makefile
> index 21df023a2103..4c8fae193cf5 100644
> --- a/tools/lib/perf/Makefile
> +++ b/tools/lib/perf/Makefile
> @@ -75,6 +75,7 @@ override CFLAGS += -Werror -Wall
>  override CFLAGS += -fPIC
>  override CFLAGS += $(INCLUDES)
>  override CFLAGS += -fvisibility=hidden
> +override CFLAGS += -D_GNU_SOURCE
>
>  all:
>
> diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
> index 00c0cea43b52..a289f6c44d7c 100644
> --- a/tools/lib/perf/evsel.c
> +++ b/tools/lib/perf/evsel.c
> @@ -460,6 +460,79 @@ void perf_counts_values__scale(struct perf_counts_values *count,
>                 *pscaled = scaled;
>  }
>
> +static int perf_evsel__run_fcntl(struct perf_evsel *evsel,
> +                                unsigned int cmd, unsigned long arg,
> +                                int cpu_map_idx)

I think it'd be better to have _cpu suffix in the function name
as it handles on the specific cpu.

> +{
> +       int thread;
> +
> +       for (thread = 0; thread < xyarray__max_y(evsel->fd); thread++) {
> +               int err;
> +               int *fd = FD(evsel, cpu_map_idx, thread);
> +
> +               if (!fd || *fd < 0)
> +                       return -1;
> +
> +               err = fcntl(*fd, cmd, arg);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static int perf_evsel__set_signal_handler(struct perf_evsel *evsel,
> +                                         struct perf_evsel_open_opts *opts)
> +{
> +       unsigned int flags;
> +       unsigned int signal;
> +       struct f_owner_ex owner;
> +       struct sigaction *sig;

It can be confused with the signal number, how about sigact?


> +       int cpu_map_idx;
> +       int err = 0;
> +
> +       flags = OPTS_GET(opts, flags, (O_RDWR | O_NONBLOCK | O_ASYNC));
> +       signal = OPTS_GET(opts, signal, SIGIO);
> +       owner.type = OPTS_GET(opts, owner_type, F_OWNER_PID);
> +       sig = OPTS_GET(opts, sig, NULL);
> +
> +       if (flags == 0 && signal == 0 && !owner.type == 0 && sig == 0)

You man want to check owner.type without "!".

> +               return err;
> +
> +       err = sigaction(signal, sig, NULL);
> +       if (err)
> +               return err;
> +
> +       switch (owner.type) {
> +       case F_OWNER_PID:
> +               owner.pid = getpid();
> +               break;
> +       case F_OWNER_TID:
> +               owner.pid = syscall(SYS_gettid);
> +               break;
> +       case F_OWNER_PGRP:
> +       default:
> +               return -1;
> +       }
> +
> +       for (cpu_map_idx = 0; cpu_map_idx < xyarray__max_x(evsel->fd); cpu_map_idx++) {
> +               err = perf_evsel__run_fcntl(evsel, F_SETFL, flags, cpu_map_idx);
> +               if (err)
> +                       return err;
> +
> +               err = perf_evsel__run_fcntl(evsel, F_SETSIG, signal, cpu_map_idx);
> +               if (err)
> +                       return err;
> +
> +               err = perf_evsel__run_fcntl(evsel, F_SETOWN_EX,
> +                                           (unsigned long)&owner, cpu_map_idx);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return err;
> +}
> +
>  int perf_evsel__open_opts(struct perf_evsel *evsel, struct perf_cpu_map *cpus,
>                           struct perf_thread_map *threads,
>                           struct perf_evsel_open_opts *opts)
> @@ -474,6 +547,12 @@ int perf_evsel__open_opts(struct perf_evsel *evsel, struct perf_cpu_map *cpus,
>         evsel->open_flags = OPTS_GET(opts, open_flags, 0);
>
>         err = perf_evsel__open(evsel, cpus, threads);
> +       if (err)
> +               return err;
> +
> +       err = perf_evsel__set_signal_handler(evsel, opts);
> +       if (err)
> +               return err;
>
>         return err;
>  }
> diff --git a/tools/lib/perf/include/perf/evsel.h b/tools/lib/perf/include/perf/evsel.h
> index 1140df4d2578..50662babfe97 100644
> --- a/tools/lib/perf/include/perf/evsel.h
> +++ b/tools/lib/perf/include/perf/evsel.h
> @@ -29,8 +29,12 @@ struct perf_evsel_open_opts {
>         size_t sz;
>
>         unsigned long open_flags;       /* perf_event_open flags */
> +       int flags;                      /* fcntl flags */
> +       unsigned int signal;
> +       int owner_type;
> +       struct sigaction *sig;
>  };
> -#define perf_evsel_open_opts__last_field open_flags
> +#define perf_evsel_open_opts__last_field sig
>
>  #define LIBPERF_OPTS(TYPE, NAME, ...)                  \
>         struct TYPE NAME = ({                           \
> diff --git a/tools/lib/perf/tests/test-evlist.c b/tools/lib/perf/tests/test-evlist.c
> index ed616fc19b4f..d8f9493cd4d1 100644
> --- a/tools/lib/perf/tests/test-evlist.c
> +++ b/tools/lib/perf/tests/test-evlist.c
> @@ -1,5 +1,4 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#define _GNU_SOURCE // needed for sched.h to get sched_[gs]etaffinity and CPU_(ZERO,SET)

Could be in a separate change.

Thanks,
Namhyung


>  #include <inttypes.h>
>  #include <sched.h>
>  #include <stdio.h>
> --
> 2.25.1
>