Messages in this thread | | | Date | Fri, 14 May 2021 10:16:18 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH v3 2/2] perf header: Support HYBRID_CPU_PMU_CAPS feature |
| |
On Tue, May 11, 2021 at 01:30:03PM +0800, Jin Yao wrote:
SNIP
> diff --git a/tools/perf/Documentation/perf.data-file-format.txt b/tools/perf/Documentation/perf.data-file-format.txt > index fbee9e580ee4..e6ff8c898ada 100644 > --- a/tools/perf/Documentation/perf.data-file-format.txt > +++ b/tools/perf/Documentation/perf.data-file-format.txt > @@ -419,6 +419,22 @@ Example: > cpu_core cpu list : 0-15 > cpu_atom cpu list : 16-23 > > + HEADER_HYBRID_CPU_PMU_CAPS = 31, > + > + A list of hybrid CPU PMU capabilities. > + > +struct { > + u32 nr_pmu; > + struct { > + u32 nr_cpu_pmu_caps; > + { > + char name[]; > + char value[]; > + } [nr_cpu_pmu_caps]; > + char pmu_name[]; > + } [nr_pmu]; > +};
when I saw it's similar to the previous one I thought we could have one big hybrid feature.. but that would be probably more complex and we might not be able to reuse the code as much as you did
> free_value: > @@ -3142,6 +3208,64 @@ static int process_cpu_pmu_caps(struct feat_fd *ff, > return -1; > } > > +static int process_cpu_pmu_caps(struct feat_fd *ff, > + void *data __maybe_unused) > +{ > + int ret; > + > + ret = process_per_cpu_pmu_caps(ff, &ff->ph->env.nr_cpu_pmu_caps, > + &ff->ph->env.cpu_pmu_caps, > + &ff->ph->env.max_branches); > + return ret;
why the 'ret' var? could be just:
return process_per_cpu_pmu_caps(...
> +} > + > +static int process_hybrid_cpu_pmu_caps(struct feat_fd *ff, > + void *data __maybe_unused) > +{ > + struct hybrid_cpc_node *nodes; > + u32 nr_pmu, i; > + int ret; > + > + if (do_read_u32(ff, &nr_pmu)) > + return -1; > + > + if (!nr_pmu) { > + pr_debug("hybrid cpu pmu capabilities not available\n"); > + return 0; > + } > + > + nodes = zalloc(sizeof(*nodes) * nr_pmu); > + if (!nodes) > + return -ENOMEM; > + > + for (i = 0; i < nr_pmu; i++) { > + struct hybrid_cpc_node *n = &nodes[i]; > + > + ret = process_per_cpu_pmu_caps(ff, &n->nr_cpu_pmu_caps, > + &n->cpu_pmu_caps, > + &n->max_branches); > + if (ret) > + goto err; > + > + n->pmu_name = do_read_string(ff); > + if (!n->pmu_name)
should you set 'ret = -1' in here?
other than this both patches look good to me
thanks, jirka
> + goto err; > + } > + > + ff->ph->env.nr_hybrid_cpc_nodes = nr_pmu; > + ff->ph->env.hybrid_cpc_nodes = nodes; > + return 0; > + > +err: > + for (i = 0; i < nr_pmu; i++) { > + free(nodes[i].cpu_pmu_caps);
SNIP
| |