Messages in this thread | | | Date | Thu, 26 May 2022 11:55:09 -0400 | Subject | Re: [PATCH v4 2/5] perf header: Parse non-cpu pmu capabilities | From | "Liang, Kan" <> |
| |
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
| |