Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 8 Apr 2022 09:06:35 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Ali Saidi <alisaidi@amazon.com>
Cc:     Nick.Forrington@arm.com, acme@kernel.org,
        alexander.shishkin@linux.intel.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
Message-ID: <20220408010635.GA973239@leoy-ThinkPad-X240s>
References: <20220404151218.GA898573@leoy-ThinkPad-X240s>
 <20220406210017.11887-1-alisaidi@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220406210017.11887-1-alisaidi@amazon.com>
Precedence: bulk

On Wed, Apr 06, 2022 at 09:00:17PM +0000, Ali Saidi wrote:
> 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:

[...]

> > > 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? 

I think we need to handle both load and store operations in 'perf c2c'
tool.

For the load operation, in the 'cache line details' view, we need to
support 'snoop_peer' conlumn; and since Arm SPE doesn't give any data
source info for store opeartion, so my plan is to add an extra conlumn
'Other' based on the two existed conlumns 'L1 Hit' and 'L1 Miss'.

Could you leave this part for me?  I will respin my patch set for
extend 'perf c2c' for this (and hope can support the old Arm SPE trace
data).

Please note, when you spin new patch set, you need to take care for
the store operations.  In the current patch set, it will wrongly
always set L1 hit for all store operations due to the data source
field is always zero.  My understanding is for all store operations,
we need to set the cache level as PERF_MEM_LVLNUM_ANY_CACHE and snoop
type as PERF_MEM_SNOOP_NA.

> > 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. 

Okay, it's fine for me to move forward to use MEM_SNOOP_PEER as the
solution.

Since German is looking into this part, @German, if you have any comment
on this part, just let us know.

Thanks,
Leo