Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Ali Saidi <alisaidi@amazon.com>
To:     <leo.yan@linaro.org>
CC:     <acme@kernel.org>, <alexander.shishkin@linux.intel.com>,
        <alisaidi@amazon.com>, <andrew.kilroy@arm.com>,
        <benh@kernel.crashing.org>, <german.gomez@arm.com>,
        <james.clark@arm.com>, <john.garry@huawei.com>, <jolsa@redhat.com>,
        <kjain@linux.ibm.com>, <lihuafei1@huawei.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-perf-users@vger.kernel.org>,
        <mark.rutland@arm.com>, <mathieu.poirier@linaro.org>,
        <mingo@redhat.com>, <namhyung@kernel.org>, <peterz@infradead.org>,
        <will@kernel.org>, <yao.jin@linux.intel.com>
Subject: Re: [PATCH v2 1/2] perf arm-spe: Use SPE data source for neoverse cores
Date:   Sun, 13 Mar 2022 19:06:19 +0000
Message-ID: <20220313190619.18914-1-alisaidi@amazon.com>
In-Reply-To: <20220313114615.GA143848@leoy-ThinkPad-X240s>
References: <20220313114615.GA143848@leoy-ThinkPad-X240s>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk

On Sun, 13 Mar 2022 11:47:58 +0000 Leo Yan wrote:
> Hi Ali, German,
> 
> On Wed, Mar 02, 2022 at 11:59:05AM +0000, German Gomez wrote:
> > Hi Ali,
> > 
> > On 21/02/2022 22:47, Ali Saidi wrote:
> > > When synthesizing data from SPE, augment the type with source information
> > > for Arm Neoverse cores. The field is IMPLDEF but the Neoverse cores all use
> > > the same encoding. I can't find encoding information for any other SPE
> > > implementations to unify their choices with Arm's thus that is left for
> > > future work.
[snip]
> > >  
> > > +static void arm_spe__synth_data_source_neoverse(const struct arm_spe_record *record,
> > > +						union perf_mem_data_src *data_src)
> > > +{
> > > +	switch (record->source) {
> > > +	case ARM_SPE_NV_L1D:
> > > +		data_src->mem_lvl = PERF_MEM_LVL_HIT;
> > 
> > I understand mem_lvl is deprecated but shouldn't we add the level bits here as well for backwards compat?
> 
> Thanks for pointing out this.  Yeah, I think German's suggestion is
> valid, the commit 6ae5fa61d27d ("perf/x86: Fix data source decoding
> for Skylake") introduces new field 'mem_lvl_num', but it also keeps
> backwards compatible for the field 'mem_lvl'.
I thought about that, but then I'm making some assumption about how to fit
this into the old LVL framework, which is perhaps OK (afaik there are no
Neoverse systems with more than 3 cache levels). What stopped me was that
perf_mem__lvl_scnprintf() does the wrong thing when both are set so I
assumed that setting both was not the right course of action.

> 
> > > +		data_src->mem_lvl_num = PERF_MEM_LVLNUM_L1;
> > > +		break;
> > > +	case ARM_SPE_NV_L2:
> > > +		data_src->mem_lvl = PERF_MEM_LVL_HIT;
> > > +		data_src->mem_lvl_num = PERF_MEM_LVLNUM_L2;
> > > +		break;
> > > +	case ARM_SPE_NV_PEER_CORE:
> > > +		data_src->mem_lvl = PERF_MEM_LVL_HIT;
> > > +		data_src->mem_snoop = PERF_MEM_SNOOP_HITM;
> > > +		data_src->mem_lvl_num = PERF_MEM_LVLNUM_ANY_CACHE;
> 
> For PEER_CORE data source, we don't know if it's coming from peer
> core's L1 cache or L2 cache, right?
We don't.

> 
> If so, do you think if it's possible to retrieve more accurate info
> from the field "record->type"?
No, we just don't know and it really doesn't matter. The main reason to
understand the source is to understand the penalty of data coming from
the source and that it's coming from a core should be sufficient.

> 
> > > +		break;
> > > +	/*
> > > +	 * We don't know if this is L1, L2, or even L3 (for the cases the system
> > > +	 * has an L3, but we do know it was a cache-2-cache transfer, so set
> > > +	 * SNOOP_HITM
> > > +	 */
> > > +	case ARM_SPE_NV_LCL_CLSTR:
> > > +	case ARM_SPE_NV_PEER_CLSTR:
> > > +		data_src->mem_lvl = PERF_MEM_LVL_HIT;
> > > +		data_src->mem_snoop = PERF_MEM_SNOOP_HITM;
> > > +		data_src->mem_lvl_num = PERF_MEM_LVLNUM_ANY_CACHE;
> 
> Seems to me, we need to add attribution to indicate the difference
> between ARM_SPE_NV_PEER_CORE and ARM_SPE_NV_LCL_CLSTR.
I don't think we really do, see my reasoning above. 

> 
> For ARM_SPE_NV_PEER_CLSTR data source, should we set any "remote"
> attribution as well?

No, we should leave remote for data coming from another chip/socket
which is really impactful. 

> 
> > > +		break;
> > > +	/*
> > > +	 * System cache is assumed to be L4, as cluster cache (if it exists)
> > > +	 * would be L3 cache on Neoverse platforms
> > > +	 */
> > > +	case ARM_SPE_NV_SYS_CACHE:
> > > +		data_src->mem_lvl = PERF_MEM_LVL_HIT;
> > > +		data_src->mem_lvl_num = PERF_MEM_LVLNUM_L4;
> > > +		break;
> > > +	/*
> > > +	 * We don't know what level it hit in, except it came from the other
> > > +	 * socket
> > > +	 */
> > > +	case ARM_SPE_NV_REMOTE:
> > > +		data_src->mem_snoop = PERF_MEM_SNOOP_HITM;
> > > +		data_src->mem_remote = PERF_MEM_REMOTE_REMOTE;
> > > +		break;
> 
> Just curious, is it possible that 'record->source' combines multiple
> bits?  Like we can get a data source value with:
> 
>   ARM_SPE_NV_REMOTE | ARM_SPE_NV_REMOTE

source encodes a single value (not bits that represent flags) on Neoverse
cores.

[snip]
> > > @@ -796,6 +868,10 @@ static int arm_spe_process_event(struct perf_session *session,
> > >  	u64 timestamp;
> > >  	struct arm_spe *spe = container_of(session->auxtrace,
> > >  			struct arm_spe, auxtrace);
> > > +	const char *cpuid = perf_env__cpuid(session->evlist->env);
> > > +	u64 midr = strtol(cpuid, NULL, 16);
> > > +
> > > +	spe->midr = midr;
> > 
> > I think this midr setup belongs in the arm_spe_process_auxtrace_info callback instead.
> 
> Yeah, arm_spe_process_event() would be invoked for multiple times for
> processing perf events.  arm_spe_process_auxtrace_info() would be a
> good place to initialize midr.

Will do.

Thanks,
Ali