Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Ali Saidi <alisaidi@amazon.com>
To:     <leo.yan@linaro.org>
CC:     <Al.Grant@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@redhat.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] perf arm-spe: Use SPE data source for neoverse cores
Date:   Sat, 5 Feb 2022 00:07:19 +0000
Message-ID: <20220205000719.19986-1-alisaidi@amazon.com>
In-Reply-To: <20220203091934.GA2013381@leoy-ThinkPad-X240s>
References: <20220203091934.GA2013381@leoy-ThinkPad-X240s>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk

Hi Leo,

On 2/3/22, 3:20 AM, "Leo Yan" <leo.yan@linaro.org> wrote:
>[...]
>> >> >> +			data_src.mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_HIT;
>> >> >
>> >> >This one also adds PERF_MEM_LVL_HIT even though the check of "if (record->type & ARM_SPE_LLC_MISS)"
>> >> >hasn't happened yet. Maybe some comments would make it a bit clearer, but at the moment it's
>> >> >not obvious how the result is derived because there are some things that don't add up like
>> >> >ARM_SPE_LLC_MISS == PERF_MEM_LVL_HIT.
>> >> 
>> >> Assuming the above is correct, my reading of the existing code that creates the
>> >> c2c output is that when an access is marked as an LLC hit, that doesn't
>> >> necessarily mean that the data was present in the LLC. I don't see how it could
>> >> given that LLC_HIT + HITM means the line was dirty in another CPUs cache, and so
>> >> LLC_HIT + HITM seems to mean that it was a hit in the LLC snoop filter and
>> >> required a different core to provide the line. This and the above certainly
>> >> deserve a comment as to why the miss is being attributed in this way if it's
>> >> otherwise acceptable.
>> >
>> >As James pointed out, this might introduce confusion.  I am wanderding
>> >if we can extract two functions for synthesizing the data source, one is
>> >for Neoverse platform and another is for generic purpose (which
>> >without data source packets), below code is to demonstrate the basic
>> >idea.
>> 
>> The code below is cleaner, and I'm happy to rework the patches in this way, but
>> I think the question still remains about unifying behavior of the tool. If we
>> mark something with a data source of ARM_SPE_NV_PEER_CORE as at L1 hit + HITM
>> certainly c2c won't show the correct thing today, but i think it also hides the
>> intent. The line in question missed the L1, L2, and got to the LLC where we did
>> find a record that it was in another cores cache (L1 or L2). Looking at the way
>> that c2c works today, it seems like marking this as a hit in the LLC snoop
>> filter is the best way to unify behaviors among architectures?
>
>Thanks a lot for pointing out this.  I looked into the code and
>compared the memory trace data from x86, I found the HITM tag is always
>sticking to LLC from x86's memory events.  This would be the main reason
>why current code in perf is only support HITM for LLC.
>
>I don't think it's a good way to always mark LLC snoop, even it's a
>snooping operation in L1 or L2 cache on Arm64 platforms; this would
>introduce confusion for users when using Arm SPE for profiling.
>
>Alternatively, we can support HITM tag for L1/L2 cache levels in perf,
>this can allow us to match memory topology on Arm64 arch, and it should
>not introduce any regression on x86 arch.
>
>Could you confirm if below code can fix the issue or not?

Yes, that should do it. Want me to repsin this with the changes we discussed?

Ali