Re: [PATCH v3 2/2] perf header: Support HYBRID_CPU_PMU_CAPS feature

Jin, Yao
2021-05-14T08:25:49Z

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
>