[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <adf43431-f828-75b2-359e-652c5fc96dc7@linux.intel.com>
Date: Thu, 26 May 2022 11:55:09 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Ravi Bangoria <ravi.bangoria@....com>
Cc: acme@...nel.org, irogers@...gle.com, peterz@...radead.org,
rrichter@....com, mingo@...hat.com, mark.rutland@....com,
jolsa@...nel.org, namhyung@...nel.org, tglx@...utronix.de,
bp@...en8.de, james.clark@....com, leo.yan@...aro.org,
ak@...ux.intel.com, eranian@...gle.com, like.xu.linux@...il.com,
x86@...nel.org, linux-perf-users@...r.kernel.org,
linux-kernel@...r.kernel.org, sandipan.das@....com,
ananth.narayan@....com, kim.phillips@....com,
santosh.shukla@....com
Subject: Re: [PATCH v4 2/5] perf header: Parse non-cpu pmu capabilities
On 5/26/2022 11:08 AM, Ravi Bangoria wrote:
> Hi Kan,
>
> [...]
>
>>> +static int write_pmu_caps(struct feat_fd *ff, struct evlist *evlist __maybe_unused)
>>> +{
>>> + struct perf_pmu_caps *caps = NULL;
>>> + struct perf_pmu *pmu = NULL;
>>> + u32 nr_pmus = 0;
>>> + int ret;
>>> +
>>> + while ((pmu = perf_pmu__scan(pmu))) {
>>> + if (!pmu->name || !strncmp(pmu->name, "cpu", 3) ||
>>> + perf_pmu__caps_parse(pmu) <= 0)
>>> + continue;
>>> + nr_pmus++;
>>> + }
>>> +
>>> + ret = do_write(ff, &nr_pmus, sizeof(nr_pmus));
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (!nr_pmus)
>>> + return 0;
>>> +
>>> + while ((pmu = perf_pmu__scan(pmu))) {
>>> + if (!pmu->name || !strncmp(pmu->name, "cpu", 3) || !pmu->nr_caps)
>>> + continue;
>>> +
>>> + ret = do_write_string(ff, pmu->name);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = do_write(ff, &pmu->nr_caps, sizeof(pmu->nr_caps));
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + list_for_each_entry(caps, &pmu->caps, list) {
>>> + ret = do_write_string(ff, caps->name);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = do_write_string(ff, caps->value);
>>> + if (ret < 0)
>>> + return ret;
>>> + }
>>> + }
>>
>> The write_per_cpu_pmu_caps() also does a similar thing. Can we factor out a generic write_pmu_caps() which works for both cpu and non-cpu pmu capabilities?
>
> I might be able to do this but..
>
>> It seems the print_pmu_caps()/process_pmu_caps() can also does similar factor out.
>
> not this, see below..
>
>> Actually, more aggressively, why not use the HEADER_PMU_CAPS to replace the HEADER_HYBRID_CPU_PMU_CAPS? The HEADER_HYBRID_CPU_PMU_CAPS is the last header feature. It seems doable. We can always write/print the "cpu_" kind of PMU first to be compatible with the old tools.
>
> There are some differences in how capabilities are stored in perf.data header
> as well as perf_env. In case of HEADER_CPU_PMU_CAPS or
> HEADER_HYBRID_CPU_PMU_CAPS, all capabilities are stored in a single string
> separated by NULL character.
I think this is the format for the internal string, not the format of
the perf.data header.
For the perf.data, here is the existing format for the
HEADER_HYBRID_CPU_PMU_CAPS.
struct {
u32 nr_pmu;
struct {
u32 nr_cpu_pmu_caps;
{
char name[];
char value[];
} [nr_cpu_pmu_caps];
char pmu_name[];
} [nr_pmu];
};
Here is your proposal.
+struct {
+ u32 nr_pmus;
+ struct {
+ char pmu_name[];
+ u32 nr_caps;
+ struct {
+ char name[];
+ char value[];
+ } [nr_caps];
+ } [nr_pmus];
+};
From my understanding, they are the same. (It doesn't matter where we
put the char pmu_name[];)
That's also why I think we should merge the HEADER_HYBRID_CPU_PMU_CAPS
and HEADER_PMU_CAPS. I don't think it make senses to basically handle
the same thing with different codes.
> Whereas, in case of HEADER_PMU_CAPS, they are
> stored as an array of strings. The reason for this difference is, searching
> in an array is far easier compared to searching in a NULL separated string.
I think the hybrid_cpc_node can be replaced by the env_pmu_caps.
Then you don't need to modify the perf_env__find_pmu_cap().
Thanks,
Kan
> So, I don't think I can extend HEADER_HYBRID_CPU_PMU_CAPS as HEADER_PMU_CAPS
> without adding complexity in perf_env__find_pmu_cap().
>
> Thanks for the review,
> Ravi
Powered by blists - more mailing lists