From: Leo Yan <leo.yan@linaro.org> To: Ali Saidi <alisaidi@amazon.com> Cc: 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@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: Mon, 14 Mar 2022 12:05:42 +0800 [thread overview] Message-ID: <20220314040542.GA163961@leoy-ThinkPad-X240s> (raw) In-Reply-To: <20220313190619.18914-1-alisaidi@amazon.com> On Sun, Mar 13, 2022 at 07:06:19PM +0000, Ali Saidi wrote: [...] > > > > +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. Thanks for pointing out this. I looked at perf_mem__lvl_scnprintf() and it prints both for fields 'mem_lvl' and 'mem_lvl_num'. Thus I can see the output result shows the duplicate info for memory access like "L1 or L1 hit", "L3 or L3 hit", etc. This would be a common issue crossing archs. Do I miss any other issues? > > > > + 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. Okay, the question is Neoverse has three different data sources "ARM_SPE_NV_PEER_CORE", "ARM_SPE_NV_LCL_CLSTR" and "ARM_SPE_NV_PEER_CLSTR", but the patch only uses the same attribution for all of them. To be honest, I don't have precise understanding the definition for these three types, seems to me "ARM_SPE_NV_PEER_CORE" means to fetch data cache from peer core (like SMT things), "ARM_SPE_NV_LCL_CLSTR" means cache conherency within the same cluster with SCU, "ARM_SPE_NV_PEER_CLSTR" means the conherency happens with external bus (like CCI or CMN). So I'd like to suggest to consider to extend the level definitions so can allow us to express the data source for Arm arch precisely. It's important to understand current cache level definitions which is derived from x86 arch and think what's the good way to match and extend for Arm memory hierarchy. I will think a bit more for this, and if have any idea will share back. Thanks, Leo
WARNING: multiple messages have this Message-ID (diff)
From: Leo Yan <leo.yan@linaro.org> To: Ali Saidi <alisaidi@amazon.com> Cc: 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@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: Mon, 14 Mar 2022 12:05:42 +0800 [thread overview] Message-ID: <20220314040542.GA163961@leoy-ThinkPad-X240s> (raw) In-Reply-To: <20220313190619.18914-1-alisaidi@amazon.com> On Sun, Mar 13, 2022 at 07:06:19PM +0000, Ali Saidi wrote: [...] > > > > +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. Thanks for pointing out this. I looked at perf_mem__lvl_scnprintf() and it prints both for fields 'mem_lvl' and 'mem_lvl_num'. Thus I can see the output result shows the duplicate info for memory access like "L1 or L1 hit", "L3 or L3 hit", etc. This would be a common issue crossing archs. Do I miss any other issues? > > > > + 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. Okay, the question is Neoverse has three different data sources "ARM_SPE_NV_PEER_CORE", "ARM_SPE_NV_LCL_CLSTR" and "ARM_SPE_NV_PEER_CLSTR", but the patch only uses the same attribution for all of them. To be honest, I don't have precise understanding the definition for these three types, seems to me "ARM_SPE_NV_PEER_CORE" means to fetch data cache from peer core (like SMT things), "ARM_SPE_NV_LCL_CLSTR" means cache conherency within the same cluster with SCU, "ARM_SPE_NV_PEER_CLSTR" means the conherency happens with external bus (like CCI or CMN). So I'd like to suggest to consider to extend the level definitions so can allow us to express the data source for Arm arch precisely. It's important to understand current cache level definitions which is derived from x86 arch and think what's the good way to match and extend for Arm memory hierarchy. I will think a bit more for this, and if have any idea will share back. Thanks, Leo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-03-14 4:06 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-02-21 22:47 [PATCH v2 1/2] perf arm-spe: Use SPE data source for neoverse cores Ali Saidi 2022-02-21 22:47 ` Ali Saidi 2022-02-21 22:48 ` [PATCH v2 2/2] perf mem: Support HITM for when mem_lvl_num is used Ali Saidi 2022-02-21 22:48 ` Ali Saidi 2022-03-02 15:39 ` German Gomez 2022-03-02 15:39 ` German Gomez 2022-03-13 12:44 ` Leo Yan 2022-03-13 12:44 ` Leo Yan 2022-03-13 19:19 ` Ali Saidi 2022-03-13 19:19 ` Ali Saidi 2022-03-14 6:33 ` Leo Yan 2022-03-14 6:33 ` Leo Yan 2022-03-14 18:00 ` German Gomez 2022-03-14 18:00 ` German Gomez 2022-03-14 18:37 ` Ali Saidi 2022-03-14 18:37 ` Ali Saidi 2022-03-15 18:44 ` German Gomez 2022-03-15 18:44 ` German Gomez 2022-03-16 11:43 ` German Gomez 2022-03-16 11:43 ` German Gomez 2022-03-16 12:42 ` Leo Yan 2022-03-16 12:42 ` Leo Yan 2022-03-16 15:10 ` German Gomez 2022-03-16 15:10 ` German Gomez 2022-03-02 11:59 ` [PATCH v2 1/2] perf arm-spe: Use SPE data source for neoverse cores German Gomez 2022-03-02 11:59 ` German Gomez 2022-03-13 11:46 ` Leo Yan 2022-03-13 11:46 ` Leo Yan 2022-03-13 19:06 ` Ali Saidi 2022-03-13 19:06 ` Ali Saidi 2022-03-14 4:05 ` Leo Yan [this message] 2022-03-14 4:05 ` Leo Yan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220314040542.GA163961@leoy-ThinkPad-X240s \ --to=leo.yan@linaro.org \ --cc=acme@kernel.org \ --cc=alexander.shishkin@linux.intel.com \ --cc=alisaidi@amazon.com \ --cc=andrew.kilroy@arm.com \ --cc=benh@kernel.crashing.org \ --cc=german.gomez@arm.com \ --cc=james.clark@arm.com \ --cc=john.garry@huawei.com \ --cc=jolsa@redhat.com \ --cc=kjain@linux.ibm.com \ --cc=lihuafei1@huawei.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-perf-users@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mathieu.poirier@linaro.org \ --cc=mingo@redhat.com \ --cc=namhyung@kernel.org \ --cc=peterz@infradead.org \ --cc=will@kernel.org \ --cc=yao.jin@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.