* [PATCH] perf/x86/ibs: Set data_src.mem_lvl_num as well
@ 2023-03-11 0:06 Namhyung Kim
2023-03-21 6:33 ` Ravi Bangoria
0 siblings, 1 reply; 5+ messages in thread
From: Namhyung Kim @ 2023-03-11 0:06 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar
Cc: Ravi Bangoria, Arnaldo Carvalho de Melo, Stephane Eranian,
Kan Liang, LKML
The IBS PMU driver sets data source of memory operations but it missed
mem_lvl_num field. So I'm adding them here.
Most cases are straight-forward but please check if MEM_LVLNUM_ANY_CACHE
for peer cache hits is ok. Also I wonder we can add MEM_REMOTE_REMOTE
for peer cache hits in a far CCX.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
arch/x86/events/amd/ibs.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 64582954b5f6..819869fc2dc7 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -724,6 +724,7 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
/* L1 Hit */
if (op_data3->dc_miss == 0) {
data_src->mem_lvl = PERF_MEM_LVL_L1 | PERF_MEM_LVL_HIT;
+ data_src->mem_lvl_num = PERF_MEM_LVLNUM_L1;
return;
}
@@ -733,6 +734,7 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
if (boot_cpu_data.x86 != 0x19 || boot_cpu_data.x86_model > 0xF ||
!(op_data3->sw_pf || op_data3->dc_miss_no_mab_alloc)) {
data_src->mem_lvl = PERF_MEM_LVL_L2 | PERF_MEM_LVL_HIT;
+ data_src->mem_lvl_num = PERF_MEM_LVLNUM_L2;
return;
}
}
@@ -748,12 +750,14 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
if (ibs_caps & IBS_CAPS_ZEN4) {
if (ibs_data_src == IBS_DATA_SRC_EXT_LOC_CACHE) {
data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_HIT;
+ data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
return;
}
} else {
if (ibs_data_src == IBS_DATA_SRC_LOC_CACHE) {
data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_REM_CCE1 |
PERF_MEM_LVL_HIT;
+ data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
return;
}
}
@@ -762,6 +766,7 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
if (ibs_caps & IBS_CAPS_ZEN4 &&
ibs_data_src == IBS_DATA_SRC_EXT_NEAR_CCX_CACHE) {
data_src->mem_lvl = PERF_MEM_LVL_REM_CCE1 | PERF_MEM_LVL_HIT;
+ data_src->mem_lvl_num = PERF_MEM_LVLNUM_ANY_CACHE;
return;
}
@@ -769,11 +774,13 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
if (ibs_caps & IBS_CAPS_ZEN4) {
if (ibs_data_src == IBS_DATA_SRC_EXT_FAR_CCX_CACHE) {
data_src->mem_lvl = PERF_MEM_LVL_REM_CCE2 | PERF_MEM_LVL_HIT;
+ data_src->mem_lvl_num = PERF_MEM_LVLNUM_ANY_CACHE;
return;
}
} else {
if (ibs_data_src == IBS_DATA_SRC_REM_CACHE) {
data_src->mem_lvl = PERF_MEM_LVL_REM_CCE2 | PERF_MEM_LVL_HIT;
+ data_src->mem_lvl_num = PERF_MEM_LVLNUM_ANY_CACHE;
return;
}
}
@@ -784,6 +791,7 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
data_src->mem_lvl = PERF_MEM_LVL_LOC_RAM | PERF_MEM_LVL_HIT;
else
data_src->mem_lvl = PERF_MEM_LVL_REM_RAM1 | PERF_MEM_LVL_HIT;
+ data_src->mem_lvl_num = PERF_MEM_LVLNUM_RAM;
return;
}
@@ -832,10 +840,12 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
*/
if (op_data3->dc_miss_no_mab_alloc) {
data_src->mem_lvl = PERF_MEM_LVL_LFB | PERF_MEM_LVL_HIT;
+ data_src->mem_lvl_num = PERF_MEM_LVLNUM_LFB;
return;
}
data_src->mem_lvl = PERF_MEM_LVL_NA;
+ data_src->mem_lvl_num = PERF_MEM_LVLNUM_NA;
}
static bool perf_ibs_cache_hit_st_valid(void)
--
2.40.0.rc1.284.g88254d51c5-goog
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] perf/x86/ibs: Set data_src.mem_lvl_num as well
2023-03-11 0:06 [PATCH] perf/x86/ibs: Set data_src.mem_lvl_num as well Namhyung Kim
@ 2023-03-21 6:33 ` Ravi Bangoria
2023-03-22 15:54 ` Namhyung Kim
0 siblings, 1 reply; 5+ messages in thread
From: Ravi Bangoria @ 2023-03-21 6:33 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Stephane Eranian, Kan Liang, LKML,
Peter Zijlstra, Ingo Molnar, Ravi Bangoria
Hi Namhyung,
> @@ -748,12 +750,14 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
> if (ibs_caps & IBS_CAPS_ZEN4) {
> if (ibs_data_src == IBS_DATA_SRC_EXT_LOC_CACHE) {
> data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_HIT;
> + data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
> return;
> }
> } else {
> if (ibs_data_src == IBS_DATA_SRC_LOC_CACHE) {
> data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_REM_CCE1 |
> PERF_MEM_LVL_HIT;
> + data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
mem_lvl_num does not have option to set multiple sources. Setting just
PERF_MEM_LVLNUM_L3 is bit misleading here. Documentation (PPR 55898 Rev
0.70 - Oct 14, 2022) says:
"data returned from shared L3, other L2 on same CCX or other core's
cache trough same node."
As per my knowledge, "shared L3" and "other L2 on same CCX" has similar
latency. But request need to go through DF for "other core's cache trough
same node" which incurs higher latency. Thus, setting both is important.
This was one of the reason to not use mem_lvl_num in IBS code.
2nd reason was, perf c2c (c2c_decode_stats()) does not use mem_lvl_num.
3rd reason was, perf mem sorting logic (sort__lvl_cmp()) does not consider
mem_lvl_num.
4th one was, if I set both mem_lvl and mem_lvl_num, like what other archs
do, `perf mem report` prints both, which is kind of ugly:
464029 N/A
340728 L1 or L1 hit
8312 LFB/MAB or LFB/MAB hit
7901 L2 or L2 hit
123 L3 or Remote Cache (1 hop) or L3 hit
Without mem_lvl_num it's much cleaner:
330057 N/A
229646 L1 hit
5842 L2 hit
5726 LFB/MAB hit
78 L3 or Remote Cache (1 hop) hit
I think we should clean this before applying this patch? Other option is
to add bpf filter support for mem_lvl. What do you think?
Thanks,
Ravi
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] perf/x86/ibs: Set data_src.mem_lvl_num as well
2023-03-21 6:33 ` Ravi Bangoria
@ 2023-03-22 15:54 ` Namhyung Kim
2023-03-23 14:11 ` Ravi Bangoria
0 siblings, 1 reply; 5+ messages in thread
From: Namhyung Kim @ 2023-03-22 15:54 UTC (permalink / raw)
To: Ravi Bangoria, Peter Zijlstra
Cc: Arnaldo Carvalho de Melo, Stephane Eranian, Kan Liang, LKML, Ingo Molnar
Hi Ravi,
On Mon, Mar 20, 2023 at 11:33 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Hi Namhyung,
>
> > @@ -748,12 +750,14 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
> > if (ibs_caps & IBS_CAPS_ZEN4) {
> > if (ibs_data_src == IBS_DATA_SRC_EXT_LOC_CACHE) {
> > data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_HIT;
> > + data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
> > return;
> > }
> > } else {
> > if (ibs_data_src == IBS_DATA_SRC_LOC_CACHE) {
> > data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_REM_CCE1 |
> > PERF_MEM_LVL_HIT;
> > + data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
>
> mem_lvl_num does not have option to set multiple sources. Setting just
> PERF_MEM_LVLNUM_L3 is bit misleading here. Documentation (PPR 55898 Rev
> 0.70 - Oct 14, 2022) says:
>
> "data returned from shared L3, other L2 on same CCX or other core's
> cache trough same node."
>
> As per my knowledge, "shared L3" and "other L2 on same CCX" has similar
> latency. But request need to go through DF for "other core's cache trough
> same node" which incurs higher latency. Thus, setting both is important.
> This was one of the reason to not use mem_lvl_num in IBS code.
I suspect it's a quality issue for CPUs prior to Zen4 not to identify
data source precisely. How about setting LVLNUM_ANY_CACHE then?
>
> 2nd reason was, perf c2c (c2c_decode_stats()) does not use mem_lvl_num.
Maybe we can change that. It'd be easy as long as they provide
the same information. IOW mem_lvl = mem_lvl_num + remote + snoop.
>
> 3rd reason was, perf mem sorting logic (sort__lvl_cmp()) does not consider
> mem_lvl_num.
Likewise.
>
> 4th one was, if I set both mem_lvl and mem_lvl_num, like what other archs
> do, `perf mem report` prints both, which is kind of ugly:
>
> 464029 N/A
> 340728 L1 or L1 hit
> 8312 LFB/MAB or LFB/MAB hit
> 7901 L2 or L2 hit
> 123 L3 or Remote Cache (1 hop) or L3 hit
>
> Without mem_lvl_num it's much cleaner:
>
> 330057 N/A
> 229646 L1 hit
> 5842 L2 hit
> 5726 LFB/MAB hit
> 78 L3 or Remote Cache (1 hop) hit
Agreed. It doesn't need to repeat the same information.
>
> I think we should clean this before applying this patch? Other option is
> to add bpf filter support for mem_lvl. What do you think?
I still prefer using mem_lvl_num as I think it's the way to go,
but I'm open for change.
Peter, what do you think?
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] perf/x86/ibs: Set data_src.mem_lvl_num as well
2023-03-22 15:54 ` Namhyung Kim
@ 2023-03-23 14:11 ` Ravi Bangoria
2023-03-23 14:41 ` Namhyung Kim
0 siblings, 1 reply; 5+ messages in thread
From: Ravi Bangoria @ 2023-03-23 14:11 UTC (permalink / raw)
To: Namhyung Kim
Cc: Peter Zijlstra, Arnaldo Carvalho de Melo, Stephane Eranian,
Kan Liang, LKML, Ingo Molnar, Ravi Bangoria
Hi Namhyung,
>>> @@ -748,12 +750,14 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
>>> if (ibs_caps & IBS_CAPS_ZEN4) {
>>> if (ibs_data_src == IBS_DATA_SRC_EXT_LOC_CACHE) {
>>> data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_HIT;
>>> + data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
>>> return;
>>> }
>>> } else {
>>> if (ibs_data_src == IBS_DATA_SRC_LOC_CACHE) {
>>> data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_REM_CCE1 |
>>> PERF_MEM_LVL_HIT;
>>> + data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
>>
>> mem_lvl_num does not have option to set multiple sources. Setting just
>> PERF_MEM_LVLNUM_L3 is bit misleading here. Documentation (PPR 55898 Rev
>> 0.70 - Oct 14, 2022) says:
>>
>> "data returned from shared L3, other L2 on same CCX or other core's
>> cache trough same node."
>>
>> As per my knowledge, "shared L3" and "other L2 on same CCX" has similar
>> latency. But request need to go through DF for "other core's cache trough
>> same node" which incurs higher latency. Thus, setting both is important.
>> This was one of the reason to not use mem_lvl_num in IBS code.
>
> I suspect it's a quality issue for CPUs prior to Zen4 not to identify
> data source precisely. How about setting LVLNUM_ANY_CACHE then?
Ok. Although, ANY_CACHE is mostly clueless, adding HOPS_0 will make it
more consumable. There are many other places where this patch needs to
set mem_remote and mem_hops. Also, these changes will result in too many
assignment operations. So, I think IBS code should switch to using
PERF_MEM_S() macro. Do you mind if I send v2 with all those changes?
>
>>
>> 2nd reason was, perf c2c (c2c_decode_stats()) does not use mem_lvl_num.
>
> Maybe we can change that. It'd be easy as long as they provide
> the same information. IOW mem_lvl = mem_lvl_num + remote + snoop.
>
>>
>> 3rd reason was, perf mem sorting logic (sort__lvl_cmp()) does not consider
>> mem_lvl_num.
>
> Likewise.
>
>>
>> 4th one was, if I set both mem_lvl and mem_lvl_num, like what other archs
>> do, `perf mem report` prints both, which is kind of ugly:
>>
>> 464029 N/A
>> 340728 L1 or L1 hit
>> 8312 LFB/MAB or LFB/MAB hit
>> 7901 L2 or L2 hit
>> 123 L3 or Remote Cache (1 hop) or L3 hit
>>
>> Without mem_lvl_num it's much cleaner:
>>
>> 330057 N/A
>> 229646 L1 hit
>> 5842 L2 hit
>> 5726 LFB/MAB hit
>> 78 L3 or Remote Cache (1 hop) hit
>
> Agreed. It doesn't need to repeat the same information.
>
>>
>> I think we should clean this before applying this patch? Other option is
>> to add bpf filter support for mem_lvl. What do you think?
>
> I still prefer using mem_lvl_num as I think it's the way to go,
> but I'm open for change.
Sure. 2nd, 3rd and 4th are all tool side improvements. Although it would
be good to fix those, let me post v2 of this patch for now?
Thanks,
Ravi
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] perf/x86/ibs: Set data_src.mem_lvl_num as well
2023-03-23 14:11 ` Ravi Bangoria
@ 2023-03-23 14:41 ` Namhyung Kim
0 siblings, 0 replies; 5+ messages in thread
From: Namhyung Kim @ 2023-03-23 14:41 UTC (permalink / raw)
To: Ravi Bangoria
Cc: Peter Zijlstra, Arnaldo Carvalho de Melo, Stephane Eranian,
Kan Liang, LKML, Ingo Molnar
On Thu, Mar 23, 2023 at 7:11 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Hi Namhyung,
>
> >>> @@ -748,12 +750,14 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
> >>> if (ibs_caps & IBS_CAPS_ZEN4) {
> >>> if (ibs_data_src == IBS_DATA_SRC_EXT_LOC_CACHE) {
> >>> data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_HIT;
> >>> + data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
> >>> return;
> >>> }
> >>> } else {
> >>> if (ibs_data_src == IBS_DATA_SRC_LOC_CACHE) {
> >>> data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_REM_CCE1 |
> >>> PERF_MEM_LVL_HIT;
> >>> + data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
> >>
> >> mem_lvl_num does not have option to set multiple sources. Setting just
> >> PERF_MEM_LVLNUM_L3 is bit misleading here. Documentation (PPR 55898 Rev
> >> 0.70 - Oct 14, 2022) says:
> >>
> >> "data returned from shared L3, other L2 on same CCX or other core's
> >> cache trough same node."
> >>
> >> As per my knowledge, "shared L3" and "other L2 on same CCX" has similar
> >> latency. But request need to go through DF for "other core's cache trough
> >> same node" which incurs higher latency. Thus, setting both is important.
> >> This was one of the reason to not use mem_lvl_num in IBS code.
> >
> > I suspect it's a quality issue for CPUs prior to Zen4 not to identify
> > data source precisely. How about setting LVLNUM_ANY_CACHE then?
>
> Ok. Although, ANY_CACHE is mostly clueless, adding HOPS_0 will make it
> more consumable. There are many other places where this patch needs to
> set mem_remote and mem_hops. Also, these changes will result in too many
> assignment operations. So, I think IBS code should switch to using
> PERF_MEM_S() macro. Do you mind if I send v2 with all those changes?
Sounds good!
>
> >
> >>
> >> 2nd reason was, perf c2c (c2c_decode_stats()) does not use mem_lvl_num.
> >
> > Maybe we can change that. It'd be easy as long as they provide
> > the same information. IOW mem_lvl = mem_lvl_num + remote + snoop.
> >
> >>
> >> 3rd reason was, perf mem sorting logic (sort__lvl_cmp()) does not consider
> >> mem_lvl_num.
> >
> > Likewise.
> >
> >>
> >> 4th one was, if I set both mem_lvl and mem_lvl_num, like what other archs
> >> do, `perf mem report` prints both, which is kind of ugly:
> >>
> >> 464029 N/A
> >> 340728 L1 or L1 hit
> >> 8312 LFB/MAB or LFB/MAB hit
> >> 7901 L2 or L2 hit
> >> 123 L3 or Remote Cache (1 hop) or L3 hit
> >>
> >> Without mem_lvl_num it's much cleaner:
> >>
> >> 330057 N/A
> >> 229646 L1 hit
> >> 5842 L2 hit
> >> 5726 LFB/MAB hit
> >> 78 L3 or Remote Cache (1 hop) hit
> >
> > Agreed. It doesn't need to repeat the same information.
> >
> >>
> >> I think we should clean this before applying this patch? Other option is
> >> to add bpf filter support for mem_lvl. What do you think?
> >
> > I still prefer using mem_lvl_num as I think it's the way to go,
> > but I'm open for change.
>
> Sure. 2nd, 3rd and 4th are all tool side improvements. Although it would
> be good to fix those, let me post v2 of this patch for now?
Sure, please go ahead.
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-23 14:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-11 0:06 [PATCH] perf/x86/ibs: Set data_src.mem_lvl_num as well Namhyung Kim
2023-03-21 6:33 ` Ravi Bangoria
2023-03-22 15:54 ` Namhyung Kim
2023-03-23 14:11 ` Ravi Bangoria
2023-03-23 14:41 ` Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®