Messages in this thread | | | Subject | Re: [PATCH v3 2/2] perf header: Support HYBRID_CPU_PMU_CAPS feature | From | "Jin, Yao" <> | Date | Fri, 14 May 2021 16:25:42 +0800 |
| |
Hi Jiri,
On 5/14/2021 4:16 PM, Jiri Olsa wrote: > 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 >
Yes. Actually I had the same idea before but as you said the code would be more complex.
> >> 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(... >
OK, I will fix it in v4.
>> +} >> + >> +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? >
Yes, I should add 'ret = -1' before 'n->pmu_name = do_read_string(ff);'.
> other than this both patches look good to me >
Thanks, I will prepare v4 soon.
Thanks Jin Yao
> 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 >
| |