Date:   Thu, 25 Nov 2021 20:30:53 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     James Clark <james.clark@arm.com>
Cc:     German Gomez <german.gomez@arm.com>, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org, acme@kernel.org,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        John Garry <john.garry@huawei.com>,
        Will Deacon <will@kernel.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [RESEND PATCH 1/1] perf arm-spe: report all SPE records as "all"
 events
Message-ID: <20211125123053.GB1599216@leoy-ThinkPad-X240s>
References: <20211117142833.226629-1-german.gomez@arm.com>
 <20211125075358.GA1599216@leoy-ThinkPad-X240s>
 <12d44d96-1fcd-1fdd-64ea-beef40a27d1d@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <12d44d96-1fcd-1fdd-64ea-beef40a27d1d@arm.com>
Precedence: bulk

On Thu, Nov 25, 2021 at 10:21:48AM +0000, James Clark wrote:
> On 25/11/2021 07:53, Leo Yan wrote:

[...]

> >> +static int arm_spe__synth_other_sample(struct arm_spe_queue *speq,
> >> +				       u64 spe_events_id)
> >> +{
> >> +	struct arm_spe *spe = speq->spe;
> >> +	struct arm_spe_record *record = &speq->decoder->record;
> >> +	union perf_event *event = speq->event_buf;
> >> +	struct perf_sample sample = { .ip = 0, };
> >> +
> >> +	arm_spe_prep_sample(spe, speq, event, &sample);
> >> +
> >> +	sample.id = spe_events_id;
> >> +	sample.stream_id = spe_events_id;
> >> +	sample.addr = record->to_ip;
> > 
> > After checked the event types, I think "other" samples would include
> > below raw event types:
> 
> Maybe we should rename some of the functions and variables if there is
> confusion, but I think this new group is "all" rather than "other" because
> it also includes all the events that would be put in other groups.
> 
> > 
> >   EV_EXCEPTION_GEN
> >   EV_RETIRED
> >   EV_NOT_TAKEN
> >   EV_ALIGNMENT
> >   EV_PARTIAL_PREDICATE
> >   EV_EMPTY_PREDICATE
> > 
> > I am just wander if we can use sample.transaction to store these event
> > types, otherwise, we cannot distinguish the event type for the samples.
> 
> If we can use the transaction field to distinguish sample types, I'm
> wondering why we need the separate groups at all. If this new group
> includes all sample types, and they're all labelled, do we need to
> continue with the other groups like "tlb-access" and "branch-miss"?

I admit the samples for "tlb-access" and "branch-miss" might not a
good practice.  At the time when I was upstreaming the Arm SPE patches
(mainly based Hisilicon patches), the main idea for use some events to
output samples, this is why "tlb-access" and "branch-miss" events were
introduced.

But when worked on Arm SPE for enabling "perf mem" and "perf c2c", I
recognized that _consuming_ hardware trace data is much more important
than merely outputting samples.  A better way for _consuming_ the Arm SPE
trace data is to synthesize samples with a prominent type and use an
extra field in sample for the associated attribution.  E.g. we can
synthesize memory samples and uses field "sample.data_src" to
distinguish different memory attributions, thus the events
"tlb-access" and "branch-miss" are not useful.  This approach can be
applied to instruction event and branch event, and both of them use
field "sample.flags" to indicate what's the type of instruction or
branch.

If we follow up this approach, below records can be considered to
synthesize instruction or branch samples:

  EV_EXCEPTION_GEN
  EV_RETIRED
  EV_NOT_TAKEN

Below records can be considered to generate memory samples:

  EV_ALIGNMENT
  EV_PARTIAL_PREDICATE
  EV_EMPTY_PREDICATE

We can consider to extend sample's three fields:
sample::flags for instruction/branch samples
sample::data_srouce for memory samples
sample::transaction for memory transactions (see macros with
prefix PERF_TXN_).

> Or does the perf GUI not allow filtering by transaction type?

To be honest, when introduced the events "tlb-access" and
"branch-miss", I didn't consider transaction type at all.

Thanks,
Leo