linux-kernel - Re: [RFC PATCH 0/7] libperf: Add overflow detection of sampling events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 26 Feb 2022 18:23:10 +0100
From:   Jiri Olsa <olsajiri@...il.com>
To:     Shunsuke <nakamura.shun@...itsu.com>
Cc:     peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
        mark.rutland@....com, alexander.shishkin@...ux.intel.com,
        jolsa@...hat.com, namhyung@...nel.org,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [RFC PATCH 0/7] libperf: Add overflow detection of sampling
 events

On Fri, Feb 25, 2022 at 07:31:07PM +0900, Shunsuke wrote:
> From: Shunsuke Nakamura <nakamura.shun@...itsu.com>
> 
> This patch series adds sampling event overflow detection capability
> to libperf.
> 
> First patch fixes a typo in the error message that I noticed.
> 
> Second patch  adds a interface to set PERF_FLAG_FD_CLOEXEC
> 
> Third patch adds a interface to perform IOC_REFRESH and IOC_PERIOD.
> 
> Fourth patch adds a interface to set the signal handler.
> 
> Fifth patch adds a interface to detect overflowed events.
> 
> Sixth and seventh patch adds tests.
> 
> Shunsuke Nakamura (7):
>   libperf tests: Fix typo in the error message "evsel" -> "evlist"
>   libperf: Add perf_evsel__set_close_on_exec() function
>   libperf: Add perf_evsel__refresh()/period() functions
>   libperf: Add perf_evsel__set_signal() functions
>   libperf: Add perf_evsel__check_fd() functions
>   libperf test: Add test_stat_overflow()
>   libperf test: Add test_detect_overflow_event()

hi,
I think the interface might be more clear if we use additional options
for new and open functions, same way like it's done in libbpf

how about adding following interface:

  - perf_evsel__new_opts would allow to setup needed attr fields before opening

    struct perf_evsel_new_opts {
      /* size of this struct, for forward/backward compatiblity */
      size_t sz;

      int open_flags;
      int sample_period;
    }
   
    struct perf_evsel *perf_evsel__new_opts(struct perf_event_attr *attr,
                                            struct perf_evsel_new_opts *opts);

  - perf_evlist__open_opts would do additional setup 'after' the event
    is open and we have file descriptor available


    struct perf_evsel_open_opts {
      /* size of this struct, for forward/backward compatiblity */
      size_t sz;

      int flags; /* fcntl flags */
      int signal;
      int owner_type;
    }

    int perf_evlist__open_opts(struct perf_evlist *evlist,
                               struct perf_evsel_open_opts *opts);)

    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)

    not sure we want special opts for evlist.. the evlist open
    is just a wrapper and I don't think there's special open
    info just for evlist

I did not analyze your usecases deeply, so I might be missing some
case where above API would need some adjustment, but from the quick
look this could fit and I think it's better than special functions
for each feature

there are additional macros like DECLARE_LIBBPF_OPTS that allows to
define the opts structures in backward compatible way, we'd need to
'borrow' that as well

thoughts?

thanks,
jirka