Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

From: Peter Zijlstra <peterz@infradead.org>
To: Will Deacon <will@kernel.org>
Cc: Leo Yan <leo.yan@linaro.org>, Mark Rutland <mark.rutland@arm.com>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Mike Leach <mike.leach@linaro.org>, Al Grant <Al.Grant@arm.com>,
	James Clark <James.Clark@arm.com>,
	maz@kernel.org, tglx@linutronix.de
Subject: Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer
Date: Thu, 30 Apr 2020 18:27:50 +0200	[thread overview]
Message-ID: <20200430162750.GD13575@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20200430145823.GA25258@willie-the-truck>

On Thu, Apr 30, 2020 at 03:58:24PM +0100, Will Deacon wrote:
> On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:

> > @@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct perf_event *event,
> >  		userpg->time_mult >>= 1;
> >  	}
> >  	userpg->time_shift = (u16)shift;
> > -	userpg->time_offset = -now;
> > +
> > +	/*
> > +	 * Since arch timer is enabled ealier than sched clock registration,
> > +	 * compuate the delta (in nanosecond unit) between the arch timer
> > +	 * counter and sched clock, assign the delta to time_offset and
> > +	 * perf tool can use it for timestamp calculation.
> > +	 *
> > +	 * The formula for conversion arch timer cycle to ns is:
> > +	 *   quot = (cyc >> time_shift);
> > +	 *   rem  = cyc & ((1 << time_shift) - 1);
> > +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
> > +	 */
> > +	count = arch_timer_read_counter();
> > +	quot = count >> shift;
> > +	rem = count & ((1 << shift) - 1);
> > +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
> > +	userpg->time_offset = now - ns;
> 
> Hmm, reading the counter and calculating the delta feels horribly
> approximate to me. It would be much better if we could get hold of the
> initial epoch cycles from the point at which sched_clock was initialised
> using the counter. This represents the true cycle delta between the counter
> and what sched_clock uses for 0 ns.
> 
> Unfortunately, I can't see a straightforward way to grab that information.
> It looks like x86 pulls this directly from the TSC driver.

Yeah, and I'm thinking you should do the same. IIRC ARM uses this
kernel/time/sched_clock.c thing, and if I read that right, the struct
clock_data there has all the bits you need here.

So I'm thinking that you might want to add a helper function here to get
you the good stuff.