Re: [PATCH v4 2/4] perf arm-spe: Use SPE data source for neoverse cores

From: Ali Saidi <alisaidi@amazon.com>
To: <leo.yan@linaro.org>
Cc: <Nick.Forrington@arm.com>, <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@kernel.org>,
	<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>
Subject: Re: [PATCH v4 2/4] perf arm-spe: Use SPE data source for neoverse cores
Date: Wed, 6 Apr 2022 21:00:17 +0000	[thread overview]
Message-ID: <20220406210017.11887-1-alisaidi@amazon.com> (raw)
In-Reply-To: <20220404151218.GA898573@leoy-ThinkPad-X240s>

On Mon, 4 Apr 2022 15:12:18  +0000, Leo Yan wrote:
> On Sun, Apr 03, 2022 at 08:33:37PM +0000, Ali Saidi wrote:

[...]

> > The latter logic is why I think it's perfectly acceptable to use HITM to
> > indicate a peer cache-to-cache transfer, however since others don't feel that way
> > let me propose a single additional snooping type PERF_MEM_SNOOP_PEER that
> > indicates some peer of the hierarchy below the originating core sourced the
> > data.  This clears up the definition that line came from from a peer and may or
> > may not have been modified, but it doesn't add a lot of implementation dependant
> > functionality into the SNOOP API. 
> > 
> > We could use the mem-level to indicate the level of the cache hierarchy we had
> > to get to before the snoop traveled upward, which seems like what x86 is doing
> > here.
> 
> It makes sense to me that to use the highest cache level as mem-level.
> Please add comments in the code for this, this would be useful for
> understanding the code.

Ok.

> > PEER_CORE -> MEM_SNOOP_PEER + L2
> > PEER_CLSTR -> MEM_SNOOP_PEER + L3
> > PEER_LCL_CLSTR -> MEM_SNOOP_PEER + L3 (since newer neoverse cores don't support
> > the clusters and the existing commercial implementations don't have them).
> 
> Generally, this idea is fine for me.

Great.  

Now the next tricky thing. Since we're not using HITM for recording the memory
events, the question becomes for the c2c output should we output the SNOOP_PEER
events as if they are HITM events with a clarification in the perf-c2c man page
or effectively duplicate all the lcl_hitm logic, which is a fair amount,  in
perf c2c to add a column and sort option? 

> Following your suggestion, if we connect the concepts PoC and PoU in Arm
> reference manual, we can extend the snooping mode with MEM_SNOOP_POU
> (for PoU) and MEM_SNOOP_POC (for PoC), so:
> 
> PEER_CORE -> MEM_SNOOP_POU + L2
> PEER_LCL_CLSTR -> MEM_SNOOP_POU + L3
> PEER_CLSTR -> MEM_SNOOP_POC + L3
> 
> Seems to me, we could consider for this.  If this is over complexity or
> even I said any wrong concepts for this, please use your method.

I think this adds a lot of complexity and reduces clarity. Some systems
implement coherent icaches and the PoU would be the L1 cache, others don't so
that would be the L2 (or wherever there is a unified cache). Similarly, with the
point of coherency, some systems would consider that dram, but other systems
have transparent LLCs and it would be the LLC. 

Thanks,
Ali

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel