Messages in this thread | | | Date | Thu, 30 Mar 2023 10:08:33 +0530 | Subject | Re: [PATCH v2] perf/x86/ibs: Set mem_lvl_num, mem_remote and mem_hops for data_src | From | Ravi Bangoria <> |
| |
On 30-Mar-23 5:17 AM, Namhyung Kim wrote: > On Wed, Mar 29, 2023 at 2:45 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote: >> >> Hi Namhyung, >> >>>> @@ -716,25 +748,19 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2, >>>> * DcMiss, L2Miss, DataSrc, DcMissLat etc. are all invalid for Uncached >>>> * memory accesses. So, check DcUcMemAcc bit early. >>>> */ >>>> - if (op_data3->dc_uc_mem_acc && ibs_data_src != IBS_DATA_SRC_EXT_IO) { >>>> - data_src->mem_lvl = PERF_MEM_LVL_UNC | PERF_MEM_LVL_HIT; >>>> - return; >>>> - } >>>> + if (op_data3->dc_uc_mem_acc && ibs_data_src != IBS_DATA_SRC_EXT_IO) >>>> + return L(UNC); >>> >>> Hmm.. it seems we don't have PERF_MEM_LVLNUM_UNC. >> >> Right. Is it worth to introduce one? > > I think MEM_LVLNUM should express every memory level in MEM_LVL.
Ok.
> >> >> On a side note, I came to know that IBS OpData2[RmtNode] is not applicable >> when DataSrc=7 (I/O). So, I need to respin this patch with that change. >> >> [...] >> >>>> check_mab: >>>> @@ -830,12 +810,11 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2, >>>> * DataSrc simultaneously. Prioritize DataSrc over MAB, i.e. set >>>> * MAB only when IBS fails to provide DataSrc. >>>> */ >>>> - if (op_data3->dc_miss_no_mab_alloc) { >>>> - data_src->mem_lvl = PERF_MEM_LVL_LFB | PERF_MEM_LVL_HIT; >>>> - return; >>>> - } >>>> + if (op_data3->dc_miss_no_mab_alloc) >>>> + return L(LFB) | LN(LFB); >>>> >>>> data_src->mem_lvl = PERF_MEM_LVL_NA; >>>> + return 0; >>> >>> Wouldn't it be 'return L(NA) | LN(NA);' ? >> >> IBS has no instruction type filtering, i.e. it tags whatever instruction it >> sees at overflow. When IBS tags non-load/store instruction, data_src->val is >> set to PERF_MEM_NA, which does not initialize mem_lvl_num (Shall we change >> that?). If I set both LVL_NA and LVL_NUM_NA for load/store with no DataSrc >> info, perf mem output becomes funny: > > Probably worth changing PERF_MEM_NA.
Yeah seems so.
> >> >> $ sudo ./perf mem report -F sample,mem --stdio >> # Samples Memory access >> # ............ ....................................... >> # >> 1914 N/A <====== Non-LS >> 905 L1 or L1 hit >> 19 L3 or L3 hit >> 16 L2 or L2 hit >> 6 N/A or N/A hit <====== LS with no DataSrc info >> 6 Local RAM or RAM hit >> 4 Remote node, same socket RAM hit >> 3 Remote core, same node Any cache hit >> 2 Remote node, same socket Any cache hit > > Maybe that's better to differentiate them :)
:). I would rather add instruction type column and print non-LS, L, S or LS.
> >> >> Also, L(NA) is PERF_MEM_LVL_NA | PERF_MEM_LVL_HIT. If I just return L(NA), >> perf tools shows it as "N/A hit". >> >> So, until tool code gets refactored, setting mem_lvl = NA here is hiding >> tool's dumbness :(. Maybe I should refactor perf_mem__snp_scnprintf() as >> part of this patchset. > > I think we can change the tool independently - preferring MEM_LVLNUM > if present but you might want to add an option to override. > Given IBS didn't set it so far, the output would remain mostly the same.
Sure.
Thanks, Ravi
| |