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] [day] [month] [year] [list]

Message-ID: <OSBPR01MB4600BC13514B3D5326FD6F93F7029@OSBPR01MB4600.jpnprd01.prod.outlook.com>
Date:   Tue, 1 Mar 2022 08:32:59 +0000
From:   "nakamura.shun@...itsu.com" <nakamura.shun@...itsu.com>
To:     'Jiri Olsa' <olsajiri@...il.com>
CC:     "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "acme@...nel.org" <acme@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "alexander.shishkin@...ux.intel.com" 
        <alexander.shishkin@...ux.intel.com>,
        "jolsa@...hat.com" <jolsa@...hat.com>,
        "namhyung@...nel.org" <namhyung@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>
Subject: RE: [RFC PATCH 0/7] libperf: Add overflow detection of sampling
 events

Hi jirka

> 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?

Thank you for your comment. I'll consider about it.

Best Regards
Shunsuke