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

[Jin, Yao]

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