Re: [PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible

From: Adrian Hunter
Date: Mon Apr 26 2021 - 01:55:19 EST


On 12/04/21 11:34 am, Leo Yan wrote:
> Commit d110162cafc8 ("perf tsc: Support cap_user_time_short for event
> TIME_CONV") supports the extended parameters for event TIME_CONV, but it
> broke the backwards compatibility, so any perf data file with old event
> format fails to convert timestamp.
>
> For the backwards-compatibility, this patch checks the event size, if
> the event size confirms the extended parameters are supported in the
> event TIME_CONV, then copies these parameters.
>
> Fixes: d110162cafc8 ("perf tsc: Support cap_user_time_short for event TIME_CONV")
> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>

Perhaps could make use of a helper like the one described in my comments for patch 2.
Nevertheless:

Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>

> ---
> tools/perf/util/jitdump.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 9760d8e7b386..67b514c38a43 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -396,21 +396,32 @@ static pid_t jr_entry_tid(struct jit_buf_desc *jd, union jr_entry *jr)
>
> static uint64_t convert_timestamp(struct jit_buf_desc *jd, uint64_t timestamp)
> {
> - struct perf_tsc_conversion tc;
> + struct perf_tsc_conversion tc = { 0 };
> + struct perf_record_time_conv *time_conv = &jd->session->time_conv;
>
> if (!jd->use_arch_timestamp)
> return timestamp;
>
> - tc.time_shift = jd->session->time_conv.time_shift;
> - tc.time_mult = jd->session->time_conv.time_mult;
> - tc.time_zero = jd->session->time_conv.time_zero;
> - tc.time_cycles = jd->session->time_conv.time_cycles;
> - tc.time_mask = jd->session->time_conv.time_mask;
> - tc.cap_user_time_zero = jd->session->time_conv.cap_user_time_zero;
> - tc.cap_user_time_short = jd->session->time_conv.cap_user_time_short;
> + tc.time_shift = time_conv->time_shift;
> + tc.time_mult = time_conv->time_mult;
> + tc.time_zero = time_conv->time_zero;
>
> - if (!tc.cap_user_time_zero)
> - return 0;
> + /*
> + * The event TIME_CONV was extended for the fields from "time_cycles"
> + * when supported cap_user_time_short, for backward compatibility,
> + * checks the event size and assigns these extended fields if these
> + * fields are contained in the event.
> + */
> + if (time_conv->header.size >
> + ((void *)&time_conv->time_cycles - (void *)time_conv)) {
> + tc.time_cycles = time_conv->time_cycles;
> + tc.time_mask = time_conv->time_mask;
> + tc.cap_user_time_zero = time_conv->cap_user_time_zero;
> + tc.cap_user_time_short = time_conv->cap_user_time_short;
> +
> + if (!tc.cap_user_time_zero)
> + return 0;
> + }
>
> return tsc_to_perf_time(timestamp, &tc);
> }
>