Re: [PATCH v2] perf/x86/ibs: Set mem_lvl_num, mem_remote and mem_hops for data_src

[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@xxxxxxx> 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