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

["Jin, Yao" <yao.jin@linux.intel.com>]

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
>