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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 29 Mar 2023 16:47:52 -0700
From:   Namhyung Kim <namhyung@...nel.org>
To:     Ravi Bangoria <ravi.bangoria@....com>
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
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@....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