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: <20230311000642.1270971-1-namhyung@kernel.org> <2154046a-2081-606d-a1ea-33fd2d48cce7@amd.com>
In-Reply-To: <2154046a-2081-606d-a1ea-33fd2d48cce7@amd.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Wed, 22 Mar 2023 08:54:33 -0700
Message-ID: <CAM9d7chDaKn57UT9D+BeZBwAQ3Mo4bVgJpign4O0De=9sFsu=w@mail.gmail.com>
Subject: Re: [PATCH] perf/x86/ibs: Set data_src.mem_lvl_num as well
To:     Ravi Bangoria <ravi.bangoria@amd.com>,
        Peter Zijlstra <peterz@infradead.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Stephane Eranian <eranian@google.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hi Ravi,

On Mon, Mar 20, 2023 at 11:33 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Hi Namhyung,
>
> > @@ -748,12 +750,14 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2,
> >       if (ibs_caps & IBS_CAPS_ZEN4) {
> >               if (ibs_data_src == IBS_DATA_SRC_EXT_LOC_CACHE) {
> >                       data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_HIT;
> > +                     data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
> >                       return;
> >               }
> >       } else {
> >               if (ibs_data_src == IBS_DATA_SRC_LOC_CACHE) {
> >                       data_src->mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_REM_CCE1 |
> >                                           PERF_MEM_LVL_HIT;
> > +                     data_src->mem_lvl_num = PERF_MEM_LVLNUM_L3;
>
> mem_lvl_num does not have option to set multiple sources. Setting just
> PERF_MEM_LVLNUM_L3 is bit misleading here. Documentation (PPR 55898 Rev
> 0.70 - Oct 14, 2022) says:
>
>  "data returned from shared L3, other L2 on same CCX or other core's
>   cache trough same node."
>
> As per my knowledge, "shared L3" and "other L2 on same CCX" has similar
> latency. But request need to go through DF for "other core's cache trough
> same node" which incurs higher latency. Thus, setting both is important.
> This was one of the reason to not use mem_lvl_num in IBS code.

I suspect it's a quality issue for CPUs prior to Zen4 not to identify
data source precisely.  How about setting LVLNUM_ANY_CACHE then?

>
> 2nd reason was, perf c2c (c2c_decode_stats()) does not use mem_lvl_num.

Maybe we can change that.  It'd be easy as long as they provide
the same information.  IOW mem_lvl = mem_lvl_num + remote + snoop.

>
> 3rd reason was, perf mem sorting logic (sort__lvl_cmp()) does not consider
> mem_lvl_num.

Likewise.

>
> 4th one was, if I set both mem_lvl and mem_lvl_num, like what other archs
> do, `perf mem report` prints both, which is kind of ugly:
>
>           464029  N/A
>           340728  L1 or L1 hit
>             8312  LFB/MAB or LFB/MAB hit
>             7901  L2 or L2 hit
>              123  L3 or Remote Cache (1 hop) or L3 hit
>
> Without mem_lvl_num it's much cleaner:
>
>           330057  N/A
>           229646  L1 hit
>             5842  L2 hit
>             5726  LFB/MAB hit
>               78  L3 or Remote Cache (1 hop) hit

Agreed.  It doesn't need to repeat the same information.

>
> I think we should clean this before applying this patch? Other option is
> to add bpf filter support for mem_lvl. What do you think?

I still prefer using mem_lvl_num as I think it's the way to go,
but I'm open for change.

Peter, what do you think?

Thanks,
Namhyung