On Thu, Oct 19, 2023 at 11:31:16AM +0100, James Clark wrote:

[...]

...

--- a/tools/perf/Documentation/itrace.txt +++ b/tools/perf/Documentation/itrace.txt @@ -25,6 +25,7 @@ q quicker (less detailed) decoding A approximate IPC Z prefer to ignore timestamps (so-called "timeless" decoding)

• T	use the timestamp trace as kernel time
  

Maybe "Treat hardware timestamps as kernel time (trace and CPU time use same clock source)" would be clearer.

Agreed.

And another point, although this isn't really related to this patch, but why do we have the single letter arguments for itrace? It seems like it massively restricts the available options and makes the command lines hard to read because they don't have long forms. Why not just have them as normal arguments?

TBH, I tried a bit for adding a normal argument, but I found it's not nature like itrace option for passing the normal argument into the decoder. And if we add a normal argument, we need to consider adding into perf commands for 'perf report', 'perf script', etc; itrace options can be shared by perf output commands.

I understand an advantage of using normal argument is the 'perf report' command (e.g. perf report --aux-trace-kernel-time) doesn't need to specify any extra itrace option.

If anyone still has concern for adding itrace optiona and prefer to add normal argument, I am happy to rework for adding normal argument.

If it's a backwards compatibility thing, would there be any objection to adding this new option as a normal one rather than an itrace one?

backward compatibility would not a problem - we add a new feature and at meantime we don't break anything.

...

The default is all events i.e. the same as --itrace=iybxwpe, except for perf script where it is --itrace=ce diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c index a0368202a746..f528c4364d23 100644 --- a/tools/perf/util/auxtrace.c +++ b/tools/perf/util/auxtrace.c @@ -1638,6 +1638,9 @@ int itrace_do_parse_synth_opts(struct itrace_synth_opts *synth_opts, case 'Z': synth_opts->timeless_decoding = true; break;

• case 'T':
  

• 	synth_opts->use_timestamp = true;
  

• 	break;
  

  case ' ': case ',': break;

diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h index 29eb82dff574..55702215a82d 100644 --- a/tools/perf/util/auxtrace.h +++ b/tools/perf/util/auxtrace.h @@ -99,6 +99,7 @@ enum itrace_period_type {

• @remote_access: whether to synthesize remote access events
• @mem: whether to synthesize memory events
• @timeless_decoding: prefer "timeless" decoding i.e. ignore timestamps

• • @use_timestamp: use the timestamp trace as kernel time
  • @vm_time_correlation: perform VM Time Correlation
  • @vm_tm_corr_dry_run: VM Time Correlation dry-run
  • @vm_tm_corr_args: VM Time Correlation implementation-specific arguments

@@ -146,6 +147,7 @@ struct itrace_synth_opts { bool remote_access; bool mem; bool timeless_decoding;

• bool use_timestamp;

And then this one could be like "hw_time_is_kernel_time", but I'm stuggling to think of something shorter. Maybe your one is fine along with the comment.

It's fine for me to rename as "hw_time_is_kernel_time" for more readable. Will do in next spin.

Thanks, Leo

On 14/10/2023 08:45, Leo Yan wrote:

Prior to Armv8.4, the feature FEAT_TRF is not supported by Arm CPUs. Consequently, the sysfs node 'ts_source' will not be set as 1 by the CoreSight ETM driver. On the other hand, the perf tool relies on the 'ts_source' node to determine whether the kernel timestamp is traced. Since the 'ts_source' is not set for Arm CPUs prior to Armv8.4, platforms in this case cannot utilize the traced timestamp as the kernel time.

This patch enables the 'T' itrace option, which forcibly utilizes the traced timestamp as the kernel time. If users are aware that their working platform's Arm CoreSight shares the same counter with the kernel time, they can specify 'T' option to decode the traced timestamp as the kernel time.

An usage example is:

# perf record -e cs_etm// -- test_program # perf script --itrace=i10ibT # perf report --itrace=i10ibT

Signed-off-by: Leo Yan leo.yan@linaro.org

tools/perf/util/cs-etm.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 9729d006550d..4a37fdeb1795 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -3322,12 +3322,27 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event, etm->metadata = metadata; etm->auxtrace_type = auxtrace_info->type;

• /* Use virtual timestamps if all ETMs report ts_source = 1 */
• etm->has_virtual_ts = cs_etm__has_virtual_ts(metadata, num_cpu);

• if (etm->synth_opts.use_timestamp)

• /*
  

•  * Prior to Armv8.4, Arm CPUs don't support FEAT_TRF feature,
  

•  * therefore the decoder cannot know if the timestamp trace is
  

•  * same with the kernel time.
  

•  *
  

•  * If a user has knowledge for the working platform and can
  

•  * specify itrace option 'T' to tell decoder to forcely use the
  

•  * traced timestamp as the kernel time.
  

•  */
  

• etm->has_virtual_ts = true;
  

• else

• /* Use virtual timestamps if all ETMs report ts_source = 1 */
  

• etm->has_virtual_ts = cs_etm__has_virtual_ts(metadata, num_cpu);
  

if (!etm->has_virtual_ts) ui__warning("Virtual timestamps are not enabled, or not supported by the traced system.\n"

• 	    "The time field of the samples will not be set accurately.\n\n");
  

• 	    "The time field of the samples will not be set accurately.\n"
  

• 	    "For Arm CPUs prior to Armv8.4 or without support FEAT_TRF,\n"
  

• 	    "you can specify the itrace option 'T' for timestamp decoding\n"
  

• 	    "if the Coresight timestamp on the platform is same with the kernel time.\n\n");
  

etm->auxtrace.process_event = cs_etm__process_event; etm->auxtrace.process_auxtrace_event = cs_etm__process_auxtrace_event;

Reviewed-by: James Clark james.clark@arm.com