Received-SPF: pass (google.com: domain of linux-kernel+bounces-154469-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240103165438.633054-1-samuel.holland@sifive.com>
 <CAOnJCUJ4rC+Rrs6GV4t+=NWA=LtTZix5Nk1VzgP9CK-3+5-jAg@mail.gmail.com>
 <06e71142-e113-40ac-b2c2-b20893aa714f@sifive.com> <a4c4c9f2-9fe1-4c22-a99c-1667481ddd6b@rivosinc.com>
 <CAAhSdy1yoCMtR52X7tS6dB2x_ysgA8K4hSSXH044bbq2uFY7jQ@mail.gmail.com>
In-Reply-To: <CAAhSdy1yoCMtR52X7tS6dB2x_ysgA8K4hSSXH044bbq2uFY7jQ@mail.gmail.com>
From: Ian Rogers <irogers@google.com>
Date: Mon, 22 Apr 2024 20:44:35 -0700
Message-ID: <CAP-5=fX5JRYxZ26buLujSVP7roQTaWofqdiX1y1rAKQ70Yu+QQ@mail.gmail.com>
Subject: Re: [PATCH] perf: RISC-V: Check standard event availability
To: Anup Patel <anup@brainfault.org>, Ian Rogers <irogers@google.com>
Cc: Atish Patra <atishp@rivosinc.com>, Samuel Holland <samuel.holland@sifive.com>, 
	Atish Patra <atishp@atishpatra.org>, Albert Ou <aou@eecs.berkeley.edu>, 
	Mark Rutland <mark.rutland@arm.com>, Palmer Dabbelt <palmer@dabbelt.com>, 
	Paul Walmsley <paul.walmsley@sifive.com>, Will Deacon <will@kernel.org>, 
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, 
	linux-riscv@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 15, 2024 at 9:07=E2=80=AFPM Anup Patel <anup@brainfault.org> wr=
ote:
>
> On Tue, Apr 16, 2024 at 5:31=E2=80=AFAM Atish Patra <atishp@rivosinc.com>=
 wrote:
> >
> > On 4/10/24 18:40, Samuel Holland wrote:
> > > Hi Atish,
> > >
> > > On 2024-03-18 2:44 PM, Atish Patra wrote:
> > >> On Wed, Jan 3, 2024 at 8:54=E2=80=AFAM Samuel Holland <samuel.hollan=
d@sifive.com> wrote:
> > >>>
> > >>> The RISC-V SBI PMU specification defines several standard hardware =
and
> > >>> cache events. Currently, all of these events appear in the `perf li=
st`
> > >>> output, even if they are not actually implemented. Add logic to che=
ck
> > >>> which events are supported by the hardware (i.e. can be mapped to s=
ome
> > >>> counter), so only usable events are reported to userspace.
> > >>>
> > >>
> > >> Thanks for the patch.
> > >> This adds tons of SBI calls at every boot for a use case which is at
> > >> best confusing for a subset of users who actually wants to run perf.
> > >
> > > I should have been clearer in the patch description. This is not just=
 a cosmetic
> > > change; because perf sees these as valid events, it tries to use them=
 in
> > > commands like `perf stat`. When the error from SBI_EXT_PMU_COUNTER_CF=
G_MATCH
> > > causes the ->add() callback to fail, this prevents any other events f=
rom being
> > > scheduled on that same CPU (search can_add_hw in kernel/events/core.c=
). That is
> > > why the dTLB/iTLB miss counts are missing in the "before" example bel=
ow.
> > >
> >
> > Thanks for explaining the problem. I can reproduce it in qemu as well i=
f
> > enough number of invalid events given on the command line and the
> > workload is short enough.
> >
> > >> This probing can be done at runtime by invoking the
> > >> pmu_sbi_check_event from pmu_sbi_event_map.
> > >> We can update the event map after that so that it doesn't need to
> > >> invoke pmu_sbi_check_event next time.
> > >
> > > I tried to implement this suggestion, but it did not work. The SBI in=
terface
> > > does not distinguish between "none of the counters can monitor the sp=
ecified
> > > event [because the event is unsupported]" and "none of the counters c=
an monitor
> > > the specified event [because the counters are busy]". It is not suffi=
cient for
> > > the kernel to verify that at least one counter is available before pe=
rforming
> > > the check, because certain events may only be usable on a subset of c=
ounters
> > > (per riscv,event-to-mhpmcounters), and the kernel does not know that =
mapping.
> > >
> >
> > Yeah. My suggestion was to fix the perf list issue which is different
> > than the issue reported now.
> >
> > > As a result, checking for event support is only reliable when none of=
 the
> > > counters are in use. So the check can be asynchronous/deferred to lat=
er in the
> > > boot process, but I believe it still needs to be performed for all ev=
ents before
> > > userspace starts using the counters.
> > >
> >
> > We should defer it a work queue for sure. We can also look at improving
> > SBI PMU extension to support bulk matching behavior as well.
> >
> > However, I think a better solution would be to just rely on the json
> > file mappings instead of making SBI calls. We are going to have the
> > event encoding and mappings in the json in the future.
>
> The problem with JSON based event encoding is how to deal in-case
> we are running inside Guest/VM because Host could be anything.
>
> IMO, the JSON based approach is not suitable for SBI PMU. For now,
> we either defer the work using the work queue or keep the approach
> of this patch as-is.
>
> The good part about SBI PMU extension is that we do have a standard
> set of events and we only need a way to discover supported standard
> events with a minimum number of SBI calls. It is better to add a new
> SBI PMU call to assist supported event discovery which will also
> help us virtualize it.
>
> Regards,
> Anup

+Ian Rogers

`perf list` will already filter some events depending on whether the
PMU supports them, for example, legacy cache events. I think we can
extend this to json events.

Thanks,
Ian

> >
> > I had added it only for platforms with counter delegation[1] but I thin=
k
> > this can be generalized for platforms with SBI PMU as well.
> >
> > I had some hacks to specify the legacy event encodings but Ian rogers
> > improved with a generic support by preferring sysfs/json event encoding=
s
> > over fixed ones. I am yet to rebase and try Ian's series on top of the
> > counter delegation though. Thoughts ?
> >
> > [1]
> > https://lore.kernel.org/lkml/20240217005738.3744121-1-atishp@rivosinc.c=
om/
> > [2]
> > https://lore.kernel.org/bpf/20240415063626.453987-2-irogers@google.com/=
T/
> >
> >
> > > Regards,
> > > Samuel
> > >
> > >>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> > >>> ---
> > >>> Before this patch:
> > >>> $ perf list hw
> > >>>
> > >>> List of pre-defined events (to be used in -e or -M):
> > >>>
> > >>>    branch-instructions OR branches                    [Hardware eve=
nt]
> > >>>    branch-misses                                      [Hardware eve=
nt]
> > >>>    bus-cycles                                         [Hardware eve=
nt]
> > >>>    cache-misses                                       [Hardware eve=
nt]
> > >>>    cache-references                                   [Hardware eve=
nt]
> > >>>    cpu-cycles OR cycles                               [Hardware eve=
nt]
> > >>>    instructions                                       [Hardware eve=
nt]
> > >>>    ref-cycles                                         [Hardware eve=
nt]
> > >>>    stalled-cycles-backend OR idle-cycles-backend      [Hardware eve=
nt]
> > >>>    stalled-cycles-frontend OR idle-cycles-frontend    [Hardware eve=
nt]
> > >>>
> > >>> $ perf stat -ddd true
> > >>>
> > >>>   Performance counter stats for 'true':
> > >>>
> > >>>                4.36 msec task-clock                       #    0.74=
4 CPUs utilized
> > >>>                   1      context-switches                 #  229.32=
5 /sec
> > >>>                   0      cpu-migrations                   #    0.00=
0 /sec
> > >>>                  38      page-faults                      #    8.71=
4 K/sec
> > >>>           4,375,694      cycles                           #    1.00=
3 GHz                         (60.64%)
> > >>>             728,945      instructions                     #    0.17=
  insn per cycle
> > >>>              79,199      branches                         #   18.16=
2 M/sec
> > >>>              17,709      branch-misses                    #   22.36=
% of all branches
> > >>>             181,734      L1-dcache-loads                  #   41.67=
6 M/sec
> > >>>               5,547      L1-dcache-load-misses            #    3.05=
% of all L1-dcache accesses
> > >>>       <not counted>      LLC-loads                                 =
                              (0.00%)
> > >>>       <not counted>      LLC-load-misses                           =
                              (0.00%)
> > >>>       <not counted>      L1-icache-loads                           =
                              (0.00%)
> > >>>       <not counted>      L1-icache-load-misses                     =
                              (0.00%)
> > >>>       <not counted>      dTLB-loads                                =
                              (0.00%)
> > >>>       <not counted>      dTLB-load-misses                          =
                              (0.00%)
> > >>>       <not counted>      iTLB-loads                                =
                              (0.00%)
> > >>>       <not counted>      iTLB-load-misses                          =
                              (0.00%)
> > >>>       <not counted>      L1-dcache-prefetches                      =
                              (0.00%)
> > >>>       <not counted>      L1-dcache-prefetch-misses                 =
                              (0.00%)
> > >>>
> > >>>         0.005860375 seconds time elapsed
> > >>>
> > >>>         0.000000000 seconds user
> > >>>         0.010383000 seconds sys
> > >>>
> > >>> After this patch:
> > >>> $ perf list hw
> > >>>
> > >>> List of pre-defined events (to be used in -e or -M):
> > >>>
> > >>>    branch-instructions OR branches                    [Hardware eve=
nt]
> > >>>    branch-misses                                      [Hardware eve=
nt]
> > >>>    cache-misses                                       [Hardware eve=
nt]
> > >>>    cache-references                                   [Hardware eve=
nt]
> > >>>    cpu-cycles OR cycles                               [Hardware eve=
nt]
> > >>>    instructions                                       [Hardware eve=
nt]
> > >>>
> > >>> $ perf stat -ddd true
> > >>>
> > >>>   Performance counter stats for 'true':
> > >>>
> > >>>                5.16 msec task-clock                       #    0.84=
8 CPUs utilized
> > >>>                   1      context-switches                 #  193.81=
7 /sec
> > >>>                   0      cpu-migrations                   #    0.00=
0 /sec
> > >>>                  37      page-faults                      #    7.17=
1 K/sec
> > >>>           5,183,625      cycles                           #    1.00=
5 GHz
> > >>>             961,696      instructions                     #    0.19=
  insn per cycle
> > >>>              85,853      branches                         #   16.64=
0 M/sec
> > >>>              20,462      branch-misses                    #   23.83=
% of all branches
> > >>>             243,545      L1-dcache-loads                  #   47.20=
3 M/sec
> > >>>               5,974      L1-dcache-load-misses            #    2.45=
% of all L1-dcache accesses
> > >>>     <not supported>      LLC-loads
> > >>>     <not supported>      LLC-load-misses
> > >>>     <not supported>      L1-icache-loads
> > >>>     <not supported>      L1-icache-load-misses
> > >>>     <not supported>      dTLB-loads
> > >>>              19,619      dTLB-load-misses
> > >>>     <not supported>      iTLB-loads
> > >>>               6,831      iTLB-load-misses
> > >>>     <not supported>      L1-dcache-prefetches
> > >>>     <not supported>      L1-dcache-prefetch-misses
> > >>>
> > >>>         0.006085625 seconds time elapsed
> > >>>
> > >>>         0.000000000 seconds user
> > >>>         0.013022000 seconds sys
> > >>>
> > >>>
> > >>>   drivers/perf/riscv_pmu_sbi.c | 37 +++++++++++++++++++++++++++++++=
+++--
> > >>>   1 file changed, 35 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_=
sbi.c
> > >>> index 16acd4dcdb96..b58a70ee8317 100644
> > >>> --- a/drivers/perf/riscv_pmu_sbi.c
> > >>> +++ b/drivers/perf/riscv_pmu_sbi.c
> > >>> @@ -86,7 +86,7 @@ struct sbi_pmu_event_data {
> > >>>          };
> > >>>   };
> > >>>
> > >>> -static const struct sbi_pmu_event_data pmu_hw_event_map[] =3D {
> > >>> +static struct sbi_pmu_event_data pmu_hw_event_map[] =3D {
> > >>>          [PERF_COUNT_HW_CPU_CYCLES]              =3D {.hw_gen_event=
 =3D {
> > >>>                                                          SBI_PMU_HW=
_CPU_CYCLES,
> > >>>                                                          SBI_PMU_EV=
ENT_TYPE_HW, 0}},
> > >>> @@ -120,7 +120,7 @@ static const struct sbi_pmu_event_data pmu_hw_e=
vent_map[] =3D {
> > >>>   };
> > >>>
> > >>>   #define C(x) PERF_COUNT_HW_CACHE_##x
> > >>> -static const struct sbi_pmu_event_data pmu_cache_event_map[PERF_CO=
UNT_HW_CACHE_MAX]
> > >>> +static struct sbi_pmu_event_data pmu_cache_event_map[PERF_COUNT_HW=
_CACHE_MAX]
> > >>>   [PERF_COUNT_HW_CACHE_OP_MAX]
> > >>>   [PERF_COUNT_HW_CACHE_RESULT_MAX] =3D {
> > >>>          [C(L1D)] =3D {
> > >>> @@ -265,6 +265,36 @@ static const struct sbi_pmu_event_data pmu_cac=
he_event_map[PERF_COUNT_HW_CACHE_M
> > >>>          },
> > >>>   };
> > >>>
> > >>> +static void pmu_sbi_check_event(struct sbi_pmu_event_data *edata)
> > >>> +{
> > >>> +       struct sbiret ret;
> > >>> +
> > >>> +       ret =3D sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATC=
H,
> > >>> +                       0, cmask, 0, edata->event_idx, 0, 0);
> > >>> +       if (!ret.error) {
> > >>> +               sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP,
> > >>> +                         ret.value, 0x1, SBI_PMU_STOP_FLAG_RESET, =
0, 0, 0);
> > >>> +       } else if (ret.error =3D=3D SBI_ERR_NOT_SUPPORTED) {
> > >>> +               /* This event cannot be monitored by any counter */
> > >>> +               edata->event_idx =3D -EINVAL;
> > >>> +       }
> > >>> +}
> > >>> +
> > >>> +static void pmu_sbi_update_events(void)
> > >>> +{
> > >>> +       /* Ensure events are not already mapped to a counter */
> > >>> +       sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP,
> > >>> +                 0, cmask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0);
> > >>> +
> > >>> +       for (int i =3D 0; i < ARRAY_SIZE(pmu_hw_event_map); i++)
> > >>> +               pmu_sbi_check_event(&pmu_hw_event_map[i]);
> > >>> +
> > >>> +       for (int i =3D 0; i < ARRAY_SIZE(pmu_cache_event_map); i++)
> > >>> +               for (int j =3D 0; j < ARRAY_SIZE(pmu_cache_event_ma=
p[i]); j++)
> > >>> +                       for (int k =3D 0; k < ARRAY_SIZE(pmu_cache_=
event_map[i][j]); k++)
> > >>> +                               pmu_sbi_check_event(&pmu_cache_even=
t_map[i][j][k]);
> > >>> +}
> > >>> +
> > >>>   static int pmu_sbi_ctr_get_width(int idx)
> > >>>   {
> > >>>          return pmu_ctr_list[idx].width;
> > >>> @@ -1046,6 +1076,9 @@ static int pmu_sbi_device_probe(struct platfo=
rm_device *pdev)
> > >>>          if (pmu_sbi_get_ctrinfo(num_counters, &cmask))
> > >>>                  goto out_free;
> > >>>
> > >>> +       /* Check which standard events are available */
> > >>> +       pmu_sbi_update_events();
> > >>> +
> > >>>          ret =3D pmu_sbi_setup_irqs(pmu, pdev);
> > >>>          if (ret < 0) {
> > >>>                  pr_info("Perf sampling/filtering is not supported =
as sscof extension is not available\n");
> > >>> --
> > >>> 2.42.0
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
>