Re: [PATCH v5 10/15] perf jevents: Generate metrics and events as separate tables

From: Ian Rogers <irogers@google.com>
To: John Garry <john.g.garry@oracle.com>
Cc: Will Deacon <will@kernel.org>, James Clark <james.clark@arm.com>,
	Mike Leach <mike.leach@linaro.org>, Leo Yan <leo.yan@linaro.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Kan Liang <kan.liang@linux.intel.com>,
	Kim Phillips <kim.phillips@amd.com>,
	Florian Fischer <florian.fischer@muhq.space>,
	Ravi Bangoria <ravi.bangoria@amd.com>,
	Xing Zhengjun <zhengjun.xing@linux.intel.com>,
	Rob Herring <robh@kernel.org>,
	Kang Minchul <tegongkang@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	Sandipan Das <sandipan.das@amd.com>,
	Jing Zhang <renyu.zj@linux.alibaba.com>,
	linuxppc-dev@lists.ozlabs.org, Kajol Jain <kjain@linux.ibm.com>,
	Stephane Eranian <eranian@google.com>,
	Perry Taylor <perry.taylor@intel.com>,
	Caleb Biggers <caleb.biggers@intel.com>
Subject: Re: [PATCH v5 10/15] perf jevents: Generate metrics and events as separate tables
Date: Mon, 30 Jan 2023 14:54:26 -0800	[thread overview]
Message-ID: <CAP-5=fUTT1gh0xd-um7NSyg=XuayjWzF90W6Dp4=DOvwyGcmNw@mail.gmail.com> (raw)
In-Reply-To: <8f53bb96-8213-12f1-f91c-7fb311e4c9a4@oracle.com>

On Mon, Jan 30, 2023 at 8:07 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 26/01/2023 23:36, Ian Rogers wrote:
> > @@ -660,7 +763,29 @@ const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu)
> >
> >   const struct pmu_metrics_table *perf_pmu__find_metrics_table(struct perf_pmu *pmu)
> >   {
> > -        return (struct pmu_metrics_table *)perf_pmu__find_events_table(pmu);
> > +        const struct pmu_metrics_table *table = NULL;
> > +        char *cpuid = perf_pmu__getcpuid(pmu);
> > +        int i;
> > +
> > +        /* on some platforms which uses cpus map, cpuid can be NULL for
> > +         * PMUs other than CORE PMUs.
> > +         */
> > +        if (!cpuid)
> > +                return NULL;
> > +
> > +        i = 0;
> > +        for (;;) {
> > +                const struct pmu_events_map *map = &pmu_events_map[i++];
> > +                if (!map->arch)
> > +                        break;
> > +
> > +                if (!strcmp_cpuid_str(map->cpuid, cpuid)) {
> > +                        table = &map->metric_table;
> > +                        break;
> > +                }
> > +        }
> > +        free(cpuid);
> > +        return table;
> >   }
>
> This is almost identical to generated perf_pmu__find_events_table(),
> except we return a pmu_metrics_table * (instead of a pmu_events_table *)
> and also return the metric table member (instead of event table). But
> the definitions are:
>
> /* Struct used to make the PMU event table implementation opaque to
> callers. */
> struct pmu_events_table {
>          const struct compact_pmu_event *entries;
>          size_t length;
> };
>
> /* Struct used to make the PMU metric table implementation opaque to
> callers. */
> struct pmu_metrics_table {
>          const struct compact_pmu_event *entries;
>          size_t length;
> };
>
> Those structs are defined to be the same thing, so I am failing to see
> the point in a) separate structure types b) why so much duplication
>
> As for b), I know that they are generated and the python code may be
> simpler this way (is it?), but still...

Agreed. The point is to separate the two tables for the typing at the
API layer, internally the representation is the same. When we decode
one we get a pmu_event and the other we get a pmu_metric, so we don't
want to allow the tables to be switched - hence two types.

Thanks,
Ian

> Thanks,
> John