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

Jiri Olsa
2021-05-14T08:16:28Z

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