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:   Mon, 4 Apr 2022 23:12:18 +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: <20220404151218.GA898573@leoy-ThinkPad-X240s>
References: <20220331124425.GB1704284@leoy-ThinkPad-X240s>
 <20220403203337.18927-1-alisaidi@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220403203337.18927-1-alisaidi@amazon.com>
Precedence: bulk

On Sun, Apr 03, 2022 at 08:33:37PM +0000, Ali Saidi wrote:

[...]

> > Let me just bring up my another
> > thinking (sorry that my suggestion is float): another choice is we set
> > ANY_CACHE as cache level if we are not certain the cache level, and
> > extend snoop field to indicate the snooping logics, like:
> > 
> >   PERF_MEM_SNOOP_PEER_CORE
> >   PERF_MEM_SNOOP_LCL_CLSTR
> >   PERF_MEM_SNOOP_PEER_CLSTR
> > 
> > Seems to me, we doing this is not only for cache level, it's more
> > important for users to know the variant cost for involving different
> > snooping logics.
> 
> I think we've come full circle :).

Not too bad, and I learned a lot :)

> Going back to what do we want to indicate to
> a user about the source of the cache line, I believe there are three things with
> an eye toward helping a user of the data improve the performance of their
> application:

Thanks a lot for summary!

> 1. The level below them in the hierarchy it it (L1, L2, LLC, local DRAM).
> Depending on the level this directly indicates the expense of the operation. 
> 
> 2. If it came from a peer of theirs on the same socket. I'm really of the
> opinion still that exactly which peer, doesn't matter much as it's a 2nd or 3rd
> order concern compared to, it it couldn't be sourced from a cache level below
> the originating core, had to come from a local peer and the request went to
> that lower levels and was eventually sourced from a peer.  Why it was sourced
> from the peer is still almost irrelevant to me. If it was truly modified or the
> core it was sourced from only had permission to modify it the snoop filter
> doesn't necessarily need to know the difference and the outcome is the same. 

I think here the key information delivered is:

For the peer snooping, you think there has big cost difference between
L2 cache snooping and L3 cache snooping; for L3 cache snooping, we
don't care about it's an internal cluster snooping or external cluster
snooping, and we have no enough info to reason snooping type (HIT vs
HITM).

> 3. For multi-socket systems that it came from a different socket and there it is
> probably most interesting if it came from DRAM on the remote socket or a cache.
>
> I'm putting 3 aside for now since we've really been focusing on 1 and 2 in this
> discussion and I think the biggest hangup has been the definition of HIT vs
> HITM.

Agree on the item 3.

> If someone has a precise definition, that would be great, but AFAIK it
> goes back to the P6 bus where HIT was asserted by another core if it had a line
> (in any state) and HITM was additionally asserted if a core needed to inhibit
> another device (e.g. DDR controller) from providing that line to the requestor. 

Thanks for sharing the info for how the bus implements HIT/HITM.

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

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

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.

Thanks,
Leo