linux-kernel - Re: [PATCH v2] perf/x86/ibs: Set mem_lvl_num, mem_remote and mem_hops for data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 29 Mar 2023 15:15:23 +0530
From:   Ravi Bangoria <ravi.bangoria@....com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     peterz@...radead.org, mingo@...nel.org, acme@...nel.org,
        eranian@...gle.com, kan.liang@...ux.intel.com, jolsa@...nel.org,
        irogers@...gle.com, linux-perf-users@...r.kernel.org,
        linux-kernel@...r.kernel.org, sandipan.das@....com,
        ananth.narayan@....com, santosh.shukla@....com,
        Ravi Bangoria <ravi.bangoria@....com>
Subject: Re: [PATCH v2] perf/x86/ibs: Set mem_lvl_num, mem_remote and mem_hops
 for data_src

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?

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:

  $ 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

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.

Thanks,
Ravi