Messages in this thread | | | Date | Sat, 19 Aug 2023 12:16:09 +0800 | Subject | Re: [PATCH v1] perf header: Fix missing PMU caps | From | "liwei (GF)" <> |
| |
Hi Ian:
On 2023/8/19 1:19, Ian Rogers wrote: > PMU caps are written as HEADER_PMU_CAPS or for the special case of the > PMU "cpu" as HEADER_CPU_PMU_CAPS. As the PMU "cpu" is special, and not > any "core" PMU, the logic had become broken and core PMUs not called > "cpu" were not having their caps written. This affects ARM and s390 > non-hybrid PMUs. > > Simplify the PMU caps writing logic to scan one fewer time and to be > more explicit in its behavior. > > Reported-by: Wei Li <liwei391@huawei.com> > Fixes: 178ddf3bad98 ("perf header: Avoid hybrid PMU list in write_pmu_caps") > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/header.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c > index 52fbf526fe74..13c71d28e0eb 100644 > --- a/tools/perf/util/header.c > +++ b/tools/perf/util/header.c > @@ -1605,8 +1605,15 @@ static int write_pmu_caps(struct feat_fd *ff, > int ret; > > while ((pmu = perf_pmus__scan(pmu))) { > - if (!pmu->name || !strcmp(pmu->name, "cpu") || > - perf_pmu__caps_parse(pmu) <= 0) > + if (!strcmp(pmu->name, "cpu")) {
So you removed the check of 'pmu->name', does this check really redundant? since we can find such checks in many places in the perf code. If not, i think it is necessary for strcmp().
> + /* > + * The "cpu" PMU is special and covered by > + * HEADER_CPU_PMU_CAPS. Note, core PMUs are > + * counted/written here for ARM, s390 and Intel hybrid. > + */ > + continue; > + } > + if (perf_pmu__caps_parse(pmu) <= 0) > continue; > nr_pmu++; > } > @@ -1619,23 +1626,17 @@ static int write_pmu_caps(struct feat_fd *ff, > return 0; > > /* > - * Write hybrid pmu caps first to maintain compatibility with > - * older perf tool. > + * Note older perf tools assume core PMUs come first, this is a property > + * of perf_pmus__scan. > */ > - if (perf_pmus__num_core_pmus() > 1) { > - pmu = NULL; > - while ((pmu = perf_pmus__scan_core(pmu))) { > - ret = __write_pmu_caps(ff, pmu, true); > - if (ret < 0) > - return ret; > - } > - } > - > pmu = NULL; > while ((pmu = perf_pmus__scan(pmu))) { > - if (pmu->is_core || !pmu->nr_caps) > + if (!strcmp(pmu->name, "cpu")) {
same here
Thanks, Wei
> + /* Skip as above. */ > + continue; > + } > + if (perf_pmu__caps_parse(pmu) <= 0) > continue; > - > ret = __write_pmu_caps(ff, pmu, true); > if (ret < 0) > return ret;
| |