hi,
moving leader/group info to libperf's perf_evsel.
This was asked for by Shunsuke [1] and is on my list
as a prereq for event parsing move to libperf.
I still need to do more tests, but I'd like to check
with you guys if there's any feedback on this first.
Also available in:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
libperf/groups
v2 change:
- repost due to smtp failures, no changes
thanks,
jirka
[1] https://lore.kernel.org/linux-perf-users/OSBPR01MB46005B38568E90509946ECA9F7319@OSBPR01MB4600.jpnprd01.prod.outlook.com/
---
Jiri Olsa (7):
libperf: Change tests to single static and shared binaries
libperf: Move idx to perf_evsel::idx
libperf: Move leader to perf_evsel::leader
libperf: Move nr_groups to evlist::nr_groups
libperf: Add perf_evlist__set_leader function
libperF: Add group support to perf_evsel__open
libperf: Add tests for perf_evlist__set_leader function
tools/lib/perf/Build | 2 ++
tools/lib/perf/Makefile | 30 +++++++++++++++++++++++++-----
tools/lib/perf/evlist.c | 22 ++++++++++++++++++++++
tools/lib/perf/evsel.c | 33 +++++++++++++++++++++++++++++----
tools/lib/perf/include/internal/evlist.h | 2 ++
tools/lib/perf/include/internal/evsel.h | 5 ++++-
tools/lib/perf/include/internal/tests.h | 4 ++--
tools/lib/perf/include/perf/evlist.h | 1 +
tools/lib/perf/libperf.map | 1 +
tools/lib/perf/tests/Build | 5 +++++
tools/lib/perf/tests/Makefile | 40 ----------------------------------------
tools/lib/perf/tests/main.c | 15 +++++++++++++++
tools/lib/perf/tests/test-cpumap.c | 3 ++-
tools/lib/perf/tests/test-evlist.c | 30 +++++++++++++++++++++++-------
tools/lib/perf/tests/test-evsel.c | 3 ++-
tools/lib/perf/tests/test-threadmap.c | 3 ++-
tools/lib/perf/tests/tests.h | 10 ++++++++++
tools/perf/arch/x86/util/iostat.c | 4 ++--
tools/perf/builtin-diff.c | 4 ++--
tools/perf/builtin-record.c | 4 ++--
tools/perf/builtin-report.c | 8 ++++----
tools/perf/builtin-script.c | 9 +++++----
tools/perf/builtin-stat.c | 12 ++++++------
tools/perf/builtin-top.c | 10 +++++-----
tools/perf/tests/bpf.c | 2 +-
tools/perf/tests/evsel-roundtrip-name.c | 6 +++---
tools/perf/tests/mmap-basic.c | 8 ++++----
tools/perf/tests/parse-events.c | 74 +++++++++++++++++++++++++++++++++++++-------------------------------------
tools/perf/tests/pfm.c | 4 ++--
tools/perf/ui/browsers/annotate.c | 2 +-
tools/perf/util/annotate.c | 8 ++++----
tools/perf/util/auxtrace.c | 12 ++++++------
tools/perf/util/cgroup.c | 2 +-
tools/perf/util/evlist.c | 44 +++++++++++++-------------------------------
tools/perf/util/evlist.h | 2 --
tools/perf/util/evsel.c | 32 +++++++++++++++++++++++++-------
tools/perf/util/evsel.h | 14 ++++++++------
tools/perf/util/header.c | 18 +++++++++---------
tools/perf/util/metricgroup.c | 22 +++++++++++-----------
tools/perf/util/parse-events.c | 8 ++++----
tools/perf/util/pfm.c | 2 +-
tools/perf/util/python.c | 2 +-
tools/perf/util/record.c | 6 +++---
tools/perf/util/stat-shadow.c | 2 +-
tools/perf/util/stat.c | 2 +-
tools/perf/util/stream.c | 2 +-
46 files changed, 310 insertions(+), 224 deletions(-)
create mode 100644 tools/lib/perf/tests/Build
delete mode 100644 tools/lib/perf/tests/Makefile
create mode 100644 tools/lib/perf/tests/main.c
create mode 100644 tools/lib/perf/tests/tests.h
Adding support to set group_fd in perf_evsel__open
call and make it to follow the group setup.
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/lib/perf/evsel.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
index 3e6638d27c45..9ebf7122d476 100644
--- a/tools/lib/perf/evsel.c
+++ b/tools/lib/perf/evsel.c
@@ -17,6 +17,7 @@
#include <linux/string.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
+#include <asm/bug.h>
void perf_evsel__init(struct perf_evsel *evsel, struct perf_event_attr *attr,
int idx)
@@ -76,6 +77,25 @@ sys_perf_event_open(struct perf_event_attr *attr,
return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd, flags);
}
+static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread)
+{
+ struct perf_evsel *leader = evsel->leader;
+ int fd;
+
+ if (evsel == leader)
+ return -1;
+
+ /*
+ * Leader must be already processed/open,
+ * if not it's a bug.
+ */
+ BUG_ON(!leader->fd);
+
+ fd = FD(leader, cpu, thread);
+ BUG_ON(fd == -1);
+ return fd;
+}
+
int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *cpus,
struct perf_thread_map *threads)
{
@@ -111,11 +131,13 @@ int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *cpus,
for (cpu = 0; cpu < cpus->nr; cpu++) {
for (thread = 0; thread < threads->nr; thread++) {
- int fd;
+ int fd, group_fd;
+
+ group_fd = get_group_fd(evsel, cpu, thread);
fd = sys_perf_event_open(&evsel->attr,
threads->map[thread].pid,
- cpus->map[cpu], -1, 0);
+ cpus->map[cpu], group_fd, 0);
if (fd < 0)
return -errno;
--
2.31.1
Adding test for newly added perf_evlist__set_leader function.
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/lib/perf/tests/test-evlist.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/tools/lib/perf/tests/test-evlist.c b/tools/lib/perf/tests/test-evlist.c
index 7435529fb21c..c67c83399170 100644
--- a/tools/lib/perf/tests/test-evlist.c
+++ b/tools/lib/perf/tests/test-evlist.c
@@ -19,6 +19,7 @@
#include <internal/tests.h>
#include <api/fs/fs.h>
#include "tests.h"
+#include <internal/evsel.h>
static int libperf_print(enum libperf_print_level level,
const char *fmt, va_list ap)
@@ -30,7 +31,7 @@ static int test_stat_cpu(void)
{
struct perf_cpu_map *cpus;
struct perf_evlist *evlist;
- struct perf_evsel *evsel;
+ struct perf_evsel *evsel, *leader;
struct perf_event_attr attr1 = {
.type = PERF_TYPE_SOFTWARE,
.config = PERF_COUNT_SW_CPU_CLOCK,
@@ -47,7 +48,7 @@ static int test_stat_cpu(void)
evlist = perf_evlist__new();
__T("failed to create evlist", evlist);
- evsel = perf_evsel__new(&attr1);
+ evsel = leader = perf_evsel__new(&attr1);
__T("failed to create evsel1", evsel);
perf_evlist__add(evlist, evsel);
@@ -57,6 +58,10 @@ static int test_stat_cpu(void)
perf_evlist__add(evlist, evsel);
+ perf_evlist__set_leader(evlist);
+ __T("failed to set leader", leader->leader == leader);
+ __T("failed to set leader", evsel->leader == leader);
+
perf_evlist__set_maps(evlist, cpus, NULL);
err = perf_evlist__open(evlist);
@@ -85,7 +90,7 @@ static int test_stat_thread(void)
struct perf_counts_values counts = { .val = 0 };
struct perf_thread_map *threads;
struct perf_evlist *evlist;
- struct perf_evsel *evsel;
+ struct perf_evsel *evsel, *leader;
struct perf_event_attr attr1 = {
.type = PERF_TYPE_SOFTWARE,
.config = PERF_COUNT_SW_CPU_CLOCK,
@@ -104,7 +109,7 @@ static int test_stat_thread(void)
evlist = perf_evlist__new();
__T("failed to create evlist", evlist);
- evsel = perf_evsel__new(&attr1);
+ evsel = leader = perf_evsel__new(&attr1);
__T("failed to create evsel1", evsel);
perf_evlist__add(evlist, evsel);
@@ -114,6 +119,10 @@ static int test_stat_thread(void)
perf_evlist__add(evlist, evsel);
+ perf_evlist__set_leader(evlist);
+ __T("failed to set leader", leader->leader == leader);
+ __T("failed to set leader", evsel->leader == leader);
+
perf_evlist__set_maps(evlist, NULL, threads);
err = perf_evlist__open(evlist);
@@ -136,7 +145,7 @@ static int test_stat_thread_enable(void)
struct perf_counts_values counts = { .val = 0 };
struct perf_thread_map *threads;
struct perf_evlist *evlist;
- struct perf_evsel *evsel;
+ struct perf_evsel *evsel, *leader;
struct perf_event_attr attr1 = {
.type = PERF_TYPE_SOFTWARE,
.config = PERF_COUNT_SW_CPU_CLOCK,
@@ -157,7 +166,7 @@ static int test_stat_thread_enable(void)
evlist = perf_evlist__new();
__T("failed to create evlist", evlist);
- evsel = perf_evsel__new(&attr1);
+ evsel = leader = perf_evsel__new(&attr1);
__T("failed to create evsel1", evsel);
perf_evlist__add(evlist, evsel);
@@ -167,6 +176,10 @@ static int test_stat_thread_enable(void)
perf_evlist__add(evlist, evsel);
+ perf_evlist__set_leader(evlist);
+ __T("failed to set leader", leader->leader == leader);
+ __T("failed to set leader", evsel->leader == leader);
+
perf_evlist__set_maps(evlist, NULL, threads);
err = perf_evlist__open(evlist);
@@ -254,6 +267,7 @@ static int test_mmap_thread(void)
evsel = perf_evsel__new(&attr);
__T("failed to create evsel1", evsel);
+ __T("failed to set leader", evsel->leader == evsel);
perf_evlist__add(evlist, evsel);
@@ -339,6 +353,7 @@ static int test_mmap_cpus(void)
evsel = perf_evsel__new(&attr);
__T("failed to create evsel1", evsel);
+ __T("failed to set leader", evsel->leader == evsel);
perf_evlist__add(evlist, evsel);
--
2.31.1
Em Tue, Jul 06, 2021 at 05:16:57PM +0200, Jiri Olsa escreveu:
> hi,
> moving leader/group info to libperf's perf_evsel.
>
> This was asked for by Shunsuke [1] and is on my list
> as a prereq for event parsing move to libperf.
So I'll add a:
Requested-by: Shunsuke Nakamura <[email protected]>
- Arnaldo
> I still need to do more tests, but I'd like to check
> with you guys if there's any feedback on this first.
>
> Also available in:
> git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> libperf/groups
>
> v2 change:
> - repost due to smtp failures, no changes
>
> thanks,
> jirka
>
>
> [1] https://lore.kernel.org/linux-perf-users/OSBPR01MB46005B38568E90509946ECA9F7319@OSBPR01MB4600.jpnprd01.prod.outlook.com/
>
>
> ---
> Jiri Olsa (7):
> libperf: Change tests to single static and shared binaries
> libperf: Move idx to perf_evsel::idx
> libperf: Move leader to perf_evsel::leader
> libperf: Move nr_groups to evlist::nr_groups
> libperf: Add perf_evlist__set_leader function
> libperF: Add group support to perf_evsel__open
> libperf: Add tests for perf_evlist__set_leader function
>
> tools/lib/perf/Build | 2 ++
> tools/lib/perf/Makefile | 30 +++++++++++++++++++++++++-----
> tools/lib/perf/evlist.c | 22 ++++++++++++++++++++++
> tools/lib/perf/evsel.c | 33 +++++++++++++++++++++++++++++----
> tools/lib/perf/include/internal/evlist.h | 2 ++
> tools/lib/perf/include/internal/evsel.h | 5 ++++-
> tools/lib/perf/include/internal/tests.h | 4 ++--
> tools/lib/perf/include/perf/evlist.h | 1 +
> tools/lib/perf/libperf.map | 1 +
> tools/lib/perf/tests/Build | 5 +++++
> tools/lib/perf/tests/Makefile | 40 ----------------------------------------
> tools/lib/perf/tests/main.c | 15 +++++++++++++++
> tools/lib/perf/tests/test-cpumap.c | 3 ++-
> tools/lib/perf/tests/test-evlist.c | 30 +++++++++++++++++++++++-------
> tools/lib/perf/tests/test-evsel.c | 3 ++-
> tools/lib/perf/tests/test-threadmap.c | 3 ++-
> tools/lib/perf/tests/tests.h | 10 ++++++++++
> tools/perf/arch/x86/util/iostat.c | 4 ++--
> tools/perf/builtin-diff.c | 4 ++--
> tools/perf/builtin-record.c | 4 ++--
> tools/perf/builtin-report.c | 8 ++++----
> tools/perf/builtin-script.c | 9 +++++----
> tools/perf/builtin-stat.c | 12 ++++++------
> tools/perf/builtin-top.c | 10 +++++-----
> tools/perf/tests/bpf.c | 2 +-
> tools/perf/tests/evsel-roundtrip-name.c | 6 +++---
> tools/perf/tests/mmap-basic.c | 8 ++++----
> tools/perf/tests/parse-events.c | 74 +++++++++++++++++++++++++++++++++++++-------------------------------------
> tools/perf/tests/pfm.c | 4 ++--
> tools/perf/ui/browsers/annotate.c | 2 +-
> tools/perf/util/annotate.c | 8 ++++----
> tools/perf/util/auxtrace.c | 12 ++++++------
> tools/perf/util/cgroup.c | 2 +-
> tools/perf/util/evlist.c | 44 +++++++++++++-------------------------------
> tools/perf/util/evlist.h | 2 --
> tools/perf/util/evsel.c | 32 +++++++++++++++++++++++++-------
> tools/perf/util/evsel.h | 14 ++++++++------
> tools/perf/util/header.c | 18 +++++++++---------
> tools/perf/util/metricgroup.c | 22 +++++++++++-----------
> tools/perf/util/parse-events.c | 8 ++++----
> tools/perf/util/pfm.c | 2 +-
> tools/perf/util/python.c | 2 +-
> tools/perf/util/record.c | 6 +++---
> tools/perf/util/stat-shadow.c | 2 +-
> tools/perf/util/stat.c | 2 +-
> tools/perf/util/stream.c | 2 +-
> 46 files changed, 310 insertions(+), 224 deletions(-)
> create mode 100644 tools/lib/perf/tests/Build
> delete mode 100644 tools/lib/perf/tests/Makefile
> create mode 100644 tools/lib/perf/tests/main.c
> create mode 100644 tools/lib/perf/tests/tests.h
>
--
- Arnaldo
Em Tue, Jul 06, 2021 at 05:17:03PM +0200, Jiri Olsa escreveu:
> Adding support to set group_fd in perf_evsel__open
> call and make it to follow the group setup.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/lib/perf/evsel.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
> index 3e6638d27c45..9ebf7122d476 100644
> --- a/tools/lib/perf/evsel.c
> +++ b/tools/lib/perf/evsel.c
> @@ -17,6 +17,7 @@
> #include <linux/string.h>
> #include <sys/ioctl.h>
> #include <sys/mman.h>
> +#include <asm/bug.h>
>
> void perf_evsel__init(struct perf_evsel *evsel, struct perf_event_attr *attr,
> int idx)
> @@ -76,6 +77,25 @@ sys_perf_event_open(struct perf_event_attr *attr,
> return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd, flags);
> }
>
> +static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread)
> +{
> + struct perf_evsel *leader = evsel->leader;
> + int fd;
> +
> + if (evsel == leader)
> + return -1;
> +
> + /*
> + * Leader must be already processed/open,
> + * if not it's a bug.
> + */
> + BUG_ON(!leader->fd);
Humm, having panics in library code looks ugly, why can't we just return
some errno and let the whatever is using the library to fail gracefully?
I applied the patches so far, will make them available at tmp.perf/core
now.
- Arnaldo
> + fd = FD(leader, cpu, thread);
> + BUG_ON(fd == -1);
> + return fd;
> +}
> +
> int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *cpus,
> struct perf_thread_map *threads)
> {
> @@ -111,11 +131,13 @@ int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *cpus,
>
> for (cpu = 0; cpu < cpus->nr; cpu++) {
> for (thread = 0; thread < threads->nr; thread++) {
> - int fd;
> + int fd, group_fd;
> +
> + group_fd = get_group_fd(evsel, cpu, thread);
>
> fd = sys_perf_event_open(&evsel->attr,
> threads->map[thread].pid,
> - cpus->map[cpu], -1, 0);
> + cpus->map[cpu], group_fd, 0);
>
> if (fd < 0)
> return -errno;
> --
> 2.31.1
>
--
- Arnaldo
On Wed, Jul 07, 2021 at 11:47:23AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jul 06, 2021 at 05:16:57PM +0200, Jiri Olsa escreveu:
> > hi,
> > moving leader/group info to libperf's perf_evsel.
> >
> > This was asked for by Shunsuke [1] and is on my list
> > as a prereq for event parsing move to libperf.
>
> So I'll add a:
>
> Requested-by: Shunsuke Nakamura <[email protected]>
yep, but let's hear from him first if that's the case actualy ;-)
he did not mention any specific interface.. so I wonder
what we have is ok with him
thanks,
jirka
>
> - Arnaldo
>
> > I still need to do more tests, but I'd like to check
> > with you guys if there's any feedback on this first.
> >
> > Also available in:
> > git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > libperf/groups
> >
> > v2 change:
> > - repost due to smtp failures, no changes
> >
> > thanks,
> > jirka
> >
> >
> > [1] https://lore.kernel.org/linux-perf-users/OSBPR01MB46005B38568E90509946ECA9F7319@OSBPR01MB4600.jpnprd01.prod.outlook.com/
> >
> >
> > ---
> > Jiri Olsa (7):
> > libperf: Change tests to single static and shared binaries
> > libperf: Move idx to perf_evsel::idx
> > libperf: Move leader to perf_evsel::leader
> > libperf: Move nr_groups to evlist::nr_groups
> > libperf: Add perf_evlist__set_leader function
> > libperF: Add group support to perf_evsel__open
> > libperf: Add tests for perf_evlist__set_leader function
> >
> > tools/lib/perf/Build | 2 ++
> > tools/lib/perf/Makefile | 30 +++++++++++++++++++++++++-----
> > tools/lib/perf/evlist.c | 22 ++++++++++++++++++++++
> > tools/lib/perf/evsel.c | 33 +++++++++++++++++++++++++++++----
> > tools/lib/perf/include/internal/evlist.h | 2 ++
> > tools/lib/perf/include/internal/evsel.h | 5 ++++-
> > tools/lib/perf/include/internal/tests.h | 4 ++--
> > tools/lib/perf/include/perf/evlist.h | 1 +
> > tools/lib/perf/libperf.map | 1 +
> > tools/lib/perf/tests/Build | 5 +++++
> > tools/lib/perf/tests/Makefile | 40 ----------------------------------------
> > tools/lib/perf/tests/main.c | 15 +++++++++++++++
> > tools/lib/perf/tests/test-cpumap.c | 3 ++-
> > tools/lib/perf/tests/test-evlist.c | 30 +++++++++++++++++++++++-------
> > tools/lib/perf/tests/test-evsel.c | 3 ++-
> > tools/lib/perf/tests/test-threadmap.c | 3 ++-
> > tools/lib/perf/tests/tests.h | 10 ++++++++++
> > tools/perf/arch/x86/util/iostat.c | 4 ++--
> > tools/perf/builtin-diff.c | 4 ++--
> > tools/perf/builtin-record.c | 4 ++--
> > tools/perf/builtin-report.c | 8 ++++----
> > tools/perf/builtin-script.c | 9 +++++----
> > tools/perf/builtin-stat.c | 12 ++++++------
> > tools/perf/builtin-top.c | 10 +++++-----
> > tools/perf/tests/bpf.c | 2 +-
> > tools/perf/tests/evsel-roundtrip-name.c | 6 +++---
> > tools/perf/tests/mmap-basic.c | 8 ++++----
> > tools/perf/tests/parse-events.c | 74 +++++++++++++++++++++++++++++++++++++-------------------------------------
> > tools/perf/tests/pfm.c | 4 ++--
> > tools/perf/ui/browsers/annotate.c | 2 +-
> > tools/perf/util/annotate.c | 8 ++++----
> > tools/perf/util/auxtrace.c | 12 ++++++------
> > tools/perf/util/cgroup.c | 2 +-
> > tools/perf/util/evlist.c | 44 +++++++++++++-------------------------------
> > tools/perf/util/evlist.h | 2 --
> > tools/perf/util/evsel.c | 32 +++++++++++++++++++++++++-------
> > tools/perf/util/evsel.h | 14 ++++++++------
> > tools/perf/util/header.c | 18 +++++++++---------
> > tools/perf/util/metricgroup.c | 22 +++++++++++-----------
> > tools/perf/util/parse-events.c | 8 ++++----
> > tools/perf/util/pfm.c | 2 +-
> > tools/perf/util/python.c | 2 +-
> > tools/perf/util/record.c | 6 +++---
> > tools/perf/util/stat-shadow.c | 2 +-
> > tools/perf/util/stat.c | 2 +-
> > tools/perf/util/stream.c | 2 +-
> > 46 files changed, 310 insertions(+), 224 deletions(-)
> > create mode 100644 tools/lib/perf/tests/Build
> > delete mode 100644 tools/lib/perf/tests/Makefile
> > create mode 100644 tools/lib/perf/tests/main.c
> > create mode 100644 tools/lib/perf/tests/tests.h
> >
>
> --
>
> - Arnaldo
>
On Wed, Jul 07, 2021 at 02:43:07PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jul 06, 2021 at 05:17:03PM +0200, Jiri Olsa escreveu:
> > Adding support to set group_fd in perf_evsel__open
> > call and make it to follow the group setup.
> >
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > tools/lib/perf/evsel.c | 26 ++++++++++++++++++++++++--
> > 1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
> > index 3e6638d27c45..9ebf7122d476 100644
> > --- a/tools/lib/perf/evsel.c
> > +++ b/tools/lib/perf/evsel.c
> > @@ -17,6 +17,7 @@
> > #include <linux/string.h>
> > #include <sys/ioctl.h>
> > #include <sys/mman.h>
> > +#include <asm/bug.h>
> >
> > void perf_evsel__init(struct perf_evsel *evsel, struct perf_event_attr *attr,
> > int idx)
> > @@ -76,6 +77,25 @@ sys_perf_event_open(struct perf_event_attr *attr,
> > return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd, flags);
> > }
> >
> > +static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread)
> > +{
> > + struct perf_evsel *leader = evsel->leader;
> > + int fd;
> > +
> > + if (evsel == leader)
> > + return -1;
> > +
> > + /*
> > + * Leader must be already processed/open,
> > + * if not it's a bug.
> > + */
> > + BUG_ON(!leader->fd);
>
> Humm, having panics in library code looks ugly, why can't we just return
> some errno and let the whatever is using the library to fail gracefully?
true, I took it from perf code, did not realize this,
I'll check what can we do in ehre
>
> I applied the patches so far, will make them available at tmp.perf/core
> now.
great, I'll take the patches from there for my next change
thanks,
jirka
>
> - Arnaldo
>
> > + fd = FD(leader, cpu, thread);
> > + BUG_ON(fd == -1);
> > + return fd;
> > +}
> > +
> > int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *cpus,
> > struct perf_thread_map *threads)
> > {
> > @@ -111,11 +131,13 @@ int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *cpus,
> >
> > for (cpu = 0; cpu < cpus->nr; cpu++) {
> > for (thread = 0; thread < threads->nr; thread++) {
> > - int fd;
> > + int fd, group_fd;
> > +
> > + group_fd = get_group_fd(evsel, cpu, thread);
> >
> > fd = sys_perf_event_open(&evsel->attr,
> > threads->map[thread].pid,
> > - cpus->map[cpu], -1, 0);
> > + cpus->map[cpu], group_fd, 0);
> >
> > if (fd < 0)
> > return -errno;
> > --
> > 2.31.1
> >
>
> --
>
> - Arnaldo
>
Em Wed, Jul 07, 2021 at 08:20:04PM +0200, Jiri Olsa escreveu:
> On Wed, Jul 07, 2021 at 11:47:23AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jul 06, 2021 at 05:16:57PM +0200, Jiri Olsa escreveu:
> > > hi,
> > > moving leader/group info to libperf's perf_evsel.
> > >
> > > This was asked for by Shunsuke [1] and is on my list
> > > as a prereq for event parsing move to libperf.
> >
> > So I'll add a:
> >
> > Requested-by: Shunsuke Nakamura <[email protected]>
>
> yep, but let's hear from him first if that's the case actualy ;-)
>
> he did not mention any specific interface.. so I wonder
> what we have is ok with him
Its all in tmp.perf/core, but from what I saw, yeah, he requested being
able to have a group leader.
Nakamura-san, isn't that the case?
- Arnaldo
> thanks,
> jirka
>
> >
> > - Arnaldo
> >
> > > I still need to do more tests, but I'd like to check
> > > with you guys if there's any feedback on this first.
> > >
> > > Also available in:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > > libperf/groups
> > >
> > > v2 change:
> > > - repost due to smtp failures, no changes
> > >
> > > thanks,
> > > jirka
> > >
> > >
> > > [1] https://lore.kernel.org/linux-perf-users/OSBPR01MB46005B38568E90509946ECA9F7319@OSBPR01MB4600.jpnprd01.prod.outlook.com/
> > >
> > >
> > > ---
> > > Jiri Olsa (7):
> > > libperf: Change tests to single static and shared binaries
> > > libperf: Move idx to perf_evsel::idx
> > > libperf: Move leader to perf_evsel::leader
> > > libperf: Move nr_groups to evlist::nr_groups
> > > libperf: Add perf_evlist__set_leader function
> > > libperF: Add group support to perf_evsel__open
> > > libperf: Add tests for perf_evlist__set_leader function
> > >
> > > tools/lib/perf/Build | 2 ++
> > > tools/lib/perf/Makefile | 30 +++++++++++++++++++++++++-----
> > > tools/lib/perf/evlist.c | 22 ++++++++++++++++++++++
> > > tools/lib/perf/evsel.c | 33 +++++++++++++++++++++++++++++----
> > > tools/lib/perf/include/internal/evlist.h | 2 ++
> > > tools/lib/perf/include/internal/evsel.h | 5 ++++-
> > > tools/lib/perf/include/internal/tests.h | 4 ++--
> > > tools/lib/perf/include/perf/evlist.h | 1 +
> > > tools/lib/perf/libperf.map | 1 +
> > > tools/lib/perf/tests/Build | 5 +++++
> > > tools/lib/perf/tests/Makefile | 40 ----------------------------------------
> > > tools/lib/perf/tests/main.c | 15 +++++++++++++++
> > > tools/lib/perf/tests/test-cpumap.c | 3 ++-
> > > tools/lib/perf/tests/test-evlist.c | 30 +++++++++++++++++++++++-------
> > > tools/lib/perf/tests/test-evsel.c | 3 ++-
> > > tools/lib/perf/tests/test-threadmap.c | 3 ++-
> > > tools/lib/perf/tests/tests.h | 10 ++++++++++
> > > tools/perf/arch/x86/util/iostat.c | 4 ++--
> > > tools/perf/builtin-diff.c | 4 ++--
> > > tools/perf/builtin-record.c | 4 ++--
> > > tools/perf/builtin-report.c | 8 ++++----
> > > tools/perf/builtin-script.c | 9 +++++----
> > > tools/perf/builtin-stat.c | 12 ++++++------
> > > tools/perf/builtin-top.c | 10 +++++-----
> > > tools/perf/tests/bpf.c | 2 +-
> > > tools/perf/tests/evsel-roundtrip-name.c | 6 +++---
> > > tools/perf/tests/mmap-basic.c | 8 ++++----
> > > tools/perf/tests/parse-events.c | 74 +++++++++++++++++++++++++++++++++++++-------------------------------------
> > > tools/perf/tests/pfm.c | 4 ++--
> > > tools/perf/ui/browsers/annotate.c | 2 +-
> > > tools/perf/util/annotate.c | 8 ++++----
> > > tools/perf/util/auxtrace.c | 12 ++++++------
> > > tools/perf/util/cgroup.c | 2 +-
> > > tools/perf/util/evlist.c | 44 +++++++++++++-------------------------------
> > > tools/perf/util/evlist.h | 2 --
> > > tools/perf/util/evsel.c | 32 +++++++++++++++++++++++++-------
> > > tools/perf/util/evsel.h | 14 ++++++++------
> > > tools/perf/util/header.c | 18 +++++++++---------
> > > tools/perf/util/metricgroup.c | 22 +++++++++++-----------
> > > tools/perf/util/parse-events.c | 8 ++++----
> > > tools/perf/util/pfm.c | 2 +-
> > > tools/perf/util/python.c | 2 +-
> > > tools/perf/util/record.c | 6 +++---
> > > tools/perf/util/stat-shadow.c | 2 +-
> > > tools/perf/util/stat.c | 2 +-
> > > tools/perf/util/stream.c | 2 +-
> > > 46 files changed, 310 insertions(+), 224 deletions(-)
> > > create mode 100644 tools/lib/perf/tests/Build
> > > delete mode 100644 tools/lib/perf/tests/Makefile
> > > create mode 100644 tools/lib/perf/tests/main.c
> > > create mode 100644 tools/lib/perf/tests/tests.h
> > >
> >
> > --
> >
> > - Arnaldo
> >
>
--
- Arnaldo
Hi Jiri, Arnaldo
Thank you for posting a patch.
> Its all in tmp.perf/core, but from what I saw, yeah, he requested being
> able to have a group leader.
>
> Nakamura-san, isn't that the case?
Yes, that's right.
With these patches, I think we can use event group to register multiple
perf events, just like PAPI.
If I find any problems, I will point them out.
Best Regards
Shunsuke
Em Wed, Jul 07, 2021 at 08:15:38PM +0200, Jiri Olsa escreveu:
> On Wed, Jul 07, 2021 at 02:43:07PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jul 06, 2021 at 05:17:03PM +0200, Jiri Olsa escreveu:
> > > +static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread)
> > > +{
> > > + struct perf_evsel *leader = evsel->leader;
> > > + int fd;
> > > +
> > > + if (evsel == leader)
> > > + return -1;
> > > +
> > > + /*
> > > + * Leader must be already processed/open,
> > > + * if not it's a bug.
> > > + */
> > > + BUG_ON(!leader->fd);
> > Humm, having panics in library code looks ugly, why can't we just return
> > some errno and let the whatever is using the library to fail gracefully?
> true, I took it from perf code, did not realize this,
> I'll check what can we do in here
So, I've added this as a follow up patch:
commit 0ec138125eaea5f15157adcecc3e0def1ad2ed22
Author: Arnaldo Carvalho de Melo <[email protected]>
Date: Fri Jul 9 14:52:16 2021 -0300
libperf: Remove BUG_ON() from library code in get_group_fd()
We shouldn't just panic, return a value that doesn't clash with what
perf_evsel__open() was already returning in case of error, i.e. errno
when sys_perf_event_open() fails.
Cc: Alexander Shishkin <[email protected]>
Cc: Ian Rogers <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Michael Petlan <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Shunsuke Nakamura <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
index 9ebf7122d4766c5e..d8886720e83d8dfe 100644
--- a/tools/lib/perf/evsel.c
+++ b/tools/lib/perf/evsel.c
@@ -77,23 +77,30 @@ sys_perf_event_open(struct perf_event_attr *attr,
return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd, flags);
}
-static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread)
+static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread, int *group_fd)
{
struct perf_evsel *leader = evsel->leader;
int fd;
- if (evsel == leader)
- return -1;
+ if (evsel == leader) {
+ *group_fd = -1;
+ return 0;
+ }
/*
* Leader must be already processed/open,
* if not it's a bug.
*/
- BUG_ON(!leader->fd);
+ if (!leader->fd)
+ return -ENOTCONN;
fd = FD(leader, cpu, thread);
- BUG_ON(fd == -1);
- return fd;
+ if (fd == -1)
+ return -EBADF;
+
+ *group_fd = fd;
+
+ return 0;
}
int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *cpus,
@@ -133,7 +140,9 @@ int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *cpus,
for (thread = 0; thread < threads->nr; thread++) {
int fd, group_fd;
- group_fd = get_group_fd(evsel, cpu, thread);
+ err = get_group_fd(evsel, cpu, thread, &group_fd);
+ if (err < 0)
+ return err;
fd = sys_perf_event_open(&evsel->attr,
threads->map[thread].pid,
Em Tue, Jul 06, 2021 at 05:17:04PM +0200, Jiri Olsa escreveu:
> Adding test for newly added perf_evlist__set_leader function.
Thanks, applied.
- Arnaldo
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/lib/perf/tests/test-evlist.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/tools/lib/perf/tests/test-evlist.c b/tools/lib/perf/tests/test-evlist.c
> index 7435529fb21c..c67c83399170 100644
> --- a/tools/lib/perf/tests/test-evlist.c
> +++ b/tools/lib/perf/tests/test-evlist.c
> @@ -19,6 +19,7 @@
> #include <internal/tests.h>
> #include <api/fs/fs.h>
> #include "tests.h"
> +#include <internal/evsel.h>
>
> static int libperf_print(enum libperf_print_level level,
> const char *fmt, va_list ap)
> @@ -30,7 +31,7 @@ static int test_stat_cpu(void)
> {
> struct perf_cpu_map *cpus;
> struct perf_evlist *evlist;
> - struct perf_evsel *evsel;
> + struct perf_evsel *evsel, *leader;
> struct perf_event_attr attr1 = {
> .type = PERF_TYPE_SOFTWARE,
> .config = PERF_COUNT_SW_CPU_CLOCK,
> @@ -47,7 +48,7 @@ static int test_stat_cpu(void)
> evlist = perf_evlist__new();
> __T("failed to create evlist", evlist);
>
> - evsel = perf_evsel__new(&attr1);
> + evsel = leader = perf_evsel__new(&attr1);
> __T("failed to create evsel1", evsel);
>
> perf_evlist__add(evlist, evsel);
> @@ -57,6 +58,10 @@ static int test_stat_cpu(void)
>
> perf_evlist__add(evlist, evsel);
>
> + perf_evlist__set_leader(evlist);
> + __T("failed to set leader", leader->leader == leader);
> + __T("failed to set leader", evsel->leader == leader);
> +
> perf_evlist__set_maps(evlist, cpus, NULL);
>
> err = perf_evlist__open(evlist);
> @@ -85,7 +90,7 @@ static int test_stat_thread(void)
> struct perf_counts_values counts = { .val = 0 };
> struct perf_thread_map *threads;
> struct perf_evlist *evlist;
> - struct perf_evsel *evsel;
> + struct perf_evsel *evsel, *leader;
> struct perf_event_attr attr1 = {
> .type = PERF_TYPE_SOFTWARE,
> .config = PERF_COUNT_SW_CPU_CLOCK,
> @@ -104,7 +109,7 @@ static int test_stat_thread(void)
> evlist = perf_evlist__new();
> __T("failed to create evlist", evlist);
>
> - evsel = perf_evsel__new(&attr1);
> + evsel = leader = perf_evsel__new(&attr1);
> __T("failed to create evsel1", evsel);
>
> perf_evlist__add(evlist, evsel);
> @@ -114,6 +119,10 @@ static int test_stat_thread(void)
>
> perf_evlist__add(evlist, evsel);
>
> + perf_evlist__set_leader(evlist);
> + __T("failed to set leader", leader->leader == leader);
> + __T("failed to set leader", evsel->leader == leader);
> +
> perf_evlist__set_maps(evlist, NULL, threads);
>
> err = perf_evlist__open(evlist);
> @@ -136,7 +145,7 @@ static int test_stat_thread_enable(void)
> struct perf_counts_values counts = { .val = 0 };
> struct perf_thread_map *threads;
> struct perf_evlist *evlist;
> - struct perf_evsel *evsel;
> + struct perf_evsel *evsel, *leader;
> struct perf_event_attr attr1 = {
> .type = PERF_TYPE_SOFTWARE,
> .config = PERF_COUNT_SW_CPU_CLOCK,
> @@ -157,7 +166,7 @@ static int test_stat_thread_enable(void)
> evlist = perf_evlist__new();
> __T("failed to create evlist", evlist);
>
> - evsel = perf_evsel__new(&attr1);
> + evsel = leader = perf_evsel__new(&attr1);
> __T("failed to create evsel1", evsel);
>
> perf_evlist__add(evlist, evsel);
> @@ -167,6 +176,10 @@ static int test_stat_thread_enable(void)
>
> perf_evlist__add(evlist, evsel);
>
> + perf_evlist__set_leader(evlist);
> + __T("failed to set leader", leader->leader == leader);
> + __T("failed to set leader", evsel->leader == leader);
> +
> perf_evlist__set_maps(evlist, NULL, threads);
>
> err = perf_evlist__open(evlist);
> @@ -254,6 +267,7 @@ static int test_mmap_thread(void)
>
> evsel = perf_evsel__new(&attr);
> __T("failed to create evsel1", evsel);
> + __T("failed to set leader", evsel->leader == evsel);
>
> perf_evlist__add(evlist, evsel);
>
> @@ -339,6 +353,7 @@ static int test_mmap_cpus(void)
>
> evsel = perf_evsel__new(&attr);
> __T("failed to create evsel1", evsel);
> + __T("failed to set leader", evsel->leader == evsel);
>
> perf_evlist__add(evlist, evsel);
>
> --
> 2.31.1
>
--
- Arnaldo