Received-SPF: pass (google.com: domain of linux-kernel+bounces-66350-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 14 Feb 2024 23:08:40 -0800
From: Andi Kleen <ak@linux.intel.com>
To: Ben Gainey <Ben.Gainey@arm.com>
Cc: "irogers@google.com" <irogers@google.com>,
	"alexander.shishkin@linux.intel.com" <alexander.shishkin@linux.intel.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	Mark Rutland <Mark.Rutland@arm.com>,
	"linux-perf-users@vger.kernel.org" <linux-perf-users@vger.kernel.org>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"jolsa@kernel.org" <jolsa@kernel.org>,
	"acme@kernel.org" <acme@kernel.org>,
	"namhyung@kernel.org" <namhyung@kernel.org>,
	"adrian.hunter@intel.com" <adrian.hunter@intel.com>,
	"will@kernel.org" <will@kernel.org>
Subject: Re: [RFC PATCH 0/2] A mechanism for efficient support for
 per-function metrics
Message-ID: <Zc24eLqZycmIg3d2@tassilo>
References: <20240123113420.1928154-1-ben.gainey@arm.com>
 <87r0hfwet0.fsf@linux.intel.com>
 <7de25cbdc9df21e1723d209ca8bd377a55bb54e4.camel@arm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7de25cbdc9df21e1723d209ca8bd377a55bb54e4.camel@arm.com>

On Wed, Feb 14, 2024 at 07:13:50PM +0000, Ben Gainey wrote:
> 
> Nice, I wasn't aware of this feature. I'll have a play...

You have to use an old perf version for now, still need to fix it.
> 
> 
> > 
> > Of course the feature is somewhat dubious in that it will have a very
> > strong systematic bias against short functions and even long
> > functions
> > in some alternating execution patterns. I assume you did some
> > experiments to characterize this. It would be important
> > to emphasize this in any documentation.
> 
> The way I have been thinking about this is that for each sample you
> always maintain a periodic sample count so that the relative ranking of
> functions is maintained, and that the "same as previous" check is a way
> to enhance the attributability of the PMU data for any given sample. 
> 
> But it absolutely correct to say that this will bias the availability
> of PMU data in the way you have describe. The bias depends on sample
> window size, workload characteristics and so on.

I would be more comfortable with it if you added some randomization
on the window sizes. That would limit bias and worst case sampling
error.

> It should be possible to provide a per metric "valid sample" count that
> can be used to judge the  "quality" of the metrics for each symbol,
> which may allow the user to make some adjustments to the recording
> paramters (modify sample period, or sample window size for example).

Even that would be misleading because it assumes that the IP stayed
in the same function between the two samples. But you could have
something like

F1  sample
F2
F1  sample

and if you're unlucky this could happen systematically. The
randomization would fight it somewhat, but even there you might
be very very unlucky.

The only sure way to judge it really is to run branch trace in parallel and see
if it is correct.

Also there is of course the problem that on a modern core the
reordering window might well be larger than your sample window, so any notion
of things happening inside a short window is quite fuzzy.

> > 
> > I don't see anything ARM specific with the technique, so if it's done
> > it should be done generically IMHO
> 
> 
> Great. When i was originally thinking about the implementation of the
> event strobing feature I was thinking:
> 
>  * Add `strobe_sample` flag bit to opt into the fature
>    - This will be mutually exclusive with `freq`.
>  * Add `strobe_period` field to hold the alternate sample period (for
> the sample window.
>  * Have all PMU drivers check and reject the `strobe_sample` flag by
> default; the swizzling of the period will be done in the PMU driver its
> self if it make sense to support this feature for a given PMU.
>    - Do you think this is sensible, or would be better handled in core?

I would have a common function in core that is called from the PMU
drivers, similar to how the adaptive period is done today.

> > > the patch) suggests this approach would work for some counters.
> > > Calculating branch miss rates for example appears to be correct,
> > > likewise UOPS_EXECUTED.THREAD seems to give something like a
> > > sensible
> > > cycles-per-uop value. On the other hand the fixed function
> > > instructions
> > > counter does not appear to sample correctly (it seems to report
> > > either
> > > very small or very large numbers). No idea whats going on there, so
> > > any
> > > insight welcome...
> > 
> > If you use precise samples with 3p there is a restriction on the
> > periods
> > that is enforced by the kernel. Non precise or single/double p should
> > support arbitrary, except that any p is always period + 1.
> 
> Is there some default value for precise? when testing I didn't set any
> specific value for p modifier.

In some cases the perf tool tries to use the highest, e.g. if you don't
specify anything.

If you used :p (and not :P) it should have taken the number you
specified. Single :p is normally not useful because it samples
the -1 IP, normal use is either two or three p. Three p is more
precise but also has some restrictions on the sampling period
and the counters that can be used.

-Andi