Skip to contentSkip to conversation

Implement --switch-output-events

RFC PATCH Superseded Arnaldo Carvalho de Melo 3 participants 15 messages Apr 27, 2020 – Apr 29, 2020 perf events

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Arnaldo Carvalho de Melo

author maintainer
#0 lore raw

RFC PATCH 1/1 Implement --switch-output-events

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(-)
Signature · 1 line
2.21.1
author maintainer
#1 lore raw

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

From: Arnaldo Carvalho de Melo <[email protected]>

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

Cc Adrian Hunter <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/builtin-record.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
tools/perf/builtin-record.c +4 -4
index 2e8011f179f2..2348c4205e59 100644
@@ -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;
 }
Signature · 1 line
2.21.1
author maintainer
#2 lore raw

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

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

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

Cc Adrian Hunter <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/builtin-top.c | 7 +++----
 tools/perf/util/top.h    | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)
tools/perf/builtin-top.c +3 -4
index 6b067a5ba1d5..70e1c732db6a 100644
@@ -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);
tools/perf/util/top.h +1 -1
index 45dc84ddff37..ff8391208ecd 100644
@@ -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;
Signature · 1 line
2.21.1
author maintainer
#3 lore raw

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

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

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 <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 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(-)
tools/perf/builtin-record.c +14 -3
index 2348c4205e59..ed2244847400 100644
@@ -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");
tools/perf/builtin-top.c +13 -2
index 70e1c732db6a..de24aced7213 100644
@@ -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");
tools/perf/util/bpf-event.c +1 -2
index 0cd41a862952..3742511a08d1 100644
@@ -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,
tools/perf/util/bpf-event.h +3 -4
index 81fdc88e6c1a..2c7a50509659 100644
@@ -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;
 }
tools/perf/util/evlist.c +3 -14
index 82d9f9bb8975..1d0d36da223b 100644
@@ -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;
 }
tools/perf/util/evlist.h +1 -1
index f5bd5c386df1..0f02408fff3e 100644
@@ -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);
Signature · 1 line
2.21.1
author maintainer
#4 lore raw

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

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

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 <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/bpf-event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/bpf-event.h +1 -1
index 2c7a50509659..68f315c3df5b 100644
@@ -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;
Signature · 1 line
2.21.1
#5 lore raw

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

Quoted 4 lines · Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

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

Quoted 30 lines

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 <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/bpf-event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/bpf-event.h +1 -1
index 2c7a50509659..68f315c3df5b 100644
@@ -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;
Signature · 1 line
2.21.1
#6 lore raw

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

Em Tue, Apr 28, 2020 at 11:48:51AM +0200, Jiri Olsa escreveu:

Quoted 8 lines · Arnaldo Carvalho de Melo
Quoted 4 lines

From: Arnaldo Carvalho de Melo <[email protected]>

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.

Quoted 33 lines

jirka

Quoted 30 lines

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 <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/bpf-event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/bpf-event.h +1 -1
index 2c7a50509659..68f315c3df5b 100644
@@ -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;
Signature · 1 line
2.21.1
Signature · 1 line
- Arnaldo
author maintainer
#7 lore raw

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

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

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 <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/parse-events.c | 23 +++++++++++++++++++++++
 tools/perf/util/parse-events.h |  1 +
 2 files changed, 24 insertions(+)
tools/perf/util/parse-events.c +23 -0
index 10107747b361..5795f3a8f71c 100644
@@ -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,
tools/perf/util/parse-events.h +1 -0
index 27596cbd0ba0..6ead9661238c 100644
@@ -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);
Signature · 1 line
2.21.1
author maintainer
#8 lore raw

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

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

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 <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/evlist.c | 22 ++++++++++++++++++++++
 tools/perf/util/evlist.h |  1 +
 2 files changed, 23 insertions(+)
tools/perf/util/evlist.c +22 -0
index 1d0d36da223b..849058766757 100644
@@ -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)
tools/perf/util/evlist.h +1 -0
index 0f02408fff3e..1a8a979ae137 100644
@@ -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);
Signature · 1 line
2.21.1
author maintainer
#9 lore raw

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

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

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 <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Cc Wang Nan <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/Documentation/perf-record.txt | 13 ++++++++
 tools/perf/builtin-record.c              | 40 +++++++++++++++++++++---
 2 files changed, 49 insertions(+), 4 deletions(-)
tools/perf/Documentation/perf-record.txt +13 -0
index 6e8b4649307c..561ef55743e2 100644
@@ -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.
tools/perf/builtin-record.c +36 -4
index ed2244847400..8ff5eaec26e9 100644
@@ -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,
Signature · 1 line
2.21.1
#10 lore raw

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

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

SNIP

Quoted 15 lines
---
 tools/perf/Documentation/perf-record.txt | 13 ++++++++
 tools/perf/builtin-record.c              | 40 +++++++++++++++++++++---
 2 files changed, 49 insertions(+), 4 deletions(-)
tools/perf/Documentation/perf-record.txt +2 -0
index 6e8b4649307c..561ef55743e2 100644
@@ -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

tools/perf/Documentation/perf-record.txt overhead. You can still switch them on with: · line 570

Quoted 10 lines
tools/perf/Documentation/perf-record.txt +10 -0
+--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

Quoted 49 lines
tools/perf/Documentation/perf-record.txt +1 -0
+
 --switch-max-files=N::
 
 When rotating perf.data with --switch-output, only keep N files.
tools/perf/builtin-record.c +18 -3
index ed2244847400..8ff5eaec26e9 100644
@@ -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.");

tools/perf/builtin-record.c static int __cmd_record(struct record *rec, int argc, const char **argv) · line 1611

Quoted 10 lines
tools/perf/builtin-record.c +6 -1
-			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

Quoted 4 lines
tools/perf/builtin-record.c +0 -0
@@ -2179,10 +2199,19 @@ static int switch_output_setup(struct record *rec)
 	};
 	unsigned long val;

tools/perf/builtin-record.c static int switch_output_setup(struct record *rec) · line 2211

Quoted 10 lines
tools/perf/builtin-record.c +8 -0
+	/*
+	 * 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

Quoted 19 lines
tools/perf/builtin-record.c +4 -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,
Signature · 1 line
2.21.1
#11 lore raw

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

Em Tue, Apr 28, 2020 at 11:48:39AM +0200, Jiri Olsa escreveu:

Quoted 17 lines · Arnaldo Carvalho de Melo

SNIP

Quoted 13 lines
---
 tools/perf/Documentation/perf-record.txt | 13 ++++++++
 tools/perf/builtin-record.c              | 40 +++++++++++++++++++++---
 2 files changed, 49 insertions(+), 4 deletions(-)
tools/perf/Documentation/perf-record.txt +0 -0
index 6e8b4649307c..561ef55743e2 100644
@@ -556,6 +556,19 @@ overhead. You can still switch them on with:
 
   --switch-output --no-no-buildid  --no-no-buildid-cache
Quoted 16 lines

tools/perf/Documentation/perf-record.txt overhead. You can still switch them on with: · line 570

Quoted 12 lines
tools/perf/Documentation/perf-record.txt +12 -0
+--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?

Quoted 58 lines
Quoted 58 lines
tools/perf/Documentation/perf-record.txt +1 -0
+
 --switch-max-files=N::
 
 When rotating perf.data with --switch-output, only keep N files.
tools/perf/builtin-record.c +24 -4
index ed2244847400..8ff5eaec26e9 100644
@@ -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;
+			}
 		}
Quoted 4 lines

tools/perf/builtin-record.c static int __cmd_record(struct record *rec, int argc, const char **argv) · line 1611

Quoted 1 line
tools/perf/builtin-record.c +0 -0
 		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.

Quoted 4 lines
Quoted 4 lines
tools/perf/builtin-record.c +0 -0
@@ -2179,10 +2199,19 @@ static int switch_output_setup(struct record *rec)
 	};
 	unsigned long val;
Quoted 15 lines

tools/perf/builtin-record.c static int switch_output_setup(struct record *rec) · line 2211

Quoted 10 lines
tools/perf/builtin-record.c +8 -0
+	/*
+	 * 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

tools/lib/subcmd/parse-options.h +2 -0
index af9def589863..d2414144eb8c 100644
@@ -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) \
tools/perf/builtin-record.c +5 -4
index 8ff5eaec26e9..7a6a89972691 100644
@@ -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,
#12 lore raw

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

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

SNIP

Quoted 10 lines
Quoted 9 lines

Review

Quoted 6 lines
(quoted diff) +3 -0
+				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

Quoted 4 lines

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

Quoted 21 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +0 -0
@@ -2179,10 +2199,19 @@ static int switch_output_setup(struct record *rec)
 	};
 	unsigned long val;
Quoted 15 lines

Review static int switch_output_setup(struct record *rec) · line 2211

Quoted 10 lines
(quoted diff) +8 -0
+	/*
+	 * 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?

Quoted 7 lines

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

Quoted 3 lines

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

#13 lore raw

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

Em Tue, Apr 28, 2020 at 03:22:57PM +0200, Jiri Olsa escreveu:

Quoted 14 lines · Arnaldo Carvalho de Melo

SNIP

Quoted 9 lines
Quoted 9 lines

Review

Quoted 6 lines
(quoted diff) +3 -0
+				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

Quoted 7 lines
Quoted 4 lines

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 <[email protected]>
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 <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
tools/perf/builtin-record.c +41 -30
index 7a6a89972691..bb3d30616bf3 100644
@@ -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)
#14 lore raw

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

Quoted 20 lines · Arnaldo Carvalho de Melo

Em Tue, Apr 28, 2020 at 03:22:57PM +0200, Jiri Olsa escreveu:

Quoted 18 lines · Arnaldo Carvalho de Melo

SNIP

Quoted 14 lines
Quoted 9 lines

Review

Quoted 6 lines
(quoted diff) +3 -0
+				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.

Quoted 8 lines
Quoted 2 lines

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

Quoted 108 lines

- Arnaldo

commit a25516b4db23ba8f956b990d37ec6728e5221718

Author: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Song Liu <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
tools/perf/builtin-record.c +41 -30
index 7a6a89972691..bb3d30616bf3 100644
@@ -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)