linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/7] perf record: Track sideband events for all CPUs when tracing selected CPUs
@ 2023-07-15  3:29 Yang Jihong
  2023-07-15  3:29 ` [PATCH v2 1/7] perf evlist: Add perf_evlist__go_system_wide() helper Yang Jihong
                   ` (6 more replies)
  0 siblings, 7 replies; 27+ messages in thread
From: Yang Jihong @ 2023-07-15  3:29 UTC (permalink / raw)
  To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, irogers, adrian.hunter, kan.liang, james.clark, tmricht,
	ak, anshuman.khandual, linux-kernel, linux-perf-users
  Cc: yangjihong1

User space tasks can migrate between CPUs, track sideband events for all
CPUs.

The specific scenarios are as follows:

         CPU0                                 CPU1
  perf record -C 0 start
                              taskA starts to be created and executed
                                -> PERF_RECORD_COMM and PERF_RECORD_MMAP
                                   events only deliver to CPU1
                              ......
                                |
                          migrate to CPU0
                                |
  Running on CPU0    <----------/
  ...

  perf record -C 0 stop

Now perf samples the PC of taskA. However, perf does not record the
PERF_RECORD_COMM and PERF_RECORD_COMM events of taskA.
Therefore, the comm and symbols of taskA cannot be parsed.

The sys_perf_event_open invoked is as follows:

  # perf --debug verbose=3 record -e cpu-clock -C 1 true
  <SNIP>
  Opening: cpu-clock
  ------------------------------------------------------------
  perf_event_attr:
    type                             1
    size                             136
    { sample_period, sample_freq }   4000
    sample_type                      IP|TID|TIME|ID|CPU|PERIOD
    read_format                      ID|LOST
    disabled                         1
    inherit                          1
    freq                             1
    sample_id_all                    1
    exclude_guest                    1
  ------------------------------------------------------------
  sys_perf_event_open: pid -1  cpu 1  group_fd -1  flags 0x8 = 5
  Opening: dummy:HG
  ------------------------------------------------------------
  perf_event_attr:
    type                             1
    size                             136
    config                           0x9
    { sample_period, sample_freq }   1
    sample_type                      IP|TID|TIME|CPU|IDENTIFIER
    read_format                      ID|LOST
    inherit                          1
    exclude_kernel                   1
    exclude_hv                       1
    mmap                             1
    comm                             1
    task                             1
    sample_id_all                    1
    exclude_guest                    1
    mmap2                            1
    comm_exec                        1
    ksymbol                          1
    bpf_event                        1
  ------------------------------------------------------------
  sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 6
  sys_perf_event_open: pid -1  cpu 1  group_fd -1  flags 0x8 = 7
  sys_perf_event_open: pid -1  cpu 2  group_fd -1  flags 0x8 = 9
  sys_perf_event_open: pid -1  cpu 3  group_fd -1  flags 0x8 = 10
  sys_perf_event_open: pid -1  cpu 4  group_fd -1  flags 0x8 = 11
  sys_perf_event_open: pid -1  cpu 5  group_fd -1  flags 0x8 = 12
  sys_perf_event_open: pid -1  cpu 6  group_fd -1  flags 0x8 = 13
  sys_perf_event_open: pid -1  cpu 7  group_fd -1  flags 0x8 = 14
  <SNIP>

Changes since v1:
 - Add perf_evlist__go_system_wide() via internal/evlist.h instead of
   exporting perf_evlist__propagate_maps().
 - Use evlist__add_aux_dummy() instead of evlist__add_dummy() in
   evlist__findnew_tracking_event().
 - Add a parameter in evlist__findnew_tracking_event() to deal with
   system_wide inside.
 - Add sideband for all CPUs when tracing selected CPUs comments on
   the perf record man page.
 - Use "sideband events" instead of "tracking events".
 - Adjust the patches Sequence.
 - Add patch5 to skip dummy event sample_type check for evlist_config.
 - Add patch6 to update system-wide-dummy attr values for perf test.

Yang Jihong (7):
  perf evlist: Add perf_evlist__go_system_wide() helper
  perf evlist: Add evlist__findnew_tracking_event() helper
  perf record: Move setting dummy tracking before
    record__init_thread_masks()
  perf record: Track sideband events for all CPUs when tracing selected
    CPUs
  perf evlist: Skip dummy event sample_type check for evlist_config
  perf test: Update system-wide-dummy attr expected values
  perf test: Add test case for record sideband events

 tools/lib/perf/evlist.c                   |  9 +++
 tools/lib/perf/include/internal/evlist.h  |  2 +
 tools/perf/Documentation/perf-record.txt  |  3 +
 tools/perf/builtin-record.c               | 69 ++++++++++++++---------
 tools/perf/tests/attr/system-wide-dummy   | 12 ++--
 tools/perf/tests/shell/record_tracking.sh | 44 +++++++++++++++
 tools/perf/util/evlist.c                  | 18 ++++++
 tools/perf/util/evlist.h                  |  1 +
 tools/perf/util/record.c                  |  7 +++
 9 files changed, 132 insertions(+), 33 deletions(-)
 create mode 100755 tools/perf/tests/shell/record_tracking.sh

-- 
2.30.GIT


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v2 1/7] perf evlist: Add perf_evlist__go_system_wide() helper
  2023-07-15  3:29 [PATCH v2 0/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
@ 2023-07-15  3:29 ` Yang Jihong
  2023-07-15  3:29 ` [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper Yang Jihong
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 27+ messages in thread
From: Yang Jihong @ 2023-07-15  3:29 UTC (permalink / raw)
  To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, irogers, adrian.hunter, kan.liang, james.clark, tmricht,
	ak, anshuman.khandual, linux-kernel, linux-perf-users
  Cc: yangjihong1

For dummy events that keep tracking, we may need to modify its cpu_maps.
For example, change the cpu_maps to record sideband events for all CPUS.
Add perf_evlist__go_system_wide() helper to support this scenario.

Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
 tools/lib/perf/evlist.c                  | 9 +++++++++
 tools/lib/perf/include/internal/evlist.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index b8b066d0dc5e..3acbbccc1901 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -738,3 +738,12 @@ int perf_evlist__nr_groups(struct perf_evlist *evlist)
 	}
 	return nr_groups;
 }
+
+void perf_evlist__go_system_wide(struct perf_evlist *evlist, struct perf_evsel *evsel)
+{
+	if (!evsel->system_wide) {
+		evsel->system_wide = true;
+		if (evlist->needs_map_propagation)
+			__perf_evlist__propagate_maps(evlist, evsel);
+	}
+}
diff --git a/tools/lib/perf/include/internal/evlist.h b/tools/lib/perf/include/internal/evlist.h
index 3339bc2f1765..d86ffe8ed483 100644
--- a/tools/lib/perf/include/internal/evlist.h
+++ b/tools/lib/perf/include/internal/evlist.h
@@ -135,4 +135,6 @@ int perf_evlist__id_add_fd(struct perf_evlist *evlist,
 void perf_evlist__reset_id_hash(struct perf_evlist *evlist);
 
 void __perf_evlist__set_leader(struct list_head *list, struct perf_evsel *leader);
+
+void perf_evlist__go_system_wide(struct perf_evlist *evlist, struct perf_evsel *evsel);
 #endif /* __LIBPERF_INTERNAL_EVLIST_H */
-- 
2.30.GIT


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper
  2023-07-15  3:29 [PATCH v2 0/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
  2023-07-15  3:29 ` [PATCH v2 1/7] perf evlist: Add perf_evlist__go_system_wide() helper Yang Jihong
@ 2023-07-15  3:29 ` Yang Jihong
  2023-07-19 16:44   ` Ian Rogers
  2023-07-15  3:29 ` [PATCH v2 3/7] perf record: Move setting dummy tracking before record__init_thread_masks() Yang Jihong
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Yang Jihong @ 2023-07-15  3:29 UTC (permalink / raw)
  To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, irogers, adrian.hunter, kan.liang, james.clark, tmricht,
	ak, anshuman.khandual, linux-kernel, linux-perf-users
  Cc: yangjihong1

Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
tracking to the evlist. We may need to search for the dummy event for
some settings. Therefore, add evlist__findnew_tracking_event() helper.

evlist__findnew_tracking_event() also deal with system_wide maps if
system_wide is true.

Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
 tools/perf/builtin-record.c | 11 +++--------
 tools/perf/util/evlist.c    | 18 ++++++++++++++++++
 tools/perf/util/evlist.h    |  1 +
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index aec18db7ff23..ca83599cc50c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1295,14 +1295,9 @@ static int record__open(struct record *rec)
 	 */
 	if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
 	    perf_pmus__num_core_pmus() > 1) {
-		pos = evlist__get_tracking_event(evlist);
-		if (!evsel__is_dummy_event(pos)) {
-			/* Set up dummy event. */
-			if (evlist__add_dummy(evlist))
-				return -ENOMEM;
-			pos = evlist__last(evlist);
-			evlist__set_tracking_event(evlist, pos);
-		}
+		pos = evlist__findnew_tracking_event(evlist, false);
+		if (!pos)
+			return -ENOMEM;
 
 		/*
 		 * Enable the dummy event when the process is forked for
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 7ef43f72098e..25c3ebe2c2f5 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev
 	tracking_evsel->tracking = true;
 }
 
+struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide)
+{
+	struct evsel *evsel;
+
+	evsel = evlist__get_tracking_event(evlist);
+	if (!evsel__is_dummy_event(evsel)) {
+		evsel = evlist__add_aux_dummy(evlist, system_wide);
+		if (!evsel)
+			return NULL;
+
+		evlist__set_tracking_event(evlist, evsel);
+	} else if (system_wide) {
+		perf_evlist__go_system_wide(&evlist->core, &evsel->core);
+	}
+
+	return evsel;
+}
+
 struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str)
 {
 	struct evsel *evsel;
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 664c6bf7b3e0..98e7ddb2bd30 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr);
 
 struct evsel *evlist__get_tracking_event(struct evlist *evlist);
 void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel);
+struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide);
 
 struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str);
 
-- 
2.30.GIT


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 3/7] perf record: Move setting dummy tracking before record__init_thread_masks()
  2023-07-15  3:29 [PATCH v2 0/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
  2023-07-15  3:29 ` [PATCH v2 1/7] perf evlist: Add perf_evlist__go_system_wide() helper Yang Jihong
  2023-07-15  3:29 ` [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper Yang Jihong
@ 2023-07-15  3:29 ` Yang Jihong
  2023-07-15  3:29 ` [PATCH v2 4/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 27+ messages in thread
From: Yang Jihong @ 2023-07-15  3:29 UTC (permalink / raw)
  To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, irogers, adrian.hunter, kan.liang, james.clark, tmricht,
	ak, anshuman.khandual, linux-kernel, linux-perf-users
  Cc: yangjihong1

When dummy tracking go system wide, the mmap cpu mask is changed.
Therefore, needs to be placed before record__init_thread_masks().
Dummy tracking has been set in record__open(), move it before
record__init_thread_masks() and add a helper for unified processing.

The sys_perf_event_open invoked is as follows:

  # perf --debug verbose=3 record -e cpu-clock -D 100 true
    <SNIP>
    Opening: cpu-clock
    ------------------------------------------------------------
    perf_event_attr:
      type                             1
      size                             136
      { sample_period, sample_freq }   4000
      sample_type                      IP|TID|TIME|PERIOD|IDENTIFIER
      read_format                      ID|LOST
      disabled                         1
      inherit                          1
      freq                             1
      sample_id_all                    1
      exclude_guest                    1
    ------------------------------------------------------------
    sys_perf_event_open: pid 6867  cpu 0  group_fd -1  flags 0x8 = 5
    sys_perf_event_open: pid 6867  cpu 1  group_fd -1  flags 0x8 = 6
    sys_perf_event_open: pid 6867  cpu 2  group_fd -1  flags 0x8 = 7
    sys_perf_event_open: pid 6867  cpu 3  group_fd -1  flags 0x8 = 9
    sys_perf_event_open: pid 6867  cpu 4  group_fd -1  flags 0x8 = 10
    sys_perf_event_open: pid 6867  cpu 5  group_fd -1  flags 0x8 = 11
    sys_perf_event_open: pid 6867  cpu 6  group_fd -1  flags 0x8 = 12
    sys_perf_event_open: pid 6867  cpu 7  group_fd -1  flags 0x8 = 13
    Opening: dummy:u
    ------------------------------------------------------------
    perf_event_attr:
      type                             1
      size                             136
      config                           0x9
      { sample_period, sample_freq }   1
      sample_type                      IP|TID|TIME|IDENTIFIER
      read_format                      ID|LOST
      disabled                         1
      inherit                          1
      exclude_kernel                   1
      exclude_hv                       1
      mmap                             1
      comm                             1
      enable_on_exec                   1
      task                             1
      sample_id_all                    1
      exclude_guest                    1
      mmap2                            1
      comm_exec                        1
      ksymbol                          1
      bpf_event                        1
    ------------------------------------------------------------
    sys_perf_event_open: pid 6867  cpu 0  group_fd -1  flags 0x8 = 14
    sys_perf_event_open: pid 6867  cpu 1  group_fd -1  flags 0x8 = 15
    sys_perf_event_open: pid 6867  cpu 2  group_fd -1  flags 0x8 = 16
    sys_perf_event_open: pid 6867  cpu 3  group_fd -1  flags 0x8 = 17
    sys_perf_event_open: pid 6867  cpu 4  group_fd -1  flags 0x8 = 18
    sys_perf_event_open: pid 6867  cpu 5  group_fd -1  flags 0x8 = 19
    sys_perf_event_open: pid 6867  cpu 6  group_fd -1  flags 0x8 = 20
    sys_perf_event_open: pid 6867  cpu 7  group_fd -1  flags 0x8 = 21
    <SNIP>

Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
 tools/perf/builtin-record.c | 59 +++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ca83599cc50c..ccbcb005e188 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -908,6 +908,37 @@ static int record__config_off_cpu(struct record *rec)
 	return off_cpu_prepare(rec->evlist, &rec->opts.target, &rec->opts);
 }
 
+static int record__config_tracking_events(struct record *rec)
+{
+	struct evsel *evsel;
+	struct evlist *evlist = rec->evlist;
+	struct record_opts *opts = &rec->opts;
+
+	/*
+	 * For initial_delay, system wide or a hybrid system, we need to add a
+	 * dummy event so that we can track PERF_RECORD_MMAP to cover the delay
+	 * of waiting or event synthesis.
+	 */
+	if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
+	    perf_pmus__num_core_pmus() > 1) {
+		evsel = evlist__findnew_tracking_event(evlist, false);
+		if (!evsel)
+			return -ENOMEM;
+
+		/*
+		 * Enable the dummy event when the process is forked for
+		 * initial_delay, immediately for system wide.
+		 */
+		if (opts->target.initial_delay && !evsel->immediate &&
+		    !target__has_cpu(&opts->target))
+			evsel->core.attr.enable_on_exec = 1;
+		else
+			evsel->immediate = 1;
+	}
+
+	return 0;
+}
+
 static bool record__kcore_readable(struct machine *machine)
 {
 	char kcore[PATH_MAX];
@@ -1288,28 +1319,6 @@ static int record__open(struct record *rec)
 	struct record_opts *opts = &rec->opts;
 	int rc = 0;
 
-	/*
-	 * For initial_delay, system wide or a hybrid system, we need to add a
-	 * dummy event so that we can track PERF_RECORD_MMAP to cover the delay
-	 * of waiting or event synthesis.
-	 */
-	if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
-	    perf_pmus__num_core_pmus() > 1) {
-		pos = evlist__findnew_tracking_event(evlist, false);
-		if (!pos)
-			return -ENOMEM;
-
-		/*
-		 * Enable the dummy event when the process is forked for
-		 * initial_delay, immediately for system wide.
-		 */
-		if (opts->target.initial_delay && !pos->immediate &&
-		    !target__has_cpu(&opts->target))
-			pos->core.attr.enable_on_exec = 1;
-		else
-			pos->immediate = 1;
-	}
-
 	evlist__config(evlist, opts, &callchain_param);
 
 	evlist__for_each_entry(evlist, pos) {
@@ -4235,6 +4244,12 @@ int cmd_record(int argc, const char **argv)
 		goto out;
 	}
 
+	err = record__config_tracking_events(rec);
+	if (err) {
+		pr_err("record__config_tracking_events failed, error %d\n", err);
+		goto out;
+	}
+
 	err = record__init_thread_masks(rec);
 	if (err) {
 		pr_err("Failed to initialize parallel data streaming masks\n");
-- 
2.30.GIT


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 4/7] perf record: Track sideband events for all CPUs when tracing selected CPUs
  2023-07-15  3:29 [PATCH v2 0/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
                   ` (2 preceding siblings ...)
  2023-07-15  3:29 ` [PATCH v2 3/7] perf record: Move setting dummy tracking before record__init_thread_masks() Yang Jihong
@ 2023-07-15  3:29 ` Yang Jihong
  2023-07-17 14:25   ` Adrian Hunter
  2023-07-15  3:29 ` [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config Yang Jihong
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Yang Jihong @ 2023-07-15  3:29 UTC (permalink / raw)
  To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, irogers, adrian.hunter, kan.liang, james.clark, tmricht,
	ak, anshuman.khandual, linux-kernel, linux-perf-users
  Cc: yangjihong1

User space tasks can migrate between CPUs, we need to track side-band
events for all CPUs.

The specific scenarios are as follows:

         CPU0                                 CPU1
  perf record -C 0 start
                              taskA starts to be created and executed
                                -> PERF_RECORD_COMM and PERF_RECORD_MMAP
                                   events only deliver to CPU1
                              ......
                                |
                          migrate to CPU0
                                |
  Running on CPU0    <----------/
  ...

  perf record -C 0 stop

Now perf samples the PC of taskA. However, perf does not record the
PERF_RECORD_COMM and PERF_RECORD_MMAP events of taskA.
Therefore, the comm and symbols of taskA cannot be parsed.

The solution is to record sideband events for all CPUs when tracing
selected CPUs. Because this modifies the default behavior, add related
comments to the perf record man page.

The sys_perf_event_open invoked is as follows:

  # perf --debug verbose=3 record -e cpu-clock -C 1 true
  <SNIP>
  Opening: cpu-clock
  ------------------------------------------------------------
  perf_event_attr:
    type                             1
    size                             136
    { sample_period, sample_freq }   4000
    sample_type                      IP|TID|TIME|ID|CPU|PERIOD
    read_format                      ID|LOST
    disabled                         1
    inherit                          1
    freq                             1
    sample_id_all                    1
    exclude_guest                    1
  ------------------------------------------------------------
  sys_perf_event_open: pid -1  cpu 1  group_fd -1  flags 0x8 = 5
  Opening: dummy:HG
  ------------------------------------------------------------
  perf_event_attr:
    type                             1
    size                             136
    config                           0x9
    { sample_period, sample_freq }   1
    sample_type                      IP|TID|TIME|CPU|IDENTIFIER
    read_format                      ID|LOST
    inherit                          1
    exclude_kernel                   1
    exclude_hv                       1
    mmap                             1
    comm                             1
    task                             1
    sample_id_all                    1
    exclude_guest                    1
    mmap2                            1
    comm_exec                        1
    ksymbol                          1
    bpf_event                        1
  ------------------------------------------------------------
  sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 6
  sys_perf_event_open: pid -1  cpu 1  group_fd -1  flags 0x8 = 7
  sys_perf_event_open: pid -1  cpu 2  group_fd -1  flags 0x8 = 9
  sys_perf_event_open: pid -1  cpu 3  group_fd -1  flags 0x8 = 10
  sys_perf_event_open: pid -1  cpu 4  group_fd -1  flags 0x8 = 11
  sys_perf_event_open: pid -1  cpu 5  group_fd -1  flags 0x8 = 12
  sys_perf_event_open: pid -1  cpu 6  group_fd -1  flags 0x8 = 13
  sys_perf_event_open: pid -1  cpu 7  group_fd -1  flags 0x8 = 14
  <SNIP>

Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
 tools/perf/Documentation/perf-record.txt | 3 +++
 tools/perf/builtin-record.c              | 7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 680396c56bd1..dac53ece51ab 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -388,6 +388,9 @@ comma-separated list with no space: 0,1. Ranges of CPUs are specified with -: 0-
 In per-thread mode with inheritance mode on (default), samples are captured only when
 the thread executes on the designated CPUs. Default is to monitor all CPUs.
 
+User space tasks can migrate between CPUs, so when tracing selected CPUs,
+a dummy event is created to track sideband for all CPUs.
+
 -B::
 --no-buildid::
 Do not save the build ids of binaries in the perf.data files. This skips
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ccbcb005e188..4a15b2e06f45 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -921,7 +921,12 @@ static int record__config_tracking_events(struct record *rec)
 	 */
 	if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
 	    perf_pmus__num_core_pmus() > 1) {
-		evsel = evlist__findnew_tracking_event(evlist, false);
+
+		/*
+		 * User space tasks can migrate between CPUs, so when tracing
+		 * selected CPUs, sideband for all CPUs is still needed.
+		 */
+		evsel = evlist__findnew_tracking_event(evlist, !!opts->target.cpu_list);
 		if (!evsel)
 			return -ENOMEM;
 
-- 
2.30.GIT


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config
  2023-07-15  3:29 [PATCH v2 0/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
                   ` (3 preceding siblings ...)
  2023-07-15  3:29 ` [PATCH v2 4/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
@ 2023-07-15  3:29 ` Yang Jihong
  2023-07-17 14:41   ` Adrian Hunter
  2023-07-15  3:29 ` [PATCH v2 6/7] perf test: Update system-wide-dummy attr expected values Yang Jihong
  2023-07-15  3:29 ` [PATCH v2 7/7] perf test: Add test case for record sideband events Yang Jihong
  6 siblings, 1 reply; 27+ messages in thread
From: Yang Jihong @ 2023-07-15  3:29 UTC (permalink / raw)
  To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, irogers, adrian.hunter, kan.liang, james.clark, tmricht,
	ak, anshuman.khandual, linux-kernel, linux-perf-users
  Cc: yangjihong1

The dummp event does not contain sampls data. Therefore, sample_type does
not need to be checked.

Currently, the sample id format of the actual sampling event may be changed
after the dummy event is added.

Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
 tools/perf/util/record.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
index 9eb5c6a08999..0240be3b340f 100644
--- a/tools/perf/util/record.c
+++ b/tools/perf/util/record.c
@@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
 		evlist__for_each_entry(evlist, evsel) {
 			if (evsel->core.attr.sample_type == first->core.attr.sample_type)
 				continue;
+
+			/*
+			 * Skip the sample_type check for the dummy event
+			 * because it does not have any samples anyway.
+			 */
+			if (evsel__is_dummy_event(evsel))
+				continue;
 			use_sample_identifier = perf_can_sample_identifier();
 			break;
 		}
-- 
2.30.GIT


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 6/7] perf test: Update system-wide-dummy attr expected values
  2023-07-15  3:29 [PATCH v2 0/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
                   ` (4 preceding siblings ...)
  2023-07-15  3:29 ` [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config Yang Jihong
@ 2023-07-15  3:29 ` Yang Jihong
  2023-07-15  3:29 ` [PATCH v2 7/7] perf test: Add test case for record sideband events Yang Jihong
  6 siblings, 0 replies; 27+ messages in thread
From: Yang Jihong @ 2023-07-15  3:29 UTC (permalink / raw)
  To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, irogers, adrian.hunter, kan.liang, james.clark, tmricht,
	ak, anshuman.khandual, linux-kernel, linux-perf-users
  Cc: yangjihong1

The perf record uses evlist__add_aux_dummy() instead of evlist__add_dummy()
to add a dummy event. The value of attr needs to be updated.

The perf test result is as follows:

  # ./perf test list  2>&1 | grep 'Setup struct perf_event_attr'
   17: Setup struct perf_event_attr
  # ./perf test 17
   17: Setup struct perf_event_attr                                    : Ok

Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
 tools/perf/tests/attr/system-wide-dummy | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/perf/tests/attr/system-wide-dummy b/tools/perf/tests/attr/system-wide-dummy
index 2f3e3eb728eb..69f61f3271cc 100644
--- a/tools/perf/tests/attr/system-wide-dummy
+++ b/tools/perf/tests/attr/system-wide-dummy
@@ -9,8 +9,8 @@ flags=8
 type=1
 size=136
 config=9
-sample_period=4000
-sample_type=455
+sample_period=1
+sample_type=199
 read_format=4|20
 # Event will be enabled right away.
 disabled=0
@@ -18,12 +18,12 @@ inherit=1
 pinned=0
 exclusive=0
 exclude_user=0
-exclude_kernel=0
-exclude_hv=0
+exclude_kernel=1
+exclude_hv=1
 exclude_idle=0
 mmap=1
 comm=1
-freq=1
+freq=0
 inherit_stat=0
 enable_on_exec=0
 task=1
@@ -32,7 +32,7 @@ precise_ip=0
 mmap_data=0
 sample_id_all=1
 exclude_host=0
-exclude_guest=0
+exclude_guest=1
 exclude_callchain_kernel=0
 exclude_callchain_user=0
 mmap2=1
-- 
2.30.GIT


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 7/7] perf test: Add test case for record sideband events
  2023-07-15  3:29 [PATCH v2 0/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
                   ` (5 preceding siblings ...)
  2023-07-15  3:29 ` [PATCH v2 6/7] perf test: Update system-wide-dummy attr expected values Yang Jihong
@ 2023-07-15  3:29 ` Yang Jihong
  2023-07-19 16:48   ` Ian Rogers
  6 siblings, 1 reply; 27+ messages in thread
From: Yang Jihong @ 2023-07-15  3:29 UTC (permalink / raw)
  To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, irogers, adrian.hunter, kan.liang, james.clark, tmricht,
	ak, anshuman.khandual, linux-kernel, linux-perf-users
  Cc: yangjihong1

Add a new test case to record sideband events for all CPUs when tracing
selected CPUs

Test result:

  # ./perf test list 2>&1 | grep 'perf record sideband tests'
   95: perf record sideband tests
  # ./perf test 95
   95: perf record sideband tests                                      : Ok

Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
 tools/perf/tests/shell/record_tracking.sh | 44 +++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100755 tools/perf/tests/shell/record_tracking.sh

diff --git a/tools/perf/tests/shell/record_tracking.sh b/tools/perf/tests/shell/record_tracking.sh
new file mode 100755
index 000000000000..44fc0af92f81
--- /dev/null
+++ b/tools/perf/tests/shell/record_tracking.sh
@@ -0,0 +1,44 @@
+#!/bin/sh
+# perf record sideband tests
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+
+err=0
+perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
+
+can_cpu_wide()
+{
+    if ! perf record -o ${perfdata} -BN --no-bpf-event -e dummy:u -C $1 true 2>&1 >/dev/null
+    then
+        echo "record sideband test [Skipped cannot record cpu$1]"
+        err=2
+    fi
+
+    rm -f ${perfdata}
+    return $err
+}
+
+test_system_wide_tracking()
+{
+    # Need CPU 0 and CPU 1
+    can_cpu_wide 0 || return 0
+    can_cpu_wide 1 || return 0
+
+    # Record on CPU 0 a task running on CPU 1
+    perf record -BN --no-bpf-event -o ${perfdata} -e dummy:u -C 0 -- taskset --cpu-list 1 true
+
+    # Should get MMAP events from CPU 1
+    mmap_cnt=`perf script -i ${perfdata} --show-mmap-events -C 1 2>/dev/null | grep MMAP | wc -l`
+
+    rm -f ${perfdata}
+
+    if [ ${mmap_cnt} -gt 0 ] ; then
+        return 0
+    fi
+
+    echo "Failed to record MMAP events on CPU 1 when tracing CPU 0"
+    return 1
+}
+
+test_system_wide_tracking
-- 
2.30.GIT


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/7] perf record: Track sideband events for all CPUs when tracing selected CPUs
  2023-07-15  3:29 ` [PATCH v2 4/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
@ 2023-07-17 14:25   ` Adrian Hunter
  2023-07-18  9:07     ` Yang Jihong
  0 siblings, 1 reply; 27+ messages in thread
From: Adrian Hunter @ 2023-07-17 14:25 UTC (permalink / raw)
  To: Yang Jihong, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, irogers, kan.liang,
	james.clark, tmricht, ak, anshuman.khandual, linux-kernel,
	linux-perf-users

On 15/07/23 06:29, Yang Jihong wrote:
> User space tasks can migrate between CPUs, we need to track side-band
> events for all CPUs.
> 
> The specific scenarios are as follows:
> 
>          CPU0                                 CPU1
>   perf record -C 0 start
>                               taskA starts to be created and executed
>                                 -> PERF_RECORD_COMM and PERF_RECORD_MMAP
>                                    events only deliver to CPU1
>                               ......
>                                 |
>                           migrate to CPU0
>                                 |
>   Running on CPU0    <----------/
>   ...
> 
>   perf record -C 0 stop
> 
> Now perf samples the PC of taskA. However, perf does not record the
> PERF_RECORD_COMM and PERF_RECORD_MMAP events of taskA.
> Therefore, the comm and symbols of taskA cannot be parsed.
> 
> The solution is to record sideband events for all CPUs when tracing
> selected CPUs. Because this modifies the default behavior, add related
> comments to the perf record man page.
> 
> The sys_perf_event_open invoked is as follows:
> 
>   # perf --debug verbose=3 record -e cpu-clock -C 1 true
>   <SNIP>
>   Opening: cpu-clock
>   ------------------------------------------------------------
>   perf_event_attr:
>     type                             1
>     size                             136
>     { sample_period, sample_freq }   4000
>     sample_type                      IP|TID|TIME|ID|CPU|PERIOD
>     read_format                      ID|LOST
>     disabled                         1
>     inherit                          1
>     freq                             1
>     sample_id_all                    1
>     exclude_guest                    1
>   ------------------------------------------------------------
>   sys_perf_event_open: pid -1  cpu 1  group_fd -1  flags 0x8 = 5
>   Opening: dummy:HG
>   ------------------------------------------------------------
>   perf_event_attr:
>     type                             1
>     size                             136
>     config                           0x9
>     { sample_period, sample_freq }   1
>     sample_type                      IP|TID|TIME|CPU|IDENTIFIER
>     read_format                      ID|LOST
>     inherit                          1
>     exclude_kernel                   1
>     exclude_hv                       1
>     mmap                             1
>     comm                             1
>     task                             1
>     sample_id_all                    1
>     exclude_guest                    1
>     mmap2                            1
>     comm_exec                        1
>     ksymbol                          1
>     bpf_event                        1
>   ------------------------------------------------------------
>   sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 6
>   sys_perf_event_open: pid -1  cpu 1  group_fd -1  flags 0x8 = 7
>   sys_perf_event_open: pid -1  cpu 2  group_fd -1  flags 0x8 = 9
>   sys_perf_event_open: pid -1  cpu 3  group_fd -1  flags 0x8 = 10
>   sys_perf_event_open: pid -1  cpu 4  group_fd -1  flags 0x8 = 11
>   sys_perf_event_open: pid -1  cpu 5  group_fd -1  flags 0x8 = 12
>   sys_perf_event_open: pid -1  cpu 6  group_fd -1  flags 0x8 = 13
>   sys_perf_event_open: pid -1  cpu 7  group_fd -1  flags 0x8 = 14
>   <SNIP>
> 
> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
> ---
>  tools/perf/Documentation/perf-record.txt | 3 +++
>  tools/perf/builtin-record.c              | 7 ++++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index 680396c56bd1..dac53ece51ab 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -388,6 +388,9 @@ comma-separated list with no space: 0,1. Ranges of CPUs are specified with -: 0-
>  In per-thread mode with inheritance mode on (default), samples are captured only when
>  the thread executes on the designated CPUs. Default is to monitor all CPUs.
>  
> +User space tasks can migrate between CPUs, so when tracing selected CPUs,
> +a dummy event is created to track sideband for all CPUs.
> +
>  -B::
>  --no-buildid::
>  Do not save the build ids of binaries in the perf.data files. This skips
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index ccbcb005e188..4a15b2e06f45 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -921,7 +921,12 @@ static int record__config_tracking_events(struct record *rec)
>  	 */
>  	if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
>  	    perf_pmus__num_core_pmus() > 1) {
> -		evsel = evlist__findnew_tracking_event(evlist, false);
> +
> +		/*
> +		 * User space tasks can migrate between CPUs, so when tracing
> +		 * selected CPUs, sideband for all CPUs is still needed.

So if all (non-dummy) evsel have exclude_user, then system_wide is not needed.

> +		 */
> +		evsel = evlist__findnew_tracking_event(evlist, !!opts->target.cpu_list);
>  		if (!evsel)
>  			return -ENOMEM;
>  


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config
  2023-07-15  3:29 ` [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config Yang Jihong
@ 2023-07-17 14:41   ` Adrian Hunter
  2023-07-18  9:30     ` Yang Jihong
  0 siblings, 1 reply; 27+ messages in thread
From: Adrian Hunter @ 2023-07-17 14:41 UTC (permalink / raw)
  To: Yang Jihong, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, irogers, kan.liang,
	james.clark, tmricht, ak, anshuman.khandual, linux-kernel,
	linux-perf-users

On 15/07/23 06:29, Yang Jihong wrote:
> The dummp event does not contain sampls data. Therefore, sample_type does
> not need to be checked.
> 
> Currently, the sample id format of the actual sampling event may be changed
> after the dummy event is added.
> 
> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
> ---
>  tools/perf/util/record.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
> index 9eb5c6a08999..0240be3b340f 100644
> --- a/tools/perf/util/record.c
> +++ b/tools/perf/util/record.c
> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>  		evlist__for_each_entry(evlist, evsel) {
>  			if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>  				continue;
> +
> +			/*
> +			 * Skip the sample_type check for the dummy event
> +			 * because it does not have any samples anyway.
> +			 */
> +			if (evsel__is_dummy_event(evsel))
> +				continue;

Sideband event records have "ID samples" so the sample type still matters.


>  			use_sample_identifier = perf_can_sample_identifier();
>  			break;
>  		}


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/7] perf record: Track sideband events for all CPUs when tracing selected CPUs
  2023-07-17 14:25   ` Adrian Hunter
@ 2023-07-18  9:07     ` Yang Jihong
  0 siblings, 0 replies; 27+ messages in thread
From: Yang Jihong @ 2023-07-18  9:07 UTC (permalink / raw)
  To: Adrian Hunter, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, irogers, kan.liang,
	james.clark, tmricht, ak, anshuman.khandual, linux-kernel,
	linux-perf-users

Hello,

On 2023/7/17 22:25, Adrian Hunter wrote:
> On 15/07/23 06:29, Yang Jihong wrote:
>> User space tasks can migrate between CPUs, we need to track side-band
>> events for all CPUs.
>>
>> The specific scenarios are as follows:
>>
>>           CPU0                                 CPU1
>>    perf record -C 0 start
>>                                taskA starts to be created and executed
>>                                  -> PERF_RECORD_COMM and PERF_RECORD_MMAP
>>                                     events only deliver to CPU1
>>                                ......
>>                                  |
>>                            migrate to CPU0
>>                                  |
>>    Running on CPU0    <----------/
>>    ...
>>
>>    perf record -C 0 stop
>>
>> Now perf samples the PC of taskA. However, perf does not record the
>> PERF_RECORD_COMM and PERF_RECORD_MMAP events of taskA.
>> Therefore, the comm and symbols of taskA cannot be parsed.
>>
>> The solution is to record sideband events for all CPUs when tracing
>> selected CPUs. Because this modifies the default behavior, add related
>> comments to the perf record man page.
>>
>> The sys_perf_event_open invoked is as follows:
>>
>>    # perf --debug verbose=3 record -e cpu-clock -C 1 true
>>    <SNIP>
>>    Opening: cpu-clock
>>    ------------------------------------------------------------
>>    perf_event_attr:
>>      type                             1
>>      size                             136
>>      { sample_period, sample_freq }   4000
>>      sample_type                      IP|TID|TIME|ID|CPU|PERIOD
>>      read_format                      ID|LOST
>>      disabled                         1
>>      inherit                          1
>>      freq                             1
>>      sample_id_all                    1
>>      exclude_guest                    1
>>    ------------------------------------------------------------
>>    sys_perf_event_open: pid -1  cpu 1  group_fd -1  flags 0x8 = 5
>>    Opening: dummy:HG
>>    ------------------------------------------------------------
>>    perf_event_attr:
>>      type                             1
>>      size                             136
>>      config                           0x9
>>      { sample_period, sample_freq }   1
>>      sample_type                      IP|TID|TIME|CPU|IDENTIFIER
>>      read_format                      ID|LOST
>>      inherit                          1
>>      exclude_kernel                   1
>>      exclude_hv                       1
>>      mmap                             1
>>      comm                             1
>>      task                             1
>>      sample_id_all                    1
>>      exclude_guest                    1
>>      mmap2                            1
>>      comm_exec                        1
>>      ksymbol                          1
>>      bpf_event                        1
>>    ------------------------------------------------------------
>>    sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 6
>>    sys_perf_event_open: pid -1  cpu 1  group_fd -1  flags 0x8 = 7
>>    sys_perf_event_open: pid -1  cpu 2  group_fd -1  flags 0x8 = 9
>>    sys_perf_event_open: pid -1  cpu 3  group_fd -1  flags 0x8 = 10
>>    sys_perf_event_open: pid -1  cpu 4  group_fd -1  flags 0x8 = 11
>>    sys_perf_event_open: pid -1  cpu 5  group_fd -1  flags 0x8 = 12
>>    sys_perf_event_open: pid -1  cpu 6  group_fd -1  flags 0x8 = 13
>>    sys_perf_event_open: pid -1  cpu 7  group_fd -1  flags 0x8 = 14
>>    <SNIP>
>>
>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>> ---
>>   tools/perf/Documentation/perf-record.txt | 3 +++
>>   tools/perf/builtin-record.c              | 7 ++++++-
>>   2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
>> index 680396c56bd1..dac53ece51ab 100644
>> --- a/tools/perf/Documentation/perf-record.txt
>> +++ b/tools/perf/Documentation/perf-record.txt
>> @@ -388,6 +388,9 @@ comma-separated list with no space: 0,1. Ranges of CPUs are specified with -: 0-
>>   In per-thread mode with inheritance mode on (default), samples are captured only when
>>   the thread executes on the designated CPUs. Default is to monitor all CPUs.
>>   
>> +User space tasks can migrate between CPUs, so when tracing selected CPUs,
>> +a dummy event is created to track sideband for all CPUs.
>> +
>>   -B::
>>   --no-buildid::
>>   Do not save the build ids of binaries in the perf.data files. This skips
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index ccbcb005e188..4a15b2e06f45 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -921,7 +921,12 @@ static int record__config_tracking_events(struct record *rec)
>>   	 */
>>   	if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
>>   	    perf_pmus__num_core_pmus() > 1) {
>> -		evsel = evlist__findnew_tracking_event(evlist, false);
>> +
>> +		/*
>> +		 * User space tasks can migrate between CPUs, so when tracing
>> +		 * selected CPUs, sideband for all CPUs is still needed.
> 
> So if all (non-dummy) evsel have exclude_user, then system_wide is not needed.
OK, The previous judgment system_wide is simplified and will be refined 
here.
> 
>> +		 */
>> +		evsel = evlist__findnew_tracking_event(evlist, !!opts->target.cpu_list);
>>   		if (!evsel)
>>   			return -ENOMEM;
>>   
> 
> .
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config
  2023-07-17 14:41   ` Adrian Hunter
@ 2023-07-18  9:30     ` Yang Jihong
  2023-07-18  9:56       ` Adrian Hunter
  0 siblings, 1 reply; 27+ messages in thread
From: Yang Jihong @ 2023-07-18  9:30 UTC (permalink / raw)
  To: Adrian Hunter, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, irogers, kan.liang,
	james.clark, tmricht, ak, anshuman.khandual, linux-kernel,
	linux-perf-users

Hello,

On 2023/7/17 22:41, Adrian Hunter wrote:
> On 15/07/23 06:29, Yang Jihong wrote:
>> The dummp event does not contain sampls data. Therefore, sample_type does
>> not need to be checked.
>>
>> Currently, the sample id format of the actual sampling event may be changed
>> after the dummy event is added.
>>
>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>> ---
>>   tools/perf/util/record.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>> index 9eb5c6a08999..0240be3b340f 100644
>> --- a/tools/perf/util/record.c
>> +++ b/tools/perf/util/record.c
>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>   		evlist__for_each_entry(evlist, evsel) {
>>   			if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>   				continue;
>> +
>> +			/*
>> +			 * Skip the sample_type check for the dummy event
>> +			 * because it does not have any samples anyway.
>> +			 */
>> +			if (evsel__is_dummy_event(evsel))
>> +				continue;
> 
> Sideband event records have "ID samples" so the sample type still matters.
> 
Okay, will remove this patch in next version.

Can I ask a little more about this?

Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for 
samples of type PERF_RECORD_SAMPLE, there may be different record 
formats due to different *sample_type* settings, so the fixed SAMPLE_ID 
  location mode PERF_SAMPLE_NAME is required here.

However, for the sideband event, the samples of the PERF_RECORD_SAMPLE 
type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so 
on). Therefore, the "use sample identifier "check can be skipped here.

That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any 
error, please help to correct it.

*Sideband event records have "ID samples" so the sample type still matters.*

Does this mean that sideband will also record samples of type 
PERF_RECORD_SAMPLE? What exactly is the sampling data?

Thanks,
Yang


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config
  2023-07-18  9:30     ` Yang Jihong
@ 2023-07-18  9:56       ` Adrian Hunter
  2023-07-18 10:17         ` Yang Jihong
  0 siblings, 1 reply; 27+ messages in thread
From: Adrian Hunter @ 2023-07-18  9:56 UTC (permalink / raw)
  To: Yang Jihong, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, irogers, kan.liang,
	james.clark, tmricht, ak, anshuman.khandual, linux-kernel,
	linux-perf-users

On 18/07/23 12:30, Yang Jihong wrote:
> Hello,
> 
> On 2023/7/17 22:41, Adrian Hunter wrote:
>> On 15/07/23 06:29, Yang Jihong wrote:
>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>> not need to be checked.
>>>
>>> Currently, the sample id format of the actual sampling event may be changed
>>> after the dummy event is added.
>>>
>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>> ---
>>>   tools/perf/util/record.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>> index 9eb5c6a08999..0240be3b340f 100644
>>> --- a/tools/perf/util/record.c
>>> +++ b/tools/perf/util/record.c
>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>           evlist__for_each_entry(evlist, evsel) {
>>>               if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>                   continue;
>>> +
>>> +            /*
>>> +             * Skip the sample_type check for the dummy event
>>> +             * because it does not have any samples anyway.
>>> +             */
>>> +            if (evsel__is_dummy_event(evsel))
>>> +                continue;
>>
>> Sideband event records have "ID samples" so the sample type still matters.
>>
> Okay, will remove this patch in next version.
> 
> Can I ask a little more about this?
> 
> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
> 
> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
> 
> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
> 
> *Sideband event records have "ID samples" so the sample type still matters.*
> 
> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?

No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config
  2023-07-18  9:56       ` Adrian Hunter
@ 2023-07-18 10:17         ` Yang Jihong
  2023-07-18 10:29           ` Adrian Hunter
  0 siblings, 1 reply; 27+ messages in thread
From: Yang Jihong @ 2023-07-18 10:17 UTC (permalink / raw)
  To: Adrian Hunter, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, irogers, kan.liang,
	james.clark, tmricht, ak, anshuman.khandual, linux-kernel,
	linux-perf-users

Hello,

On 2023/7/18 17:56, Adrian Hunter wrote:
> On 18/07/23 12:30, Yang Jihong wrote:
>> Hello,
>>
>> On 2023/7/17 22:41, Adrian Hunter wrote:
>>> On 15/07/23 06:29, Yang Jihong wrote:
>>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>>> not need to be checked.
>>>>
>>>> Currently, the sample id format of the actual sampling event may be changed
>>>> after the dummy event is added.
>>>>
>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>> ---
>>>>    tools/perf/util/record.c | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>>> index 9eb5c6a08999..0240be3b340f 100644
>>>> --- a/tools/perf/util/record.c
>>>> +++ b/tools/perf/util/record.c
>>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>>            evlist__for_each_entry(evlist, evsel) {
>>>>                if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>>                    continue;
>>>> +
>>>> +            /*
>>>> +             * Skip the sample_type check for the dummy event
>>>> +             * because it does not have any samples anyway.
>>>> +             */
>>>> +            if (evsel__is_dummy_event(evsel))
>>>> +                continue;
>>>
>>> Sideband event records have "ID samples" so the sample type still matters.
>>>
>> Okay, will remove this patch in next version.
>>
>> Can I ask a little more about this?
>>
>> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
>>
>> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
>>
>> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
>>
>> *Sideband event records have "ID samples" so the sample type still matters.*
>>
>> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?
> 
> No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
> 
I'm sorry, maybe my comments didn't make it clear.
I mean, can we skip the "use_sample_identifier" check here?

That is, set sample_type to *XXX|PERF_SAMPLE_ID* instead of 
*XXX|PERF_SAMPLE_IDENTIFICATION*

Thanks,
Yang

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config
  2023-07-18 10:17         ` Yang Jihong
@ 2023-07-18 10:29           ` Adrian Hunter
  2023-07-18 11:32             ` Yang Jihong
  0 siblings, 1 reply; 27+ messages in thread
From: Adrian Hunter @ 2023-07-18 10:29 UTC (permalink / raw)
  To: Yang Jihong, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, irogers, kan.liang,
	james.clark, tmricht, ak, anshuman.khandual, linux-kernel,
	linux-perf-users

On 18/07/23 13:17, Yang Jihong wrote:
> Hello,
> 
> On 2023/7/18 17:56, Adrian Hunter wrote:
>> On 18/07/23 12:30, Yang Jihong wrote:
>>> Hello,
>>>
>>> On 2023/7/17 22:41, Adrian Hunter wrote:
>>>> On 15/07/23 06:29, Yang Jihong wrote:
>>>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>>>> not need to be checked.
>>>>>
>>>>> Currently, the sample id format of the actual sampling event may be changed
>>>>> after the dummy event is added.
>>>>>
>>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>>> ---
>>>>>    tools/perf/util/record.c | 7 +++++++
>>>>>    1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>>>> index 9eb5c6a08999..0240be3b340f 100644
>>>>> --- a/tools/perf/util/record.c
>>>>> +++ b/tools/perf/util/record.c
>>>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>>>            evlist__for_each_entry(evlist, evsel) {
>>>>>                if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>>>                    continue;
>>>>> +
>>>>> +            /*
>>>>> +             * Skip the sample_type check for the dummy event
>>>>> +             * because it does not have any samples anyway.
>>>>> +             */
>>>>> +            if (evsel__is_dummy_event(evsel))
>>>>> +                continue;
>>>>
>>>> Sideband event records have "ID samples" so the sample type still matters.
>>>>
>>> Okay, will remove this patch in next version.
>>>
>>> Can I ask a little more about this?
>>>
>>> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
>>>
>>> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
>>>
>>> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
>>>
>>> *Sideband event records have "ID samples" so the sample type still matters.*
>>>
>>> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?
>>
>> No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
>>
> I'm sorry, maybe my comments didn't make it clear.
> I mean, can we skip the "use_sample_identifier" check here?
> 
> That is, set sample_type to *XXX|PERF_SAMPLE_ID* instead of *XXX|PERF_SAMPLE_IDENTIFICATION*

In general, when there are different values of sample_type, the PERF_SAMPLE_ID is needed to determine which is which.
But PERF_SAMPLE_ID is not at a fixed position, so the sample_type is needed to find it.  That is why PERF_SAMPLE_IDENTIFIER is better.

Why do want to change it?


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config
  2023-07-18 10:29           ` Adrian Hunter
@ 2023-07-18 11:32             ` Yang Jihong
  2023-07-20  5:41               ` Adrian Hunter
  0 siblings, 1 reply; 27+ messages in thread
From: Yang Jihong @ 2023-07-18 11:32 UTC (permalink / raw)
  To: Adrian Hunter, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, irogers, kan.liang,
	james.clark, tmricht, ak, anshuman.khandual, linux-kernel,
	linux-perf-users

Hello,

On 2023/7/18 18:29, Adrian Hunter wrote:
> On 18/07/23 13:17, Yang Jihong wrote:
>> Hello,
>>
>> On 2023/7/18 17:56, Adrian Hunter wrote:
>>> On 18/07/23 12:30, Yang Jihong wrote:
>>>> Hello,
>>>>
>>>> On 2023/7/17 22:41, Adrian Hunter wrote:
>>>>> On 15/07/23 06:29, Yang Jihong wrote:
>>>>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>>>>> not need to be checked.
>>>>>>
>>>>>> Currently, the sample id format of the actual sampling event may be changed
>>>>>> after the dummy event is added.
>>>>>>
>>>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>>>> ---
>>>>>>     tools/perf/util/record.c | 7 +++++++
>>>>>>     1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>>>>> index 9eb5c6a08999..0240be3b340f 100644
>>>>>> --- a/tools/perf/util/record.c
>>>>>> +++ b/tools/perf/util/record.c
>>>>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>>>>             evlist__for_each_entry(evlist, evsel) {
>>>>>>                 if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>>>>                     continue;
>>>>>> +
>>>>>> +            /*
>>>>>> +             * Skip the sample_type check for the dummy event
>>>>>> +             * because it does not have any samples anyway.
>>>>>> +             */
>>>>>> +            if (evsel__is_dummy_event(evsel))
>>>>>> +                continue;
>>>>>
>>>>> Sideband event records have "ID samples" so the sample type still matters.
>>>>>
>>>> Okay, will remove this patch in next version.
>>>>
>>>> Can I ask a little more about this?
>>>>
>>>> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
>>>>
>>>> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
>>>>
>>>> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
>>>>
>>>> *Sideband event records have "ID samples" so the sample type still matters.*
>>>>
>>>> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?
>>>
>>> No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
>>>
>> I'm sorry, maybe my comments didn't make it clear.
>> I mean, can we skip the "use_sample_identifier" check here?
>>
>> That is, set sample_type to *XXX|PERF_SAMPLE_ID* instead of *XXX|PERF_SAMPLE_IDENTIFICATION*
> 
> In general, when there are different values of sample_type, the PERF_SAMPLE_ID is needed to determine which is which.
> But PERF_SAMPLE_ID is not at a fixed position, so the sample_type is needed to find it.  That is why PERF_SAMPLE_IDENTIFIER is better.
> 
> Why do want to change it?

Without this patch, we now add a system_wide tracking event and modify 
the sample_type of the  actual sample event.

For example, when we run:
   # perf record -e cycles -C 0

1. The default sample_type of the "cycles" is IP|TID|TIME|CPU|PERIOD.
2. Then add a system_wide sideband event whose sample_type is 
IP|TID|TIME|CPU.
3. The two sample_types are different.
4. Therefore, the evlist__config adds a PERF_SAMPLE_IDENTIFICATION to 
both sample_types instead of PERF_SAMPLE_ID.

evlist__config {
         ...
          } else if (evlist->core.nr_entries > 1) {
          // One is cycles and the other is sideband . 

                  struct evsel *first = evlist__first(evlist); 

 

                  evlist__for_each_entry(evlist, evsel) { 

                          if (evsel->core.attr.sample_type == 
first->core.attr.sample_type)
                                  continue; 

                          use_sample_identifier = 
perf_can_sample_identifier();
                          // the sample_type of cycles is different from 
that of sideband.
						 // Therefore, use_sample_identifier is set to true.
                          break; 

                  } 

                  sample_id = true; 

          } 

 

          if (sample_id) { 

                  evlist__for_each_entry(evlist, evsel) 

                          evsel__set_sample_id(evsel, 
use_sample_identifier);
                          // both cycles and sideband set 
PERF_SAMPLE_IDENTIFICATION  						
          } 

         ...
}

The comparison of the sideband event sample_type is skipped so that the 
sample_type of the original cycles is not changed.

It does not seem necessary to compare the sample_type of sidebband 
event. It is not an actual sample event, so I'd like to confirm that.

If the change is as expected and necessary, then I'll remove the patch.

Thanks,
Yang


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper
  2023-07-15  3:29 ` [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper Yang Jihong
@ 2023-07-19 16:44   ` Ian Rogers
  2023-07-19 16:59     ` Adrian Hunter
  2023-07-20  7:23     ` Yang Jihong
  0 siblings, 2 replies; 27+ messages in thread
From: Ian Rogers @ 2023-07-19 16:44 UTC (permalink / raw)
  To: Yang Jihong
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, adrian.hunter, kan.liang, james.clark, tmricht, ak,
	anshuman.khandual, linux-kernel, linux-perf-users

On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote:
>
> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
> tracking to the evlist. We may need to search for the dummy event for
> some settings. Therefore, add evlist__findnew_tracking_event() helper.
>
> evlist__findnew_tracking_event() also deal with system_wide maps if
> system_wide is true.

I'm wondering if we can simplify the naming in the API, we have "dummy
event" which makes sense as we literally call the event "dummy",
"sideband" which refers to the kind of samples/events the dummy event
will record but "tracking" I think tends to get used as a verb rather
than a noun. So I think evlist__findnew_tracking_event should be
evlist__findnew_dummy_event.

Thanks,
Ian

> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
> ---
>  tools/perf/builtin-record.c | 11 +++--------
>  tools/perf/util/evlist.c    | 18 ++++++++++++++++++
>  tools/perf/util/evlist.h    |  1 +
>  3 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index aec18db7ff23..ca83599cc50c 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1295,14 +1295,9 @@ static int record__open(struct record *rec)
>          */
>         if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
>             perf_pmus__num_core_pmus() > 1) {
> -               pos = evlist__get_tracking_event(evlist);
> -               if (!evsel__is_dummy_event(pos)) {
> -                       /* Set up dummy event. */
> -                       if (evlist__add_dummy(evlist))
> -                               return -ENOMEM;
> -                       pos = evlist__last(evlist);
> -                       evlist__set_tracking_event(evlist, pos);
> -               }
> +               pos = evlist__findnew_tracking_event(evlist, false);
> +               if (!pos)
> +                       return -ENOMEM;
>
>                 /*
>                  * Enable the dummy event when the process is forked for
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 7ef43f72098e..25c3ebe2c2f5 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev
>         tracking_evsel->tracking = true;
>  }
>
> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide)
> +{
> +       struct evsel *evsel;
> +
> +       evsel = evlist__get_tracking_event(evlist);
> +       if (!evsel__is_dummy_event(evsel)) {
> +               evsel = evlist__add_aux_dummy(evlist, system_wide);
> +               if (!evsel)
> +                       return NULL;
> +
> +               evlist__set_tracking_event(evlist, evsel);
> +       } else if (system_wide) {
> +               perf_evlist__go_system_wide(&evlist->core, &evsel->core);
> +       }
> +
> +       return evsel;
> +}
> +
>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str)
>  {
>         struct evsel *evsel;
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 664c6bf7b3e0..98e7ddb2bd30 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr);
>
>  struct evsel *evlist__get_tracking_event(struct evlist *evlist);
>  void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel);
> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide);
>
>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str);
>
> --
> 2.30.GIT
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 7/7] perf test: Add test case for record sideband events
  2023-07-15  3:29 ` [PATCH v2 7/7] perf test: Add test case for record sideband events Yang Jihong
@ 2023-07-19 16:48   ` Ian Rogers
  2023-07-20  7:27     ` Yang Jihong
  0 siblings, 1 reply; 27+ messages in thread
From: Ian Rogers @ 2023-07-19 16:48 UTC (permalink / raw)
  To: Yang Jihong
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, adrian.hunter, kan.liang, james.clark, tmricht, ak,
	anshuman.khandual, linux-kernel, linux-perf-users

On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote:
>
> Add a new test case to record sideband events for all CPUs when tracing
> selected CPUs
>
> Test result:
>
>   # ./perf test list 2>&1 | grep 'perf record sideband tests'
>    95: perf record sideband tests
>   # ./perf test 95
>    95: perf record sideband tests                                      : Ok
>
> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
> ---
>  tools/perf/tests/shell/record_tracking.sh | 44 +++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100755 tools/perf/tests/shell/record_tracking.sh
>
> diff --git a/tools/perf/tests/shell/record_tracking.sh b/tools/perf/tests/shell/record_tracking.sh

Could this be record_sideband.sh ? It will be more consistent with the
test output...

Thanks,
Ian

> new file mode 100755
> index 000000000000..44fc0af92f81
> --- /dev/null
> +++ b/tools/perf/tests/shell/record_tracking.sh
> @@ -0,0 +1,44 @@
> +#!/bin/sh
> +# perf record sideband tests
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +err=0
> +perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> +
> +can_cpu_wide()
> +{
> +    if ! perf record -o ${perfdata} -BN --no-bpf-event -e dummy:u -C $1 true 2>&1 >/dev/null
> +    then
> +        echo "record sideband test [Skipped cannot record cpu$1]"
> +        err=2
> +    fi
> +
> +    rm -f ${perfdata}
> +    return $err
> +}
> +
> +test_system_wide_tracking()
> +{
> +    # Need CPU 0 and CPU 1
> +    can_cpu_wide 0 || return 0
> +    can_cpu_wide 1 || return 0
> +
> +    # Record on CPU 0 a task running on CPU 1
> +    perf record -BN --no-bpf-event -o ${perfdata} -e dummy:u -C 0 -- taskset --cpu-list 1 true
> +
> +    # Should get MMAP events from CPU 1
> +    mmap_cnt=`perf script -i ${perfdata} --show-mmap-events -C 1 2>/dev/null | grep MMAP | wc -l`
> +
> +    rm -f ${perfdata}
> +
> +    if [ ${mmap_cnt} -gt 0 ] ; then
> +        return 0
> +    fi
> +
> +    echo "Failed to record MMAP events on CPU 1 when tracing CPU 0"
> +    return 1
> +}
> +
> +test_system_wide_tracking
> --
> 2.30.GIT
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper
  2023-07-19 16:44   ` Ian Rogers
@ 2023-07-19 16:59     ` Adrian Hunter
  2023-07-19 17:12       ` Ian Rogers
  2023-07-20  7:23     ` Yang Jihong
  1 sibling, 1 reply; 27+ messages in thread
From: Adrian Hunter @ 2023-07-19 16:59 UTC (permalink / raw)
  To: Ian Rogers, Yang Jihong
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, kan.liang, james.clark, tmricht, ak, anshuman.khandual,
	linux-kernel, linux-perf-users

On 19/07/23 19:44, Ian Rogers wrote:
> On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote:
>>
>> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
>> tracking to the evlist. We may need to search for the dummy event for
>> some settings. Therefore, add evlist__findnew_tracking_event() helper.
>>
>> evlist__findnew_tracking_event() also deal with system_wide maps if
>> system_wide is true.
> 
> I'm wondering if we can simplify the naming in the API, we have "dummy
> event" which makes sense as we literally call the event "dummy",
> "sideband" which refers to the kind of samples/events the dummy event
> will record but "tracking" I think tends to get used as a verb rather
> than a noun. So I think evlist__findnew_tracking_event should be
> evlist__findnew_dummy_event.

Except the tracking event has "tracking" set but there can be other dummy
events that do not.

> 
> Thanks,
> Ian
> 
>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>> ---
>>  tools/perf/builtin-record.c | 11 +++--------
>>  tools/perf/util/evlist.c    | 18 ++++++++++++++++++
>>  tools/perf/util/evlist.h    |  1 +
>>  3 files changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index aec18db7ff23..ca83599cc50c 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -1295,14 +1295,9 @@ static int record__open(struct record *rec)
>>          */
>>         if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
>>             perf_pmus__num_core_pmus() > 1) {
>> -               pos = evlist__get_tracking_event(evlist);
>> -               if (!evsel__is_dummy_event(pos)) {
>> -                       /* Set up dummy event. */
>> -                       if (evlist__add_dummy(evlist))
>> -                               return -ENOMEM;
>> -                       pos = evlist__last(evlist);
>> -                       evlist__set_tracking_event(evlist, pos);
>> -               }
>> +               pos = evlist__findnew_tracking_event(evlist, false);
>> +               if (!pos)
>> +                       return -ENOMEM;
>>
>>                 /*
>>                  * Enable the dummy event when the process is forked for
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index 7ef43f72098e..25c3ebe2c2f5 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev
>>         tracking_evsel->tracking = true;
>>  }
>>
>> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide)
>> +{
>> +       struct evsel *evsel;
>> +
>> +       evsel = evlist__get_tracking_event(evlist);
>> +       if (!evsel__is_dummy_event(evsel)) {
>> +               evsel = evlist__add_aux_dummy(evlist, system_wide);
>> +               if (!evsel)
>> +                       return NULL;
>> +
>> +               evlist__set_tracking_event(evlist, evsel);
>> +       } else if (system_wide) {
>> +               perf_evlist__go_system_wide(&evlist->core, &evsel->core);
>> +       }
>> +
>> +       return evsel;
>> +}
>> +
>>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str)
>>  {
>>         struct evsel *evsel;
>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
>> index 664c6bf7b3e0..98e7ddb2bd30 100644
>> --- a/tools/perf/util/evlist.h
>> +++ b/tools/perf/util/evlist.h
>> @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr);
>>
>>  struct evsel *evlist__get_tracking_event(struct evlist *evlist);
>>  void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel);
>> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide);
>>
>>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str);
>>
>> --
>> 2.30.GIT
>>


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper
  2023-07-19 16:59     ` Adrian Hunter
@ 2023-07-19 17:12       ` Ian Rogers
  2023-08-14  7:40         ` Adrian Hunter
  0 siblings, 1 reply; 27+ messages in thread
From: Ian Rogers @ 2023-07-19 17:12 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Yang Jihong, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, kan.liang, james.clark,
	tmricht, ak, anshuman.khandual, linux-kernel, linux-perf-users

On Wed, Jul 19, 2023 at 9:59 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 19/07/23 19:44, Ian Rogers wrote:
> > On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote:
> >>
> >> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
> >> tracking to the evlist. We may need to search for the dummy event for
> >> some settings. Therefore, add evlist__findnew_tracking_event() helper.
> >>
> >> evlist__findnew_tracking_event() also deal with system_wide maps if
> >> system_wide is true.
> >
> > I'm wondering if we can simplify the naming in the API, we have "dummy
> > event" which makes sense as we literally call the event "dummy",
> > "sideband" which refers to the kind of samples/events the dummy event
> > will record but "tracking" I think tends to get used as a verb rather
> > than a noun. So I think evlist__findnew_tracking_event should be
> > evlist__findnew_dummy_event.
>
> Except the tracking event has "tracking" set but there can be other dummy
> events that do not.

Thanks! I'm wondering what a dummy event without tracking would be
used for - do you have a reference? I don't see anything in
perf_event.h calling things like mmap2/comm in perf_event_attr
tracking. I'm not a fan of dummy due to it not being intention
revealing. Perhaps if we could go back in time  we could call the
event "sideband", maybe we should migrate to this. We have other
non-obvious uses of the term dummy like in cpumap:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/lib/perf/include/perf/cpumap.h?h=perf-tools-next#n24
If tracking can be on any event then does that make the functions
behavior more ambiguous if it means dummy+tracking not <any
event>+tracking?

Thanks,
Ian

> >
> > Thanks,
> > Ian
> >
> >> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
> >> ---
> >>  tools/perf/builtin-record.c | 11 +++--------
> >>  tools/perf/util/evlist.c    | 18 ++++++++++++++++++
> >>  tools/perf/util/evlist.h    |  1 +
> >>  3 files changed, 22 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> >> index aec18db7ff23..ca83599cc50c 100644
> >> --- a/tools/perf/builtin-record.c
> >> +++ b/tools/perf/builtin-record.c
> >> @@ -1295,14 +1295,9 @@ static int record__open(struct record *rec)
> >>          */
> >>         if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
> >>             perf_pmus__num_core_pmus() > 1) {
> >> -               pos = evlist__get_tracking_event(evlist);
> >> -               if (!evsel__is_dummy_event(pos)) {
> >> -                       /* Set up dummy event. */
> >> -                       if (evlist__add_dummy(evlist))
> >> -                               return -ENOMEM;
> >> -                       pos = evlist__last(evlist);
> >> -                       evlist__set_tracking_event(evlist, pos);
> >> -               }
> >> +               pos = evlist__findnew_tracking_event(evlist, false);
> >> +               if (!pos)
> >> +                       return -ENOMEM;
> >>
> >>                 /*
> >>                  * Enable the dummy event when the process is forked for
> >> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >> index 7ef43f72098e..25c3ebe2c2f5 100644
> >> --- a/tools/perf/util/evlist.c
> >> +++ b/tools/perf/util/evlist.c
> >> @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev
> >>         tracking_evsel->tracking = true;
> >>  }
> >>
> >> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide)
> >> +{
> >> +       struct evsel *evsel;
> >> +
> >> +       evsel = evlist__get_tracking_event(evlist);
> >> +       if (!evsel__is_dummy_event(evsel)) {
> >> +               evsel = evlist__add_aux_dummy(evlist, system_wide);
> >> +               if (!evsel)
> >> +                       return NULL;
> >> +
> >> +               evlist__set_tracking_event(evlist, evsel);
> >> +       } else if (system_wide) {
> >> +               perf_evlist__go_system_wide(&evlist->core, &evsel->core);
> >> +       }
> >> +
> >> +       return evsel;
> >> +}
> >> +
> >>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str)
> >>  {
> >>         struct evsel *evsel;
> >> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> >> index 664c6bf7b3e0..98e7ddb2bd30 100644
> >> --- a/tools/perf/util/evlist.h
> >> +++ b/tools/perf/util/evlist.h
> >> @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr);
> >>
> >>  struct evsel *evlist__get_tracking_event(struct evlist *evlist);
> >>  void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel);
> >> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide);
> >>
> >>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str);
> >>
> >> --
> >> 2.30.GIT
> >>
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config
  2023-07-18 11:32             ` Yang Jihong
@ 2023-07-20  5:41               ` Adrian Hunter
  2023-07-20  7:25                 ` Yang Jihong
  0 siblings, 1 reply; 27+ messages in thread
From: Adrian Hunter @ 2023-07-20  5:41 UTC (permalink / raw)
  To: Yang Jihong, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, irogers, kan.liang,
	james.clark, tmricht, ak, anshuman.khandual, linux-kernel,
	linux-perf-users

On 18/07/23 14:32, Yang Jihong wrote:
> Hello,
> 
> On 2023/7/18 18:29, Adrian Hunter wrote:
>> On 18/07/23 13:17, Yang Jihong wrote:
>>> Hello,
>>>
>>> On 2023/7/18 17:56, Adrian Hunter wrote:
>>>> On 18/07/23 12:30, Yang Jihong wrote:
>>>>> Hello,
>>>>>
>>>>> On 2023/7/17 22:41, Adrian Hunter wrote:
>>>>>> On 15/07/23 06:29, Yang Jihong wrote:
>>>>>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>>>>>> not need to be checked.
>>>>>>>
>>>>>>> Currently, the sample id format of the actual sampling event may be changed
>>>>>>> after the dummy event is added.
>>>>>>>
>>>>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>>>>> ---
>>>>>>>     tools/perf/util/record.c | 7 +++++++
>>>>>>>     1 file changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>>>>>> index 9eb5c6a08999..0240be3b340f 100644
>>>>>>> --- a/tools/perf/util/record.c
>>>>>>> +++ b/tools/perf/util/record.c
>>>>>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>>>>>             evlist__for_each_entry(evlist, evsel) {
>>>>>>>                 if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>>>>>                     continue;
>>>>>>> +
>>>>>>> +            /*
>>>>>>> +             * Skip the sample_type check for the dummy event
>>>>>>> +             * because it does not have any samples anyway.
>>>>>>> +             */
>>>>>>> +            if (evsel__is_dummy_event(evsel))
>>>>>>> +                continue;
>>>>>>
>>>>>> Sideband event records have "ID samples" so the sample type still matters.
>>>>>>
>>>>> Okay, will remove this patch in next version.
>>>>>
>>>>> Can I ask a little more about this?
>>>>>
>>>>> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
>>>>>
>>>>> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
>>>>>
>>>>> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
>>>>>
>>>>> *Sideband event records have "ID samples" so the sample type still matters.*
>>>>>
>>>>> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?
>>>>
>>>> No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
>>>>
>>> I'm sorry, maybe my comments didn't make it clear.
>>> I mean, can we skip the "use_sample_identifier" check here?
>>>
>>> That is, set sample_type to *XXX|PERF_SAMPLE_ID* instead of *XXX|PERF_SAMPLE_IDENTIFICATION*
>>
>> In general, when there are different values of sample_type, the PERF_SAMPLE_ID is needed to determine which is which.
>> But PERF_SAMPLE_ID is not at a fixed position, so the sample_type is needed to find it.  That is why PERF_SAMPLE_IDENTIFIER is better.
>>
>> Why do want to change it?
> 
> Without this patch, we now add a system_wide tracking event and modify the sample_type of the  actual sample event.
> 
> For example, when we run:
>   # perf record -e cycles -C 0
> 
> 1. The default sample_type of the "cycles" is IP|TID|TIME|CPU|PERIOD.
> 2. Then add a system_wide sideband event whose sample_type is IP|TID|TIME|CPU.
> 3. The two sample_types are different.
> 4. Therefore, the evlist__config adds a PERF_SAMPLE_IDENTIFICATION to both sample_types instead of PERF_SAMPLE_ID.
> 
> evlist__config {
>         ...
>          } else if (evlist->core.nr_entries > 1) {
>          // One is cycles and the other is sideband .
>                  struct evsel *first = evlist__first(evlist);
> 
> 
>                  evlist__for_each_entry(evlist, evsel) {
>                          if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>                                  continue;
>                          use_sample_identifier = perf_can_sample_identifier();
>                          // the sample_type of cycles is different from that of sideband.
>                          // Therefore, use_sample_identifier is set to true.
>                          break;
>                  }
>                  sample_id = true;
>          }
> 
> 
>          if (sample_id) {
>                  evlist__for_each_entry(evlist, evsel)
>                          evsel__set_sample_id(evsel, use_sample_identifier);
>                          // both cycles and sideband set PERF_SAMPLE_IDENTIFICATION                         
>          }
>         ...
> }
> 
> The comparison of the sideband event sample_type is skipped so that the sample_type of the original cycles is not changed.
> 
> It does not seem necessary to compare the sample_type of sidebband event. It is not an actual sample event, so I'd like to confirm that.

It is necessary.  The sample type is used to parse ID samples
that are part of e.g. MMAP events - refer perf_evsel__parse_id_sample()

We could teach perf to handle dummy events differently because they
do not use all the sample_type bits (only the ones in perf_evsel__parse_id_sample())
but that is probably not backward compatible.

The only value in that would be to make it work without
PERF_SAMPLE_ID or PERF_SAMPLE_IDENTIFIER because that would
save 8-bytes per event record.

Otherwise there is no benefit to prefer PERF_SAMPLE_ID over
PERF_SAMPLE_IDENTIFIER except backward compatibility with
some other tool that doesn't know about PERF_SAMPLE_IDENTIFIER.

> 
> If the change is as expected and necessary, then I'll remove the patch.
> 
> Thanks,
> Yang
> 


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper
  2023-07-19 16:44   ` Ian Rogers
  2023-07-19 16:59     ` Adrian Hunter
@ 2023-07-20  7:23     ` Yang Jihong
  2023-07-28 16:40       ` Ian Rogers
  1 sibling, 1 reply; 27+ messages in thread
From: Yang Jihong @ 2023-07-20  7:23 UTC (permalink / raw)
  To: Ian Rogers
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, adrian.hunter, kan.liang, james.clark, tmricht, ak,
	anshuman.khandual, linux-kernel, linux-perf-users

Hello,

On 2023/7/20 0:44, Ian Rogers wrote:
> On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote:
>>
>> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
>> tracking to the evlist. We may need to search for the dummy event for
>> some settings. Therefore, add evlist__findnew_tracking_event() helper.
>>
>> evlist__findnew_tracking_event() also deal with system_wide maps if
>> system_wide is true.
> 
> I'm wondering if we can simplify the naming in the API, we have "dummy
> event" which makes sense as we literally call the event "dummy",
> "sideband" which refers to the kind of samples/events the dummy event
> will record but "tracking" I think tends to get used as a verb rather
> than a noun. So I think evlist__findnew_tracking_event should be
> evlist__findnew_dummy_event.
> 
Uh, from the discussion that followed, it seems that there is no 
consensus yet...
If there is a clear consensus on whether to use "dummy event" or 
"tracking event", I will change the name of the API.

I think sideband event is equivalent to tracking event (refer 
evsel__config(), tracking events include task, mmap, mmap2, and comm 
sideband events, which are all sideband).

tracking event are instances of dummy event. For example, we create 
another dummy event to record the text poke event of ksymbol (refer perf 
record --kcore).

An evlist contains only one tracking event, but can contain multiple 
dummy events.

Thanks,
Yang

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config
  2023-07-20  5:41               ` Adrian Hunter
@ 2023-07-20  7:25                 ` Yang Jihong
  0 siblings, 0 replies; 27+ messages in thread
From: Yang Jihong @ 2023-07-20  7:25 UTC (permalink / raw)
  To: Adrian Hunter, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, irogers, kan.liang,
	james.clark, tmricht, ak, anshuman.khandual, linux-kernel,
	linux-perf-users

Hello,

On 2023/7/20 13:41, Adrian Hunter wrote:
> On 18/07/23 14:32, Yang Jihong wrote:
>> Hello,
>>
>> On 2023/7/18 18:29, Adrian Hunter wrote:
>>> On 18/07/23 13:17, Yang Jihong wrote:
>>>> Hello,
>>>>
>>>> On 2023/7/18 17:56, Adrian Hunter wrote:
>>>>> On 18/07/23 12:30, Yang Jihong wrote:
>>>>>> Hello,
>>>>>>
>>>>>> On 2023/7/17 22:41, Adrian Hunter wrote:
>>>>>>> On 15/07/23 06:29, Yang Jihong wrote:
>>>>>>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>>>>>>> not need to be checked.
>>>>>>>>
>>>>>>>> Currently, the sample id format of the actual sampling event may be changed
>>>>>>>> after the dummy event is added.
>>>>>>>>
>>>>>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>>>>>> ---
>>>>>>>>      tools/perf/util/record.c | 7 +++++++
>>>>>>>>      1 file changed, 7 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>>>>>>> index 9eb5c6a08999..0240be3b340f 100644
>>>>>>>> --- a/tools/perf/util/record.c
>>>>>>>> +++ b/tools/perf/util/record.c
>>>>>>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>>>>>>              evlist__for_each_entry(evlist, evsel) {
>>>>>>>>                  if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>>>>>>                      continue;
>>>>>>>> +
>>>>>>>> +            /*
>>>>>>>> +             * Skip the sample_type check for the dummy event
>>>>>>>> +             * because it does not have any samples anyway.
>>>>>>>> +             */
>>>>>>>> +            if (evsel__is_dummy_event(evsel))
>>>>>>>> +                continue;
>>>>>>>
>>>>>>> Sideband event records have "ID samples" so the sample type still matters.
>>>>>>>
>>>>>> Okay, will remove this patch in next version.
>>>>>>
>>>>>> Can I ask a little more about this?
>>>>>>
>>>>>> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
>>>>>>
>>>>>> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
>>>>>>
>>>>>> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
>>>>>>
>>>>>> *Sideband event records have "ID samples" so the sample type still matters.*
>>>>>>
>>>>>> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?
>>>>>
>>>>> No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
>>>>>
>>>> I'm sorry, maybe my comments didn't make it clear.
>>>> I mean, can we skip the "use_sample_identifier" check here?
>>>>
>>>> That is, set sample_type to *XXX|PERF_SAMPLE_ID* instead of *XXX|PERF_SAMPLE_IDENTIFICATION*
>>>
>>> In general, when there are different values of sample_type, the PERF_SAMPLE_ID is needed to determine which is which.
>>> But PERF_SAMPLE_ID is not at a fixed position, so the sample_type is needed to find it.  That is why PERF_SAMPLE_IDENTIFIER is better.
>>>
>>> Why do want to change it?
>>
>> Without this patch, we now add a system_wide tracking event and modify the sample_type of the  actual sample event.
>>
>> For example, when we run:
>>    # perf record -e cycles -C 0
>>
>> 1. The default sample_type of the "cycles" is IP|TID|TIME|CPU|PERIOD.
>> 2. Then add a system_wide sideband event whose sample_type is IP|TID|TIME|CPU.
>> 3. The two sample_types are different.
>> 4. Therefore, the evlist__config adds a PERF_SAMPLE_IDENTIFICATION to both sample_types instead of PERF_SAMPLE_ID.
>>
>> evlist__config {
>>          ...
>>           } else if (evlist->core.nr_entries > 1) {
>>           // One is cycles and the other is sideband .
>>                   struct evsel *first = evlist__first(evlist);
>>
>>
>>                   evlist__for_each_entry(evlist, evsel) {
>>                           if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>                                   continue;
>>                           use_sample_identifier = perf_can_sample_identifier();
>>                           // the sample_type of cycles is different from that of sideband.
>>                           // Therefore, use_sample_identifier is set to true.
>>                           break;
>>                   }
>>                   sample_id = true;
>>           }
>>
>>
>>           if (sample_id) {
>>                   evlist__for_each_entry(evlist, evsel)
>>                           evsel__set_sample_id(evsel, use_sample_identifier);
>>                           // both cycles and sideband set PERF_SAMPLE_IDENTIFICATION
>>           }
>>          ...
>> }
>>
>> The comparison of the sideband event sample_type is skipped so that the sample_type of the original cycles is not changed.
>>
>> It does not seem necessary to compare the sample_type of sidebband event. It is not an actual sample event, so I'd like to confirm that.
> 
> It is necessary.  The sample type is used to parse ID samples
> that are part of e.g. MMAP events - refer perf_evsel__parse_id_sample()
> 
> We could teach perf to handle dummy events differently because they
> do not use all the sample_type bits (only the ones in perf_evsel__parse_id_sample())
> but that is probably not backward compatible.
> 
> The only value in that would be to make it work without
> PERF_SAMPLE_ID or PERF_SAMPLE_IDENTIFIER because that would
> save 8-bytes per event record.
> 
> Otherwise there is no benefit to prefer PERF_SAMPLE_ID over
> PERF_SAMPLE_IDENTIFIER except backward compatibility with
> some other tool that doesn't know about PERF_SAMPLE_IDENTIFIER.
> 
Thanks for detailed instructions, I understand, OK, will remove this 
patch in the next version.

Thanks,
Yang

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 7/7] perf test: Add test case for record sideband events
  2023-07-19 16:48   ` Ian Rogers
@ 2023-07-20  7:27     ` Yang Jihong
  0 siblings, 0 replies; 27+ messages in thread
From: Yang Jihong @ 2023-07-20  7:27 UTC (permalink / raw)
  To: Ian Rogers
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, adrian.hunter, kan.liang, james.clark, tmricht, ak,
	anshuman.khandual, linux-kernel, linux-perf-users

Hello,

On 2023/7/20 0:48, Ian Rogers wrote:
> On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote:
>>
>> Add a new test case to record sideband events for all CPUs when tracing
>> selected CPUs
>>
>> Test result:
>>
>>    # ./perf test list 2>&1 | grep 'perf record sideband tests'
>>     95: perf record sideband tests
>>    # ./perf test 95
>>     95: perf record sideband tests                                      : Ok
>>
>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>> ---
>>   tools/perf/tests/shell/record_tracking.sh | 44 +++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>   create mode 100755 tools/perf/tests/shell/record_tracking.sh
>>
>> diff --git a/tools/perf/tests/shell/record_tracking.sh b/tools/perf/tests/shell/record_tracking.sh
> 
> Could this be record_sideband.sh ? It will be more consistent with the
> test output...
> 
OK, will be changed to "record_sideband.sh" in the next version.

Thanks,
Yang

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper
  2023-07-20  7:23     ` Yang Jihong
@ 2023-07-28 16:40       ` Ian Rogers
  2023-07-29  2:10         ` Yang Jihong
  0 siblings, 1 reply; 27+ messages in thread
From: Ian Rogers @ 2023-07-28 16:40 UTC (permalink / raw)
  To: Yang Jihong
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, adrian.hunter, kan.liang, james.clark, tmricht, ak,
	anshuman.khandual, linux-kernel, linux-perf-users

On Thu, Jul 20, 2023 at 12:24 AM Yang Jihong <yangjihong1@huawei.com> wrote:
>
> Hello,
>
> On 2023/7/20 0:44, Ian Rogers wrote:
> > On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote:
> >>
> >> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
> >> tracking to the evlist. We may need to search for the dummy event for
> >> some settings. Therefore, add evlist__findnew_tracking_event() helper.
> >>
> >> evlist__findnew_tracking_event() also deal with system_wide maps if
> >> system_wide is true.
> >
> > I'm wondering if we can simplify the naming in the API, we have "dummy
> > event" which makes sense as we literally call the event "dummy",
> > "sideband" which refers to the kind of samples/events the dummy event
> > will record but "tracking" I think tends to get used as a verb rather
> > than a noun. So I think evlist__findnew_tracking_event should be
> > evlist__findnew_dummy_event.
> >
> Uh, from the discussion that followed, it seems that there is no
> consensus yet...
> If there is a clear consensus on whether to use "dummy event" or
> "tracking event", I will change the name of the API.
>
> I think sideband event is equivalent to tracking event (refer
> evsel__config(), tracking events include task, mmap, mmap2, and comm
> sideband events, which are all sideband).
>
> tracking event are instances of dummy event. For example, we create
> another dummy event to record the text poke event of ksymbol (refer perf
> record --kcore).
>
> An evlist contains only one tracking event, but can contain multiple
> dummy events.

Thanks for the feedback. So the tracking event is by definition the
first dummy event in the evlist? What is the purpose of the other
dummy events in this case? Perhaps we can get to an intention
revealing implementation something like:

/** The "tracking event" gathering sideband data is the first dummy
event in the list. */
struct evsel *evlist__findnew_tracking_event(struct evlist *evlist)
{
   struct evsel *dummy = evlist__find_first_dummy_event(evlist);

   if (!dummy) {
      dummy = evlist__add_dummy(evlist);
   }
   return dummy;
}

But I think the key thing for me is I'm still not sure what is going
on when there are multiple dummy events for you, what are the other
dummy events for other than tracking sideband data?

Thanks,
Ian

>
> Thanks,
> Yang

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper
  2023-07-28 16:40       ` Ian Rogers
@ 2023-07-29  2:10         ` Yang Jihong
  0 siblings, 0 replies; 27+ messages in thread
From: Yang Jihong @ 2023-07-29  2:10 UTC (permalink / raw)
  To: Ian Rogers, Adrian Hunter
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, kan.liang, james.clark, tmricht, ak, anshuman.khandual,
	linux-kernel, linux-perf-users

Hello,

On 2023/7/29 0:40, Ian Rogers wrote:
> On Thu, Jul 20, 2023 at 12:24 AM Yang Jihong <yangjihong1@huawei.com> wrote:
>>
>> Hello,
>>
>> On 2023/7/20 0:44, Ian Rogers wrote:
>>> On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote:
>>>>
>>>> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
>>>> tracking to the evlist. We may need to search for the dummy event for
>>>> some settings. Therefore, add evlist__findnew_tracking_event() helper.
>>>>
>>>> evlist__findnew_tracking_event() also deal with system_wide maps if
>>>> system_wide is true.
>>>
>>> I'm wondering if we can simplify the naming in the API, we have "dummy
>>> event" which makes sense as we literally call the event "dummy",
>>> "sideband" which refers to the kind of samples/events the dummy event
>>> will record but "tracking" I think tends to get used as a verb rather
>>> than a noun. So I think evlist__findnew_tracking_event should be
>>> evlist__findnew_dummy_event.
>>>
>> Uh, from the discussion that followed, it seems that there is no
>> consensus yet...
>> If there is a clear consensus on whether to use "dummy event" or
>> "tracking event", I will change the name of the API.
>>
>> I think sideband event is equivalent to tracking event (refer
>> evsel__config(), tracking events include task, mmap, mmap2, and comm
>> sideband events, which are all sideband).
>>
>> tracking event are instances of dummy event. For example, we create
>> another dummy event to record the text poke event of ksymbol (refer perf
>> record --kcore).
>>
>> An evlist contains only one tracking event, but can contain multiple
>> dummy events.
> 
> Thanks for the feedback. So the tracking event is by definition the
> first dummy event in the evlist? What is the purpose of the other
Uh... It may not be the first dummy event, but evsel->track must be 
true. Only one evsel in an evlist meets this condition.

> dummy events in this case? Perhaps we can get to an intention
> revealing implementation something like:
> 
> /** The "tracking event" gathering sideband data is the first dummy
> event in the list. */
> struct evsel *evlist__findnew_tracking_event(struct evlist *evlist)
> {
>     struct evsel *dummy = evlist__find_first_dummy_event(evlist);
> 
>     if (!dummy) {
>        dummy = evlist__add_dummy(evlist);
>     }
>     return dummy;
> }
> 
> But I think the key thing for me is I'm still not sure what is going
> on when there are multiple dummy events for you, what are the other
> dummy events for other than tracking sideband data?
> 
For other dummy events, perf record will open a dummy event to track 
ksymbol text_poke when "--kcore" option is used.
I thinks tracking ksymbol text_poke separately it needs to be processed 
independently, go system_wide and enable immediately.

All of the above is my understanding, may need Adrian to confirm whether 
it is accurate.

Thanks,
Yang

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper
  2023-07-19 17:12       ` Ian Rogers
@ 2023-08-14  7:40         ` Adrian Hunter
  0 siblings, 0 replies; 27+ messages in thread
From: Adrian Hunter @ 2023-08-14  7:40 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Yang Jihong, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, jolsa, namhyung, kan.liang, james.clark,
	tmricht, ak, anshuman.khandual, linux-kernel, linux-perf-users

On 19/07/23 20:12, Ian Rogers wrote:
> On Wed, Jul 19, 2023 at 9:59 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 19/07/23 19:44, Ian Rogers wrote:
>>> On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote:
>>>>
>>>> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
>>>> tracking to the evlist. We may need to search for the dummy event for
>>>> some settings. Therefore, add evlist__findnew_tracking_event() helper.
>>>>
>>>> evlist__findnew_tracking_event() also deal with system_wide maps if
>>>> system_wide is true.
>>>
>>> I'm wondering if we can simplify the naming in the API, we have "dummy
>>> event" which makes sense as we literally call the event "dummy",
>>> "sideband" which refers to the kind of samples/events the dummy event
>>> will record but "tracking" I think tends to get used as a verb rather
>>> than a noun. So I think evlist__findnew_tracking_event should be
>>> evlist__findnew_dummy_event.
>>
>> Except the tracking event has "tracking" set but there can be other dummy
>> events that do not.
> 
> Thanks! I'm wondering what a dummy event without tracking would be
> used for - do you have a reference?

For example, text_poke events need to be recorded on all CPUs
because changes to the kernel affect every CPU.  Another example.
it can be interesting to capture switch events CPU-wide even if
only some processes are being traced.

>                                     I don't see anything in
> perf_event.h calling things like mmap2/comm in perf_event_attr
> tracking. I'm not a fan of dummy due to it not being intention
> revealing. Perhaps if we could go back in time  we could call the
> event "sideband",

Auxiliary events are inband with respect to the perf buffer.  Only
when the "main" tracing could be considered to be the AUX area
buffer, can the events in the perf buffer be considered sideband.

>                   maybe we should migrate to this. We have other
> non-obvious uses of the term dummy like in cpumap:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/lib/perf/include/perf/cpumap.h?h=perf-tools-next#n24
> If tracking can be on any event then does that make the functions
> behavior more ambiguous if it means dummy+tracking not <any
> event>+tracking?
> 
> Thanks,
> Ian
> 
>>>
>>> Thanks,
>>> Ian
>>>
>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>> ---
>>>>  tools/perf/builtin-record.c | 11 +++--------
>>>>  tools/perf/util/evlist.c    | 18 ++++++++++++++++++
>>>>  tools/perf/util/evlist.h    |  1 +
>>>>  3 files changed, 22 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>>>> index aec18db7ff23..ca83599cc50c 100644
>>>> --- a/tools/perf/builtin-record.c
>>>> +++ b/tools/perf/builtin-record.c
>>>> @@ -1295,14 +1295,9 @@ static int record__open(struct record *rec)
>>>>          */
>>>>         if (opts->target.initial_delay || target__has_cpu(&opts->target) ||
>>>>             perf_pmus__num_core_pmus() > 1) {
>>>> -               pos = evlist__get_tracking_event(evlist);
>>>> -               if (!evsel__is_dummy_event(pos)) {
>>>> -                       /* Set up dummy event. */
>>>> -                       if (evlist__add_dummy(evlist))
>>>> -                               return -ENOMEM;
>>>> -                       pos = evlist__last(evlist);
>>>> -                       evlist__set_tracking_event(evlist, pos);
>>>> -               }
>>>> +               pos = evlist__findnew_tracking_event(evlist, false);
>>>> +               if (!pos)
>>>> +                       return -ENOMEM;
>>>>
>>>>                 /*
>>>>                  * Enable the dummy event when the process is forked for
>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>> index 7ef43f72098e..25c3ebe2c2f5 100644
>>>> --- a/tools/perf/util/evlist.c
>>>> +++ b/tools/perf/util/evlist.c
>>>> @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev
>>>>         tracking_evsel->tracking = true;
>>>>  }
>>>>
>>>> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide)
>>>> +{
>>>> +       struct evsel *evsel;
>>>> +
>>>> +       evsel = evlist__get_tracking_event(evlist);
>>>> +       if (!evsel__is_dummy_event(evsel)) {
>>>> +               evsel = evlist__add_aux_dummy(evlist, system_wide);
>>>> +               if (!evsel)
>>>> +                       return NULL;
>>>> +
>>>> +               evlist__set_tracking_event(evlist, evsel);
>>>> +       } else if (system_wide) {
>>>> +               perf_evlist__go_system_wide(&evlist->core, &evsel->core);
>>>> +       }
>>>> +
>>>> +       return evsel;
>>>> +}
>>>> +
>>>>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str)
>>>>  {
>>>>         struct evsel *evsel;
>>>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
>>>> index 664c6bf7b3e0..98e7ddb2bd30 100644
>>>> --- a/tools/perf/util/evlist.h
>>>> +++ b/tools/perf/util/evlist.h
>>>> @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr);
>>>>
>>>>  struct evsel *evlist__get_tracking_event(struct evlist *evlist);
>>>>  void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel);
>>>> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide);
>>>>
>>>>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str);
>>>>
>>>> --
>>>> 2.30.GIT
>>>>
>>


^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2023-08-14  7:41 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-15  3:29 [PATCH v2 0/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
2023-07-15  3:29 ` [PATCH v2 1/7] perf evlist: Add perf_evlist__go_system_wide() helper Yang Jihong
2023-07-15  3:29 ` [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper Yang Jihong
2023-07-19 16:44   ` Ian Rogers
2023-07-19 16:59     ` Adrian Hunter
2023-07-19 17:12       ` Ian Rogers
2023-08-14  7:40         ` Adrian Hunter
2023-07-20  7:23     ` Yang Jihong
2023-07-28 16:40       ` Ian Rogers
2023-07-29  2:10         ` Yang Jihong
2023-07-15  3:29 ` [PATCH v2 3/7] perf record: Move setting dummy tracking before record__init_thread_masks() Yang Jihong
2023-07-15  3:29 ` [PATCH v2 4/7] perf record: Track sideband events for all CPUs when tracing selected CPUs Yang Jihong
2023-07-17 14:25   ` Adrian Hunter
2023-07-18  9:07     ` Yang Jihong
2023-07-15  3:29 ` [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check for evlist_config Yang Jihong
2023-07-17 14:41   ` Adrian Hunter
2023-07-18  9:30     ` Yang Jihong
2023-07-18  9:56       ` Adrian Hunter
2023-07-18 10:17         ` Yang Jihong
2023-07-18 10:29           ` Adrian Hunter
2023-07-18 11:32             ` Yang Jihong
2023-07-20  5:41               ` Adrian Hunter
2023-07-20  7:25                 ` Yang Jihong
2023-07-15  3:29 ` [PATCH v2 6/7] perf test: Update system-wide-dummy attr expected values Yang Jihong
2023-07-15  3:29 ` [PATCH v2 7/7] perf test: Add test case for record sideband events Yang Jihong
2023-07-19 16:48   ` Ian Rogers
2023-07-20  7:27     ` Yang Jihong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).