[RFC PATCHSET] Implement --switch-output-events

[RFC PATCHSET] Implement --switch-output-events

[Arnaldo Carvalho de Melo]

Hi guys,

	Please take a look, the example provided is too simple, using
'perf probe' to put probes in specific places in some workload to then
get any other event close to the time the trigger hits comes to mind as
well, using the signal was just to reuse the pre-existing logic and keep
the patchkit small.

	Its available at:

  git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/switch-output-event

Best regards,

- Arnaldo

Arnaldo Carvalho de Melo (7):
  perf record: Move sb_evlist to 'struct record'
  perf top: Move sb_evlist to 'struct perf_top'
  perf bpf: Decouple creating the evlist from adding the SB event
  perf bpf: Decouple creating the evlist from adding the SB event
  perf parse-events: Add parse_events_option() variant that creates
    evlist
  perf evlist: Allow reusing the side band thread for more purposes
  perf record: Introduce --switch-output-event

 tools/perf/Documentation/perf-record.txt | 13 ++++++
 tools/perf/builtin-record.c              | 55 +++++++++++++++++++++---
 tools/perf/builtin-top.c                 | 20 ++++++---
 tools/perf/util/bpf-event.c              |  3 +-
 tools/perf/util/bpf-event.h              |  7 ++-
 tools/perf/util/evlist.c                 | 39 +++++++++++------
 tools/perf/util/evlist.h                 |  3 +-
 tools/perf/util/parse-events.c           | 23 ++++++++++
 tools/perf/util/parse-events.h           |  1 +
 tools/perf/util/top.h                    |  2 +-
 10 files changed, 133 insertions(+), 33 deletions(-)

-- 
2.21.1

[PATCH 1/7] perf record: Move sb_evlist to 'struct record'

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Where state related to a 'perf record' session is grouped.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2e8011f179f2..2348c4205e59 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -86,6 +86,7 @@ struct record {
 	struct auxtrace_record	*itr;
 	struct evlist	*evlist;
 	struct perf_session	*session;
+	struct evlist		*sb_evlist;
 	int			realtime_prio;
 	bool			no_buildid;
 	bool			no_buildid_set;
@@ -1446,7 +1447,6 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	struct perf_data *data = &rec->data;
 	struct perf_session *session;
 	bool disabled = false, draining = false;
-	struct evlist *sb_evlist = NULL;
 	int fd;
 	float ratio = 0;
 
@@ -1581,9 +1581,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	}
 
 	if (!opts->no_bpf_event)
-		bpf_event__add_sb_event(&sb_evlist, &session->header.env);
+		bpf_event__add_sb_event(&rec->sb_evlist, &session->header.env);
 
-	if (perf_evlist__start_sb_thread(sb_evlist, &rec->opts.target)) {
+	if (perf_evlist__start_sb_thread(rec->sb_evlist, &rec->opts.target)) {
 		pr_debug("Couldn't start the BPF side band thread:\nBPF programs starting from now on won't be annotatable\n");
 		opts->no_bpf_event = true;
 	}
@@ -1857,7 +1857,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	perf_session__delete(session);
 
 	if (!opts->no_bpf_event)
-		perf_evlist__stop_sb_thread(sb_evlist);
+		perf_evlist__stop_sb_thread(rec->sb_evlist);
 	return status;
 }
 
-- 
2.21.1

[PATCH 2/7] perf top: Move sb_evlist to 'struct perf_top'

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Where state related to a 'perf top' session is grouped.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c | 7 +++----
 tools/perf/util/top.h    | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 6b067a5ba1d5..70e1c732db6a 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1580,7 +1580,6 @@ int cmd_top(int argc, const char **argv)
 	OPTS_EVSWITCH(&top.evswitch),
 	OPT_END()
 	};
-	struct evlist *sb_evlist = NULL;
 	const char * const top_usage[] = {
 		"perf top [<options>]",
 		NULL
@@ -1744,9 +1743,9 @@ int cmd_top(int argc, const char **argv)
 	}
 
 	if (!top.record_opts.no_bpf_event)
-		bpf_event__add_sb_event(&sb_evlist, &perf_env);
+		bpf_event__add_sb_event(&top.sb_evlist, &perf_env);
 
-	if (perf_evlist__start_sb_thread(sb_evlist, target)) {
+	if (perf_evlist__start_sb_thread(top.sb_evlist, target)) {
 		pr_debug("Couldn't start the BPF side band thread:\nBPF programs starting from now on won't be annotatable\n");
 		opts->no_bpf_event = true;
 	}
@@ -1754,7 +1753,7 @@ int cmd_top(int argc, const char **argv)
 	status = __cmd_top(&top);
 
 	if (!opts->no_bpf_event)
-		perf_evlist__stop_sb_thread(sb_evlist);
+		perf_evlist__stop_sb_thread(top.sb_evlist);
 
 out_delete_evlist:
 	evlist__delete(top.evlist);
diff --git a/tools/perf/util/top.h b/tools/perf/util/top.h
index 45dc84ddff37..ff8391208ecd 100644
--- a/tools/perf/util/top.h
+++ b/tools/perf/util/top.h
@@ -18,7 +18,7 @@ struct perf_session;
 
 struct perf_top {
 	struct perf_tool   tool;
-	struct evlist *evlist;
+	struct evlist *evlist, *sb_evlist;
 	struct record_opts record_opts;
 	struct annotation_options annotation_opts;
 	struct evswitch	   evswitch;
-- 
2.21.1

[PATCH 3/7] perf bpf: Decouple creating the evlist from adding the SB event

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Renaming bpf_event__add_sb_event() to evlist__add_sb_event() and
requiring that the evlist be allocated beforehand.

This will allow using the same side band thread and evlist to be used
for multiple purposes in addition to react to PERF_RECORD_BPF_EVENT soon
after they are generated.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 17 ++++++++++++++---
 tools/perf/builtin-top.c    | 15 +++++++++++++--
 tools/perf/util/bpf-event.c |  3 +--
 tools/perf/util/bpf-event.h |  7 +++----
 tools/perf/util/evlist.c    | 17 +++--------------
 tools/perf/util/evlist.h    |  2 +-
 6 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2348c4205e59..ed2244847400 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1572,16 +1572,27 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 			goto out_child;
 	}
 
+	err = -1;
 	if (!rec->no_buildid
 	    && !perf_header__has_feat(&session->header, HEADER_BUILD_ID)) {
 		pr_err("Couldn't generate buildids. "
 		       "Use --no-buildid to profile anyway.\n");
-		err = -1;
 		goto out_child;
 	}
 
-	if (!opts->no_bpf_event)
-		bpf_event__add_sb_event(&rec->sb_evlist, &session->header.env);
+	if (!opts->no_bpf_event) {
+		rec->sb_evlist = evlist__new();
+
+		if (rec->sb_evlist == NULL) {
+			pr_err("Couldn't create side band evlist.\n.");
+			goto out_child;
+		}
+
+		if (evlist__add_bpf_sb_event(rec->sb_evlist, &session->header.env)) {
+			pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
+			goto out_child;
+		}
+	}
 
 	if (perf_evlist__start_sb_thread(rec->sb_evlist, &rec->opts.target)) {
 		pr_debug("Couldn't start the BPF side band thread:\nBPF programs starting from now on won't be annotatable\n");
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 70e1c732db6a..de24aced7213 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1742,8 +1742,19 @@ int cmd_top(int argc, const char **argv)
 		goto out_delete_evlist;
 	}
 
-	if (!top.record_opts.no_bpf_event)
-		bpf_event__add_sb_event(&top.sb_evlist, &perf_env);
+	if (!top.record_opts.no_bpf_event) {
+		top.sb_evlist = evlist__new();
+
+		if (top.sb_evlist == NULL) {
+			pr_err("Couldn't create side band evlist.\n.");
+			goto out_delete_evlist;
+		}
+
+		if (evlist__add_bpf_sb_event(top.sb_evlist, &perf_env)) {
+			pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
+			goto out_delete_evlist;
+		}
+	}
 
 	if (perf_evlist__start_sb_thread(top.sb_evlist, target)) {
 		pr_debug("Couldn't start the BPF side band thread:\nBPF programs starting from now on won't be annotatable\n");
diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
index 0cd41a862952..3742511a08d1 100644
--- a/tools/perf/util/bpf-event.c
+++ b/tools/perf/util/bpf-event.c
@@ -509,8 +509,7 @@ static int bpf_event__sb_cb(union perf_event *event, void *data)
 	return 0;
 }
 
-int bpf_event__add_sb_event(struct evlist **evlist,
-			    struct perf_env *env)
+int evlist__add_bpf_sb_event(struct evlist *evlist, struct perf_env *env)
 {
 	struct perf_event_attr attr = {
 		.type	          = PERF_TYPE_SOFTWARE,
diff --git a/tools/perf/util/bpf-event.h b/tools/perf/util/bpf-event.h
index 81fdc88e6c1a..2c7a50509659 100644
--- a/tools/perf/util/bpf-event.h
+++ b/tools/perf/util/bpf-event.h
@@ -33,8 +33,7 @@ struct btf_node {
 #ifdef HAVE_LIBBPF_SUPPORT
 int machine__process_bpf(struct machine *machine, union perf_event *event,
 			 struct perf_sample *sample);
-int bpf_event__add_sb_event(struct evlist **evlist,
-				 struct perf_env *env);
+int evlist__add_bpf_sb_event(struct evlist *evlist, struct perf_env *env);
 void bpf_event__print_bpf_prog_info(struct bpf_prog_info *info,
 				    struct perf_env *env,
 				    FILE *fp);
@@ -46,8 +45,8 @@ static inline int machine__process_bpf(struct machine *machine __maybe_unused,
 	return 0;
 }
 
-static inline int bpf_event__add_sb_event(struct evlist **evlist __maybe_unused,
-					  struct perf_env *env __maybe_unused)
+static inline int evlist__add_bpf_sb_event(struct evlist **evlist __maybe_unused,
+					   struct perf_env *env __maybe_unused)
 {
 	return 0;
 }
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 82d9f9bb8975..1d0d36da223b 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1704,38 +1704,27 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
 	return leader;
 }
 
-int perf_evlist__add_sb_event(struct evlist **evlist,
+int perf_evlist__add_sb_event(struct evlist *evlist,
 			      struct perf_event_attr *attr,
 			      perf_evsel__sb_cb_t cb,
 			      void *data)
 {
 	struct evsel *evsel;
-	bool new_evlist = (*evlist) == NULL;
-
-	if (*evlist == NULL)
-		*evlist = evlist__new();
-	if (*evlist == NULL)
-		return -1;
 
 	if (!attr->sample_id_all) {
 		pr_warning("enabling sample_id_all for all side band events\n");
 		attr->sample_id_all = 1;
 	}
 
-	evsel = perf_evsel__new_idx(attr, (*evlist)->core.nr_entries);
+	evsel = perf_evsel__new_idx(attr, evlist->core.nr_entries);
 	if (!evsel)
 		goto out_err;
 
 	evsel->side_band.cb = cb;
 	evsel->side_band.data = data;
-	evlist__add(*evlist, evsel);
+	evlist__add(evlist, evsel);
 	return 0;
-
 out_err:
-	if (new_evlist) {
-		evlist__delete(*evlist);
-		*evlist = NULL;
-	}
 	return -1;
 }
 
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index f5bd5c386df1..0f02408fff3e 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -107,7 +107,7 @@ int __perf_evlist__add_default_attrs(struct evlist *evlist,
 
 int perf_evlist__add_dummy(struct evlist *evlist);
 
-int perf_evlist__add_sb_event(struct evlist **evlist,
+int perf_evlist__add_sb_event(struct evlist *evlist,
 			      struct perf_event_attr *attr,
 			      perf_evsel__sb_cb_t cb,
 			      void *data);
-- 
2.21.1

[PATCH 4/7] perf bpf: Decouple creating the evlist from adding the SB event

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Renaming bpf_event__add_sb_event() to evlist__add_sb_event() and
requiring that the evlist be allocated beforehand.

This will allow using the same side band thread and evlist to be used
for multiple purposes in addition to react to PERF_RECORD_BPF_EVENT soon
after they are generated.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/bpf-event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/bpf-event.h b/tools/perf/util/bpf-event.h
index 2c7a50509659..68f315c3df5b 100644
--- a/tools/perf/util/bpf-event.h
+++ b/tools/perf/util/bpf-event.h
@@ -45,7 +45,7 @@ static inline int machine__process_bpf(struct machine *machine __maybe_unused,
 	return 0;
 }
 
-static inline int evlist__add_bpf_sb_event(struct evlist **evlist __maybe_unused,
+static inline int evlist__add_bpf_sb_event(struct evlist *evlist __maybe_unused,
 					   struct perf_env *env __maybe_unused)
 {
 	return 0;
-- 
2.21.1

Re: [PATCH 4/7] perf bpf: Decouple creating the evlist from adding the SB event

[Jiri Olsa]

On Mon, Apr 27, 2020 at 06:19:32PM -0300, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Renaming bpf_event__add_sb_event() to evlist__add_sb_event() and
> requiring that the evlist be allocated beforehand.

hum, this seems to be done in previous patch, maybe you
need to squash this with the previous one?

jirka

> 
> This will allow using the same side band thread and evlist to be used
> for multiple purposes in addition to react to PERF_RECORD_BPF_EVENT soon
> after they are generated.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Song Liu <songliubraving@fb.com>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/bpf-event.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/bpf-event.h b/tools/perf/util/bpf-event.h
> index 2c7a50509659..68f315c3df5b 100644
> --- a/tools/perf/util/bpf-event.h
> +++ b/tools/perf/util/bpf-event.h
> @@ -45,7 +45,7 @@ static inline int machine__process_bpf(struct machine *machine __maybe_unused,
>  	return 0;
>  }
>  
> -static inline int evlist__add_bpf_sb_event(struct evlist **evlist __maybe_unused,
> +static inline int evlist__add_bpf_sb_event(struct evlist *evlist __maybe_unused,
>  					   struct perf_env *env __maybe_unused)
>  {
>  	return 0;
> -- 
> 2.21.1
> 

Re: [PATCH 4/7] perf bpf: Decouple creating the evlist from adding the SB event

[Arnaldo Carvalho de Melo]

Em Tue, Apr 28, 2020 at 11:48:51AM +0200, Jiri Olsa escreveu:
> On Mon, Apr 27, 2020 at 06:19:32PM -0300, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > Renaming bpf_event__add_sb_event() to evlist__add_sb_event() and
> > requiring that the evlist be allocated beforehand.
> 
> hum, this seems to be done in previous patch, maybe you
> need to squash this with the previous one?

Right, doing it now.
 
> jirka
> 
> > 
> > This will allow using the same side band thread and evlist to be used
> > for multiple purposes in addition to react to PERF_RECORD_BPF_EVENT soon
> > after they are generated.
> > 
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Song Liu <songliubraving@fb.com>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  tools/perf/util/bpf-event.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/bpf-event.h b/tools/perf/util/bpf-event.h
> > index 2c7a50509659..68f315c3df5b 100644
> > --- a/tools/perf/util/bpf-event.h
> > +++ b/tools/perf/util/bpf-event.h
> > @@ -45,7 +45,7 @@ static inline int machine__process_bpf(struct machine *machine __maybe_unused,
> >  	return 0;
> >  }
> >  
> > -static inline int evlist__add_bpf_sb_event(struct evlist **evlist __maybe_unused,
> > +static inline int evlist__add_bpf_sb_event(struct evlist *evlist __maybe_unused,
> >  					   struct perf_env *env __maybe_unused)
> >  {
> >  	return 0;
> > -- 
> > 2.21.1
> > 
> 

-- 

- Arnaldo

[PATCH 5/7] perf parse-events: Add parse_events_option() variant that creates evlist

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

For the upcoming --switch-output-event option we want to create the side
band event, populate it with the specified events and then, if it is
present multiple times, go on adding to it, then, if the BPF tracking is
required, use the first event to set its attr.bpf_event to get those
PERF_RECORD_BPF_EVENT metadata events too.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-events.c | 23 +++++++++++++++++++++++
 tools/perf/util/parse-events.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 10107747b361..5795f3a8f71c 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2190,6 +2190,29 @@ int parse_events_option(const struct option *opt, const char *str,
 	return ret;
 }
 
+int parse_events_option_new_evlist(const struct option *opt, const char *str, int unset)
+{
+	struct evlist **evlistp = opt->value;
+	int ret;
+
+	if (*evlistp == NULL) {
+		*evlistp = evlist__new();
+
+		if (*evlistp == NULL) {
+			fprintf(stderr, "Not enough memory to create evlist\n");
+			return -1;
+		}
+	}
+
+	ret = parse_events_option(opt, str, unset);
+	if (ret) {
+		evlist__delete(*evlistp);
+		*evlistp = NULL;
+	}
+
+	return ret;
+}
+
 static int
 foreach_evsel_in_last_glob(struct evlist *evlist,
 			   int (*func)(struct evsel *evsel,
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 27596cbd0ba0..6ead9661238c 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -31,6 +31,7 @@ bool have_tracepoints(struct list_head *evlist);
 const char *event_type(int type);
 
 int parse_events_option(const struct option *opt, const char *str, int unset);
+int parse_events_option_new_evlist(const struct option *opt, const char *str, int unset);
 int parse_events(struct evlist *evlist, const char *str,
 		 struct parse_events_error *error);
 int parse_events_terms(struct list_head *terms, const char *str);
-- 
2.21.1

[PATCH 6/7] perf evlist: Allow reusing the side band thread for more purposes

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

I.e. so far we had just one event in that side band thread, a dummy one
with attr.bpf_event set, so that 'perf record' can go ahead and ask the
kernel for further information about BPF programs being loaded.

Allow for more than one event to be there, so that we can use it as
well for the upcoming --switch-output-event feature.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 22 ++++++++++++++++++++++
 tools/perf/util/evlist.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 1d0d36da223b..849058766757 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1777,6 +1777,19 @@ static void *perf_evlist__poll_thread(void *arg)
 	return NULL;
 }
 
+void evlist__set_cb(struct evlist *evlist, perf_evsel__sb_cb_t cb, void *data)
+{
+	struct evsel *evsel;
+
+	evlist__for_each_entry(evlist, evsel) {
+		evsel->core.attr.sample_id_all	  = 1;
+		evsel->core.attr.watermark	  = 1;
+		evsel->core.attr.wakeup_watermark = 1;
+		evsel->side_band.cb   = cb;
+		evsel->side_band.data = data;
+	}
+}
+
 int perf_evlist__start_sb_thread(struct evlist *evlist,
 				 struct target *target)
 {
@@ -1788,6 +1801,15 @@ int perf_evlist__start_sb_thread(struct evlist *evlist,
 	if (perf_evlist__create_maps(evlist, target))
 		goto out_delete_evlist;
 
+	if (evlist->core.nr_entries > 1) {
+		bool can_sample_identifier = perf_can_sample_identifier();
+
+		evlist__for_each_entry(evlist, counter)
+			perf_evsel__set_sample_id(counter, can_sample_identifier);
+
+		perf_evlist__set_id_pos(evlist);
+	}
+
 	evlist__for_each_entry(evlist, counter) {
 		if (evsel__open(counter, evlist->core.cpus,
 				     evlist->core.threads) < 0)
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 0f02408fff3e..1a8a979ae137 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -111,6 +111,7 @@ int perf_evlist__add_sb_event(struct evlist *evlist,
 			      struct perf_event_attr *attr,
 			      perf_evsel__sb_cb_t cb,
 			      void *data);
+void evlist__set_cb(struct evlist *evlist, perf_evsel__sb_cb_t cb, void *data);
 int perf_evlist__start_sb_thread(struct evlist *evlist,
 				 struct target *target);
 void perf_evlist__stop_sb_thread(struct evlist *evlist);
-- 
2.21.1

[PATCH 7/7] perf record: Introduce --switch-output-event

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Now we can use it with --overwrite to have a flight recorder mode that
gets snapshot requests from arbitrary events that are processed in the
side band thread together with the PERF_RECORD_BPF_EVENT processing.

Example:

To collect scheduler events until a recvmmsg syscall happens, system
wide:

  [root@five a]# rm -f perf.data.2020042717*
  [root@five a]# perf record --overwrite -e sched:*switch,syscalls:*recvmmsg --switch-output-event syscalls:sys_enter_recvmmsg
  [ perf record: dump data: Woken up 1 times ]
  [ perf record: Dump perf.data.2020042717585458 ]
  [ perf record: dump data: Woken up 1 times ]
  [ perf record: Dump perf.data.2020042717590235 ]
  [ perf record: dump data: Woken up 1 times ]
  [ perf record: Dump perf.data.2020042717590398 ]
  ^C[ perf record: Woken up 1 times to write data ]
  [ perf record: Dump perf.data.2020042717590511 ]
  [ perf record: Captured and wrote 7.244 MB perf.data.<timestamp> ]

So in the above case we had 3 snapshots, the fourth was forced by
control+C:

  [root@five a]# ls -la
  total 20440
  drwxr-xr-x.  2 root root    4096 Apr 27 17:59 .
  dr-xr-x---. 12 root root    4096 Apr 27 17:46 ..
  -rw-------.  1 root root 3936125 Apr 27 17:58 perf.data.2020042717585458
  -rw-------.  1 root root 5074869 Apr 27 17:59 perf.data.2020042717590235
  -rw-------.  1 root root 4291037 Apr 27 17:59 perf.data.2020042717590398
  -rw-------.  1 root root 7617037 Apr 27 17:59 perf.data.2020042717590511
  [root@five a]#

One can make this more precise by adding the switch output event to the
main -e events list, as since this is done asynchronously, a few events
after the signal event will appear in the snapshots, as can be seen
with:

  [root@five a]# rm -f perf.data.20200427175*
  [root@five a]# perf record --overwrite -e sched:*switch,syscalls:*recvmmsg --switch-output-event syscalls:sys_enter_recvmmsg
  [ perf record: dump data: Woken up 1 times ]
  [ perf record: Dump perf.data.2020042718024203 ]
  [ perf record: dump data: Woken up 1 times ]
  [ perf record: Dump perf.data.2020042718024301 ]
  [ perf record: dump data: Woken up 1 times ]
  [ perf record: Dump perf.data.2020042718024484 ]
  ^C[ perf record: Woken up 1 times to write data ]
  [ perf record: Dump perf.data.2020042718024562 ]
  [ perf record: Captured and wrote 7.337 MB perf.data.<timestamp> ]
  [root@five a]# perf script -i perf.data.2020042718024203 | tail -15
       PacerThread 148586 [005] 122.830729: sched:sched_switch: prev_comm=PacerThread prev_pid=148586...
           swapper      0 [000] 122.833588: sched:sched_switch: prev_comm=swapper/0 prev_pid=...
    NetworkManager   1251 [000] 122.833619: syscalls:sys_enter_recvmmsg: fd: 0x0000001c, mmsg: 0x7ffe83054a1...
           swapper      0 [002] 122.833624: sched:sched_switch: prev_comm=swapper/2 prev_pid=...
           swapper      0 [003] 122.833624: sched:sched_switch: prev_comm=swapper/3 prev_pid=...
    NetworkManager   1251 [000] 122.833626: syscalls:sys_exit_recvmmsg: 0x1
   kworker/3:3-eve 158946 [003] 122.833628: sched:sched_switch: prev_comm=kworker/3:3 prev_pid=15894...
           swapper      0 [004] 122.833641: sched:sched_switch: prev_comm=swapper/4 prev_pid=...
    NetworkManager   1251 [000] 122.833642: sched:sched_switch: prev_comm=NetworkManage...
              perf 228273 [002] 122.833645: sched:sched_switch: prev_comm=perf prev_pid=22827...
           swapper      0 [011] 122.833646: sched:sched_switch: prev_comm=swapper/1...
           swapper      0 [002] 122.833648: sched:sched_switch: prev_comm=swapper/...
   kworker/0:2-eve 207387 [000] 122.833648: sched:sched_switch: prev_comm=kworker/0:2 prev_pid=20738...
   kworker/2:3-eve 232038 [002] 122.833652: sched:sched_switch: prev_comm=kworker/2:3 prev_pid=23203...
              perf 235825 [003] 122.833653: sched:sched_switch: prev_comm=perf prev_pid=23582...
  [root@five a]#

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf-record.txt | 13 ++++++++
 tools/perf/builtin-record.c              | 40 +++++++++++++++++++++---
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 6e8b4649307c..561ef55743e2 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -556,6 +556,19 @@ overhead. You can still switch them on with:
 
   --switch-output --no-no-buildid  --no-no-buildid-cache
 
+--switch-output-event::
+Events that will cause the switch of the perf.data file, auto-selecting
+--switch-output=signal, the results are similar as internally the side band
+thread will also send a SIGUSR2 to the main one.
+
+Uses the same syntax as --event, it will just not be recorded, serving only to
+switch the perf.data file as soon as the --switch-output event is processed by
+a separate sideband thread.
+
+This sideband thread is also used to other purposes, like processing the
+PERF_RECORD_BPF_EVENT records as they happen, asking the kernel for extra BPF
+information, etc.
+
 --switch-max-files=N::
 
 When rotating perf.data with --switch-output, only keep N files.
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ed2244847400..8ff5eaec26e9 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -87,6 +87,7 @@ struct record {
 	struct evlist	*evlist;
 	struct perf_session	*session;
 	struct evlist		*sb_evlist;
+	pthread_t		thread_id;
 	int			realtime_prio;
 	bool			no_buildid;
 	bool			no_buildid_set;
@@ -1436,6 +1437,13 @@ static int record__synthesize(struct record *rec, bool tail)
 	return err;
 }
 
+static int record__process_signal_event(union perf_event *event __maybe_unused, void *data)
+{
+	struct record *rec = data;
+	pthread_kill(rec->thread_id, SIGUSR2);
+	return 0;
+}
+
 static int __cmd_record(struct record *rec, int argc, const char **argv)
 {
 	int err;
@@ -1580,12 +1588,24 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		goto out_child;
 	}
 
-	if (!opts->no_bpf_event) {
-		rec->sb_evlist = evlist__new();
+	if (rec->sb_evlist != NULL) {
+		/*
+		 * We get here if --switch-output-event populated the
+		 * sb_evlist, so associate a callback that will send a SIGUSR2
+		 * to the main thread.
+		 */
+		evlist__set_cb(rec->sb_evlist, record__process_signal_event, rec);
+		rec->thread_id = pthread_self();
+	}
 
+	if (!opts->no_bpf_event) {
 		if (rec->sb_evlist == NULL) {
-			pr_err("Couldn't create side band evlist.\n.");
-			goto out_child;
+			rec->sb_evlist = evlist__new();
+
+			if (rec->sb_evlist == NULL) {
+				pr_err("Couldn't create side band evlist.\n.");
+				goto out_child;
+			}
 		}
 
 		if (evlist__add_bpf_sb_event(rec->sb_evlist, &session->header.env)) {
@@ -2179,10 +2199,19 @@ static int switch_output_setup(struct record *rec)
 	};
 	unsigned long val;
 
+	/*
+	 * If we're using --switch-output-events, then we imply its 
+	 * --switch-output=signal, as we'll send a SIGUSR2 from the side band
+	 *  thread to its parent.
+	 */
+	if (rec->sb_evlist != NULL)
+		goto do_signal;
+
 	if (!s->set)
 		return 0;
 
 	if (!strcmp(s->str, "signal")) {
+do_signal:
 		s->signal = true;
 		pr_debug("switch-output with SIGUSR2 signal\n");
 		goto enabled;
@@ -2440,6 +2469,9 @@ static struct option __record_options[] = {
 			  &record.switch_output.set, "signal or size[BKMG] or time[smhd]",
 			  "Switch output when receiving SIGUSR2 (signal) or cross a size or time threshold",
 			  "signal"),
+	OPT_CALLBACK(0, "switch-output-event", &record.sb_evlist, "switch output event",
+		     "switch output event selector. use 'perf list' to list available events",
+		     parse_events_option_new_evlist),
 	OPT_INTEGER(0, "switch-max-files", &record.switch_output.num_files,
 		   "Limit number of switch output generated files"),
 	OPT_BOOLEAN(0, "dry-run", &dry_run,
-- 
2.21.1

Re: [PATCH 7/7] perf record: Introduce --switch-output-event

[Jiri Olsa]

On Mon, Apr 27, 2020 at 06:19:35PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> ---
>  tools/perf/Documentation/perf-record.txt | 13 ++++++++
>  tools/perf/builtin-record.c              | 40 +++++++++++++++++++++---
>  2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index 6e8b4649307c..561ef55743e2 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -556,6 +556,19 @@ overhead. You can still switch them on with:
>  
>    --switch-output --no-no-buildid  --no-no-buildid-cache
>  
> +--switch-output-event::
> +Events that will cause the switch of the perf.data file, auto-selecting
> +--switch-output=signal, the results are similar as internally the side band
> +thread will also send a SIGUSR2 to the main one.
> +
> +Uses the same syntax as --event, it will just not be recorded, serving only to
> +switch the perf.data file as soon as the --switch-output event is processed by
> +a separate sideband thread.
> +
> +This sideband thread is also used to other purposes, like processing the
> +PERF_RECORD_BPF_EVENT records as they happen, asking the kernel for extra BPF
> +information, etc.

first I thought we should follow the --switch-output 'xxx' way,
but then you need to specify an event, so I guess --switch-output-event
is better

> +
>  --switch-max-files=N::
>  
>  When rotating perf.data with --switch-output, only keep N files.
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index ed2244847400..8ff5eaec26e9 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -87,6 +87,7 @@ struct record {
>  	struct evlist	*evlist;
>  	struct perf_session	*session;
>  	struct evlist		*sb_evlist;
> +	pthread_t		thread_id;
>  	int			realtime_prio;
>  	bool			no_buildid;
>  	bool			no_buildid_set;
> @@ -1436,6 +1437,13 @@ static int record__synthesize(struct record *rec, bool tail)
>  	return err;
>  }
>  
> +static int record__process_signal_event(union perf_event *event __maybe_unused, void *data)
> +{
> +	struct record *rec = data;
> +	pthread_kill(rec->thread_id, SIGUSR2);
> +	return 0;
> +}
> +
>  static int __cmd_record(struct record *rec, int argc, const char **argv)
>  {
>  	int err;
> @@ -1580,12 +1588,24 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  		goto out_child;
>  	}
>  
> -	if (!opts->no_bpf_event) {
> -		rec->sb_evlist = evlist__new();
> +	if (rec->sb_evlist != NULL) {
> +		/*
> +		 * We get here if --switch-output-event populated the
> +		 * sb_evlist, so associate a callback that will send a SIGUSR2
> +		 * to the main thread.
> +		 */
> +		evlist__set_cb(rec->sb_evlist, record__process_signal_event, rec);
> +		rec->thread_id = pthread_self();
> +	}
>  
> +	if (!opts->no_bpf_event) {
>  		if (rec->sb_evlist == NULL) {
> -			pr_err("Couldn't create side band evlist.\n.");
> -			goto out_child;
> +			rec->sb_evlist = evlist__new();
> +
> +			if (rec->sb_evlist == NULL) {
> +				pr_err("Couldn't create side band evlist.\n.");
> +				goto out_child;
> +			}
>  		}
>  
>  		if (evlist__add_bpf_sb_event(rec->sb_evlist, &session->header.env)) {

it's getting bigger, I wonder we should put all the sb_* setup in
separated functions like sb_start/sb_stop


> @@ -2179,10 +2199,19 @@ static int switch_output_setup(struct record *rec)
>  	};
>  	unsigned long val;
>  
> +	/*
> +	 * If we're using --switch-output-events, then we imply its 
> +	 * --switch-output=signal, as we'll send a SIGUSR2 from the side band
> +	 *  thread to its parent.
> +	 */
> +	if (rec->sb_evlist != NULL)
> +		goto do_signal;
> +
>  	if (!s->set)
>  		return 0;

hum, it looks like this jump is not necessay and can be avoided
by some bool checks.. could we add some bool when --switch-output-event
is used, so we don't depend on wether rec->sb_evlist was allocated for
whatever reason?

jirka

>  
>  	if (!strcmp(s->str, "signal")) {
> +do_signal:
>  		s->signal = true;
>  		pr_debug("switch-output with SIGUSR2 signal\n");
>  		goto enabled;
> @@ -2440,6 +2469,9 @@ static struct option __record_options[] = {
>  			  &record.switch_output.set, "signal or size[BKMG] or time[smhd]",
>  			  "Switch output when receiving SIGUSR2 (signal) or cross a size or time threshold",
>  			  "signal"),
> +	OPT_CALLBACK(0, "switch-output-event", &record.sb_evlist, "switch output event",
> +		     "switch output event selector. use 'perf list' to list available events",
> +		     parse_events_option_new_evlist),
>  	OPT_INTEGER(0, "switch-max-files", &record.switch_output.num_files,
>  		   "Limit number of switch output generated files"),
>  	OPT_BOOLEAN(0, "dry-run", &dry_run,
> -- 
> 2.21.1
> 

Re: [PATCH 7/7] perf record: Introduce --switch-output-event

[Arnaldo Carvalho de Melo]

Em Tue, Apr 28, 2020 at 11:48:39AM +0200, Jiri Olsa escreveu:
> On Mon, Apr 27, 2020 at 06:19:35PM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > ---
> >  tools/perf/Documentation/perf-record.txt | 13 ++++++++
> >  tools/perf/builtin-record.c              | 40 +++++++++++++++++++++---
> >  2 files changed, 49 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> > index 6e8b4649307c..561ef55743e2 100644
> > --- a/tools/perf/Documentation/perf-record.txt
> > +++ b/tools/perf/Documentation/perf-record.txt
> > @@ -556,6 +556,19 @@ overhead. You can still switch them on with:
> >  
> >    --switch-output --no-no-buildid  --no-no-buildid-cache
> >  
> > +--switch-output-event::
> > +Events that will cause the switch of the perf.data file, auto-selecting
> > +--switch-output=signal, the results are similar as internally the side band
> > +thread will also send a SIGUSR2 to the main one.
> > +
> > +Uses the same syntax as --event, it will just not be recorded, serving only to
> > +switch the perf.data file as soon as the --switch-output event is processed by
> > +a separate sideband thread.
> > +
> > +This sideband thread is also used to other purposes, like processing the
> > +PERF_RECORD_BPF_EVENT records as they happen, asking the kernel for extra BPF
> > +information, etc.
> 
> first I thought we should follow the --switch-output 'xxx' way,
> but then you need to specify an event, so I guess --switch-output-event
> is better

Yeah, otherwise we'd have to play a bit more tricky games with
--switch-output parsing, to support something like:

--switch-output=event(event_desc_str)

and pass that event_desc_str to parse_events_option_new_evlist(), do you
think that sounds better?
 
> > +
> >  --switch-max-files=N::
> >  
> >  When rotating perf.data with --switch-output, only keep N files.
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index ed2244847400..8ff5eaec26e9 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -87,6 +87,7 @@ struct record {
> >  	struct evlist	*evlist;
> >  	struct perf_session	*session;
> >  	struct evlist		*sb_evlist;
> > +	pthread_t		thread_id;
> >  	int			realtime_prio;
> >  	bool			no_buildid;
> >  	bool			no_buildid_set;
> > @@ -1436,6 +1437,13 @@ static int record__synthesize(struct record *rec, bool tail)
> >  	return err;
> >  }
> >  
> > +static int record__process_signal_event(union perf_event *event __maybe_unused, void *data)
> > +{
> > +	struct record *rec = data;
> > +	pthread_kill(rec->thread_id, SIGUSR2);
> > +	return 0;
> > +}
> > +
> >  static int __cmd_record(struct record *rec, int argc, const char **argv)
> >  {
> >  	int err;
> > @@ -1580,12 +1588,24 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> >  		goto out_child;
> >  	}
> >  
> > -	if (!opts->no_bpf_event) {
> > -		rec->sb_evlist = evlist__new();
> > +	if (rec->sb_evlist != NULL) {
> > +		/*
> > +		 * We get here if --switch-output-event populated the
> > +		 * sb_evlist, so associate a callback that will send a SIGUSR2
> > +		 * to the main thread.
> > +		 */
> > +		evlist__set_cb(rec->sb_evlist, record__process_signal_event, rec);
> > +		rec->thread_id = pthread_self();
> > +	}
> >  
> > +	if (!opts->no_bpf_event) {
> >  		if (rec->sb_evlist == NULL) {
> > -			pr_err("Couldn't create side band evlist.\n.");
> > -			goto out_child;
> > +			rec->sb_evlist = evlist__new();
> > +
> > +			if (rec->sb_evlist == NULL) {
> > +				pr_err("Couldn't create side band evlist.\n.");
> > +				goto out_child;
> > +			}
> >  		}
> >  
> >  		if (evlist__add_bpf_sb_event(rec->sb_evlist, &session->header.env)) {
> 
> it's getting bigger, I wonder we should put all the sb_* setup in
> separated functions like sb_start/sb_stop

Well, the rec->thread_id = pthread_self(); part is just for reusing a
'perf record' specific mechanism, what to do when the event appears in
the side band thread ring buffer, the evlist__set_cb() also is related
to that, moving thread_id to evlist seems too much at this time.

> > @@ -2179,10 +2199,19 @@ static int switch_output_setup(struct record *rec)
> >  	};
> >  	unsigned long val;
> >  
> > +	/*
> > +	 * If we're using --switch-output-events, then we imply its 
> > +	 * --switch-output=signal, as we'll send a SIGUSR2 from the side band
> > +	 *  thread to its parent.
> > +	 */
> > +	if (rec->sb_evlist != NULL)
> > +		goto do_signal;
> > +
> >  	if (!s->set)
> >  		return 0;
 
> hum, it looks like this jump is not necessay and can be avoided
> by some bool checks.. could we add some bool when --switch-output-event
> is used, so we don't depend on wether rec->sb_evlist was allocated for
> whatever reason?

If rec->sb_evlist is NULL, then there was no --switch-output-event
passed... The only advantage in adding the complexity below would be if
we had rec->switch_output_event_set which would clarify that sb_evlist
is not used only for --switch-output-event, to make things clearer.

And this still leaves us with the jump, otherwise we would have to test
it twice, right?

I think I'll separate the patch adding OPT_CALLBACK_SET(), then fold the
switch_output_event_set addition to builtin-record, ok?

- Arnaldo

diff --git a/tools/lib/subcmd/parse-options.h b/tools/lib/subcmd/parse-options.h
index af9def589863..d2414144eb8c 100644
--- a/tools/lib/subcmd/parse-options.h
+++ b/tools/lib/subcmd/parse-options.h
@@ -151,6 +151,8 @@ struct option {
 	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = "time", .help = (h), .callback = parse_opt_approxidate_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) \
 	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = (a), .help = (h), .callback = (f) }
+#define OPT_CALLBACK_SET(s, l, v, os, a, h, f) \
+	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = (a), .help = (h), .callback = (f), .set = check_vtype(os, bool *)}
 #define OPT_CALLBACK_NOOPT(s, l, v, a, h, f) \
 	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = (a), .help = (h), .callback = (f), .flags = PARSE_OPT_NOARG }
 #define OPT_CALLBACK_DEFAULT(s, l, v, a, h, f, d) \
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 8ff5eaec26e9..7a6a89972691 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -89,6 +89,7 @@ struct record {
 	struct evlist		*sb_evlist;
 	pthread_t		thread_id;
 	int			realtime_prio;
+	bool			switch_output_event_set;
 	bool			no_buildid;
 	bool			no_buildid_set;
 	bool			no_buildid_cache;
@@ -2204,7 +2205,7 @@ static int switch_output_setup(struct record *rec)
 	 * --switch-output=signal, as we'll send a SIGUSR2 from the side band
 	 *  thread to its parent.
 	 */
-	if (rec->sb_evlist != NULL)
+	if (rec->switch_output_event_set)
 		goto do_signal;
 
 	if (!s->set)
@@ -2469,9 +2470,9 @@ static struct option __record_options[] = {
 			  &record.switch_output.set, "signal or size[BKMG] or time[smhd]",
 			  "Switch output when receiving SIGUSR2 (signal) or cross a size or time threshold",
 			  "signal"),
-	OPT_CALLBACK(0, "switch-output-event", &record.sb_evlist, "switch output event",
-		     "switch output event selector. use 'perf list' to list available events",
-		     parse_events_option_new_evlist),
+	OPT_CALLBACK_SET(0, "switch-output-event", &record.sb_evlist, &record.switch_output_event_set, "switch output event",
+			 "switch output event selector. use 'perf list' to list available events",
+			 parse_events_option_new_evlist),
 	OPT_INTEGER(0, "switch-max-files", &record.switch_output.num_files,
 		   "Limit number of switch output generated files"),
 	OPT_BOOLEAN(0, "dry-run", &dry_run,

Re: [PATCH 7/7] perf record: Introduce --switch-output-event

[Jiri Olsa]

On Tue, Apr 28, 2020 at 09:16:01AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > > +				pr_err("Couldn't create side band evlist.\n.");
> > > +				goto out_child;
> > > +			}
> > >  		}
> > >  
> > >  		if (evlist__add_bpf_sb_event(rec->sb_evlist, &session->header.env)) {
> > 
> > it's getting bigger, I wonder we should put all the sb_* setup in
> > separated functions like sb_start/sb_stop
> 
> Well, the rec->thread_id = pthread_self(); part is just for reusing a
> 'perf record' specific mechanism, what to do when the event appears in
> the side band thread ring buffer, the evlist__set_cb() also is related
> to that, moving thread_id to evlist seems too much at this time.

hum, I meant record specific static functions sb_start/sb_stop,
not inside evlist.. just to have it separated

> 
> > > @@ -2179,10 +2199,19 @@ static int switch_output_setup(struct record *rec)
> > >  	};
> > >  	unsigned long val;
> > >  
> > > +	/*
> > > +	 * If we're using --switch-output-events, then we imply its 
> > > +	 * --switch-output=signal, as we'll send a SIGUSR2 from the side band
> > > +	 *  thread to its parent.
> > > +	 */
> > > +	if (rec->sb_evlist != NULL)
> > > +		goto do_signal;
> > > +
> > >  	if (!s->set)
> > >  		return 0;
>  
> > hum, it looks like this jump is not necessay and can be avoided
> > by some bool checks.. could we add some bool when --switch-output-event
> > is used, so we don't depend on wether rec->sb_evlist was allocated for
> > whatever reason?
> 
> If rec->sb_evlist is NULL, then there was no --switch-output-event
> passed... The only advantage in adding the complexity below would be if
> we had rec->switch_output_event_set which would clarify that sb_evlist
> is not used only for --switch-output-event, to make things clearer.
> 
> And this still leaves us with the jump, otherwise we would have to test
> it twice, right?

still I like the idea of checking bool twice then adding jumps

> 
> I think I'll separate the patch adding OPT_CALLBACK_SET(), then fold the
> switch_output_event_set addition to builtin-record, ok?

ok, or set the bool directly in the callback, both works for me ;-)

thanks,
jirka

Re: [PATCH 7/7] perf record: Introduce --switch-output-event

[Arnaldo Carvalho de Melo]

Em Tue, Apr 28, 2020 at 03:22:57PM +0200, Jiri Olsa escreveu:
> On Tue, Apr 28, 2020 at 09:16:01AM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > > > +				pr_err("Couldn't create side band evlist.\n.");
> > > > +				goto out_child;
> > > > +			}
> > > >  		}

> > > >  		if (evlist__add_bpf_sb_event(rec->sb_evlist, &session->header.env)) {

> > > it's getting bigger, I wonder we should put all the sb_* setup in
> > > separated functions like sb_start/sb_stop

> > Well, the rec->thread_id = pthread_self(); part is just for reusing a
> > 'perf record' specific mechanism, what to do when the event appears in
> > the side band thread ring buffer, the evlist__set_cb() also is related
> > to that, moving thread_id to evlist seems too much at this time.

> hum, I meant record specific static functions sb_start/sb_stop,
> not inside evlist.. just to have it separated

Ok, so I have the patch below on top of that series, and its all
available in my perf/switch-output-event, that is on top of more patches
collected today, all going well, this perf/switch-output-event will turn
into perf/core ang go upstream soon, then I have to loo at Adrian's
kernel+tooling patchkit,

- Arnaldo

commit a25516b4db23ba8f956b990d37ec6728e5221718
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Tue Apr 28 14:58:29 2020 -0300

    perf record: Move side band evlist setup to separate routine
    
    It is quite big by now, move that code to a separate
    record__setup_sb_evlist() routine.
    
    Suggested-by: Jiri Olsa <jolsa@redhat.com>
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Song Liu <songliubraving@fb.com>
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 7a6a89972691..bb3d30616bf3 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1445,6 +1445,44 @@ static int record__process_signal_event(union perf_event *event __maybe_unused,
 	return 0;
 }
 
+static int record__setup_sb_evlist(struct record *rec)
+{
+	struct record_opts *opts = &rec->opts;
+
+	if (rec->sb_evlist != NULL) {
+		/*
+		 * We get here if --switch-output-event populated the
+		 * sb_evlist, so associate a callback that will send a SIGUSR2
+		 * to the main thread.
+		 */
+		evlist__set_cb(rec->sb_evlist, record__process_signal_event, rec);
+		rec->thread_id = pthread_self();
+	}
+
+	if (!opts->no_bpf_event) {
+		if (rec->sb_evlist == NULL) {
+			rec->sb_evlist = evlist__new();
+
+			if (rec->sb_evlist == NULL) {
+				pr_err("Couldn't create side band evlist.\n.");
+				return -1;
+			}
+		}
+
+		if (evlist__add_bpf_sb_event(rec->sb_evlist, &rec->session->header.env)) {
+			pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
+			return -1;
+		}
+	}
+
+	if (perf_evlist__start_sb_thread(rec->sb_evlist, &rec->opts.target)) {
+		pr_debug("Couldn't start the BPF side band thread:\nBPF programs starting from now on won't be annotatable\n");
+		opts->no_bpf_event = true;
+	}
+
+	return 0;
+}
+
 static int __cmd_record(struct record *rec, int argc, const char **argv)
 {
 	int err;
@@ -1589,36 +1627,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		goto out_child;
 	}
 
-	if (rec->sb_evlist != NULL) {
-		/*
-		 * We get here if --switch-output-event populated the
-		 * sb_evlist, so associate a callback that will send a SIGUSR2
-		 * to the main thread.
-		 */
-		evlist__set_cb(rec->sb_evlist, record__process_signal_event, rec);
-		rec->thread_id = pthread_self();
-	}
-
-	if (!opts->no_bpf_event) {
-		if (rec->sb_evlist == NULL) {
-			rec->sb_evlist = evlist__new();
-
-			if (rec->sb_evlist == NULL) {
-				pr_err("Couldn't create side band evlist.\n.");
-				goto out_child;
-			}
-		}
-
-		if (evlist__add_bpf_sb_event(rec->sb_evlist, &session->header.env)) {
-			pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
-			goto out_child;
-		}
-	}
-
-	if (perf_evlist__start_sb_thread(rec->sb_evlist, &rec->opts.target)) {
-		pr_debug("Couldn't start the BPF side band thread:\nBPF programs starting from now on won't be annotatable\n");
-		opts->no_bpf_event = true;
-	}
+	err = record__setup_sb_evlist(rec);
+	if (err)
+		goto out_child;
 
 	err = record__synthesize(rec, false);
 	if (err < 0)

Re: [PATCH 7/7] perf record: Introduce --switch-output-event

[Jiri Olsa]

On Tue, Apr 28, 2020 at 03:05:18PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 28, 2020 at 03:22:57PM +0200, Jiri Olsa escreveu:
> > On Tue, Apr 28, 2020 at 09:16:01AM -0300, Arnaldo Carvalho de Melo wrote:
> > 
> > SNIP
> > 
> > > > > +				pr_err("Couldn't create side band evlist.\n.");
> > > > > +				goto out_child;
> > > > > +			}
> > > > >  		}
> 
> > > > >  		if (evlist__add_bpf_sb_event(rec->sb_evlist, &session->header.env)) {
> 
> > > > it's getting bigger, I wonder we should put all the sb_* setup in
> > > > separated functions like sb_start/sb_stop
> 
> > > Well, the rec->thread_id = pthread_self(); part is just for reusing a
> > > 'perf record' specific mechanism, what to do when the event appears in
> > > the side band thread ring buffer, the evlist__set_cb() also is related
> > > to that, moving thread_id to evlist seems too much at this time.
> 
> > hum, I meant record specific static functions sb_start/sb_stop,
> > not inside evlist.. just to have it separated
> 
> Ok, so I have the patch below on top of that series, and its all
> available in my perf/switch-output-event, that is on top of more patches
> collected today, all going well, this perf/switch-output-event will turn
> into perf/core ang go upstream soon, then I have to loo at Adrian's
> kernel+tooling patchkit,

ok looks good

thank,
jirka

> 
> - Arnaldo
> 
> commit a25516b4db23ba8f956b990d37ec6728e5221718
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Tue Apr 28 14:58:29 2020 -0300
> 
>     perf record: Move side band evlist setup to separate routine
>     
>     It is quite big by now, move that code to a separate
>     record__setup_sb_evlist() routine.
>     
>     Suggested-by: Jiri Olsa <jolsa@redhat.com>
>     Cc: Adrian Hunter <adrian.hunter@intel.com>
>     Cc: Namhyung Kim <namhyung@kernel.org>
>     Cc: Song Liu <songliubraving@fb.com>
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 7a6a89972691..bb3d30616bf3 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1445,6 +1445,44 @@ static int record__process_signal_event(union perf_event *event __maybe_unused,
>  	return 0;
>  }
>  
> +static int record__setup_sb_evlist(struct record *rec)
> +{
> +	struct record_opts *opts = &rec->opts;
> +
> +	if (rec->sb_evlist != NULL) {
> +		/*
> +		 * We get here if --switch-output-event populated the
> +		 * sb_evlist, so associate a callback that will send a SIGUSR2
> +		 * to the main thread.
> +		 */
> +		evlist__set_cb(rec->sb_evlist, record__process_signal_event, rec);
> +		rec->thread_id = pthread_self();
> +	}
> +
> +	if (!opts->no_bpf_event) {
> +		if (rec->sb_evlist == NULL) {
> +			rec->sb_evlist = evlist__new();
> +
> +			if (rec->sb_evlist == NULL) {
> +				pr_err("Couldn't create side band evlist.\n.");
> +				return -1;
> +			}
> +		}
> +
> +		if (evlist__add_bpf_sb_event(rec->sb_evlist, &rec->session->header.env)) {
> +			pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
> +			return -1;
> +		}
> +	}
> +
> +	if (perf_evlist__start_sb_thread(rec->sb_evlist, &rec->opts.target)) {
> +		pr_debug("Couldn't start the BPF side band thread:\nBPF programs starting from now on won't be annotatable\n");
> +		opts->no_bpf_event = true;
> +	}
> +
> +	return 0;
> +}
> +
>  static int __cmd_record(struct record *rec, int argc, const char **argv)
>  {
>  	int err;
> @@ -1589,36 +1627,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  		goto out_child;
>  	}
>  
> -	if (rec->sb_evlist != NULL) {
> -		/*
> -		 * We get here if --switch-output-event populated the
> -		 * sb_evlist, so associate a callback that will send a SIGUSR2
> -		 * to the main thread.
> -		 */
> -		evlist__set_cb(rec->sb_evlist, record__process_signal_event, rec);
> -		rec->thread_id = pthread_self();
> -	}
> -
> -	if (!opts->no_bpf_event) {
> -		if (rec->sb_evlist == NULL) {
> -			rec->sb_evlist = evlist__new();
> -
> -			if (rec->sb_evlist == NULL) {
> -				pr_err("Couldn't create side band evlist.\n.");
> -				goto out_child;
> -			}
> -		}
> -
> -		if (evlist__add_bpf_sb_event(rec->sb_evlist, &session->header.env)) {
> -			pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
> -			goto out_child;
> -		}
> -	}
> -
> -	if (perf_evlist__start_sb_thread(rec->sb_evlist, &rec->opts.target)) {
> -		pr_debug("Couldn't start the BPF side band thread:\nBPF programs starting from now on won't be annotatable\n");
> -		opts->no_bpf_event = true;
> -	}
> +	err = record__setup_sb_evlist(rec);
> +	if (err)
> +		goto out_child;
>  
>  	err = record__synthesize(rec, false);
>  	if (err < 0)
>