Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230327130851.1565-1-ravi.bangoria@amd.com> <CAM9d7ciQLXAcVwFJWeJDHwkwUQ-rnHvdetYsQn7w83kkOsFKWg@mail.gmail.com>
 <00f1954f-cf55-d954-5dc6-0da95f8ea308@amd.com>
In-Reply-To: <00f1954f-cf55-d954-5dc6-0da95f8ea308@amd.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Wed, 29 Mar 2023 16:47:52 -0700
Message-ID: <CAM9d7cgHL=aXffUtu7CSNdfsVnDoO1cEzBSSpdYVQh9ZrbL-zQ@mail.gmail.com>
Subject: Re: [PATCH v2] perf/x86/ibs: Set mem_lvl_num, mem_remote and mem_hops
 for data_src
To:     Ravi Bangoria <ravi.bangoria@amd.com>
Cc:     peterz@infradead.org, mingo@kernel.org, acme@kernel.org,
        eranian@google.com, kan.liang@linux.intel.com, jolsa@kernel.org,
        irogers@google.com, linux-perf-users@vger.kernel.org,
        linux-kernel@vger.kernel.org, sandipan.das@amd.com,
        ananth.narayan@amd.com, santosh.shukla@amd.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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