Messages in this thread | | | From | Namhyung Kim <> | Date | Wed, 29 Mar 2023 16:47:52 -0700 | Subject | Re: [PATCH v2] perf/x86/ibs: Set mem_lvl_num, mem_remote and mem_hops for data_src |
| |
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.
> > 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.
> > $ 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 :)
> > 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.
Thanks, Namhyung
| |