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>
 <CAM9d7chDaKn57UT9D+BeZBwAQ3Mo4bVgJpign4O0De=9sFsu=w@mail.gmail.com> <49de2976-6e8e-0987-b423-ad65ae3bd148@amd.com>
In-Reply-To: <49de2976-6e8e-0987-b423-ad65ae3bd148@amd.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Thu, 23 Mar 2023 07:41:28 -0700
Message-ID: <CAM9d7cj1oBN3o2Mf--7QpE0Qj+4RzDSX+N2RvG5_hTLQo1rTjg@mail.gmail.com>
Subject: Re: [PATCH] perf/x86/ibs: Set data_src.mem_lvl_num as well
To:     Ravi Bangoria <ravi.bangoria@amd.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        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

On Thu, Mar 23, 2023 at 7:11 AM 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?
>
> Ok. Although, ANY_CACHE is mostly clueless, adding HOPS_0 will make it
> more consumable. There are many other places where this patch needs to
> set mem_remote and mem_hops. Also, these changes will result in too many
> assignment operations. So, I think IBS code should switch to using
> PERF_MEM_S() macro. Do you mind if I send v2 with all those changes?

Sounds good!

>
> >
> >>
> >> 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.
>
> Sure. 2nd, 3rd and 4th are all tool side improvements. Although it would
> be good to fix those, let me post v2 of this patch for now?

Sure, please go ahead.

Thanks,
Namhyung