Re: [PATCH v2 1/4] perf tools: Support Arm arch timer counter

[Leo Yan <leo.yan@linaro.org>]

Hi Wei,

On Thu, Aug 20, 2020 at 10:56:46AM +0800, liwei (GF) wrote:

[...]

> > +int perf_read_arch_timer_conversion(const struct perf_event_mmap_page *pc,
> > +				    struct perf_arch_timer_conversion *tc)
> > +{
> > +	bool cap_user_time_zero, cap_user_time_short;
> > +	u32 seq;
> > +	int i = 0;
> > +
> > +	while (1) {
> > +		seq = pc->lock;
> > +		/* Add barrier between the sequence lock and data accessing */
> > +		rmb();
> > +
> > +		tc->time_mult = pc->time_mult;
> > +		tc->time_shift = pc->time_shift;
> > +		tc->time_zero = pc->time_zero;
> > +		tc->time_cycles = pc->time_cycles;
> > +		tc->time_mask = pc->time_mask;
> > +		cap_user_time_zero = pc->cap_user_time_zero;
> > +		cap_user_time_short = pc->cap_user_time_short;
> > +
> > +		/* Add barrier between the data accessing and sequence lock */
> > +		rmb();
> > +		if (pc->lock == seq && !(seq & 1))
> > +			break;
> > +		if (++i > 10000) {
> > +			pr_debug("%s: failed to get perf_event_mmap_page lock\n",
> > +				 __func__);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	if (!cap_user_time_zero || !cap_user_time_short)
> > +		return -EOPNOTSUPP;
> > +
> > +	return 0;
> > +}
> 
> Should we implement the perf_event__synth_time_conv() method? As it can be supported
> on arm64 arch now.

Thanks a lot for pointing out, agree that we should implement
perf_event__synth_time_conv() for Arm64.  Will do it in next spin.

> These is also a tsc get method called rdtsc(), weak implementation in 'tools/perf/util/tsc.c',
> maybe we should rename it to an arch independent name, and implement the arm64 version
> (read_cntvct_el0 in patch 3) here.

If connect with your comment in patch 02, I think you are asking to
consolidate for TSC common APIs and operations.  I took time to review
the codes for conversion between timer counter and time stamp, your
suggestion is reasonable and feasible for me.

But the consolidation will impact the existed x86 implementation, before
proceed to this direction for the next patch set, I'd like to get some
review from x86's developers.

@PeterZ, could you take a look for this and let me know if you have any
concern for the consolidation?

> Thus we can also make the testcase generic,
> instead of adding a new one for arm64 :).

Exactly.  Will do this if we get agreement for consolidation.

Thanks for the reviewing,
Leo