Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

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

Hi Jiri,

On 5/4/2021 10:54 PM, Jiri Olsa wrote:
> On Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao wrote:
> 
> SNIP
> 
>> +static int write_hybrid_topology(struct feat_fd *ff,
>> +				 struct evlist *evlist __maybe_unused)
>> +{
>> +	struct hybrid_topology *tp;
>> +	int ret;
>> +	u32 i;
>> +
>> +	tp = hybrid_topology__new();
>> +	if (!tp)
>> +		return -1;
>> +
>> +	ret = do_write(ff, &tp->nr, sizeof(u32));
>> +	if (ret < 0)
>> +		goto err;
>> +
>> +	for (i = 0; i < tp->nr; i++) {
>> +		struct hybrid_topology_node *n = &tp->nodes[i];
>> +
>> +		ret = do_write_string(ff, n->pmu_name);
>> +		if (ret < 0)
>> +			goto err;
>> +
>> +		ret = do_write_string(ff, n->cpus);
>> +		if (ret < 0)
>> +			goto err;
>> +	}
>> +
>> +	ret = 0;
>> +
>> +err:
>> +	hybrid_topology__delete(tp);
>> +	return ret;
>> +}
>> +
>>   static int write_dir_format(struct feat_fd *ff,
>>   			    struct evlist *evlist __maybe_unused)
>>   {
>> @@ -1623,6 +1657,19 @@ static void print_clock_data(struct feat_fd *ff, FILE *fp)
>>   		    clockid_name(clockid));
>>   }
>>   
>> +static void print_hybrid_topology(struct feat_fd *ff, FILE *fp)
>> +{
>> +	int i;
>> +	struct hybrid_node *n;
>> +
>> +	for (i = 0; i < ff->ph->env.nr_hybrid_nodes; i++) {
>> +		n = &ff->ph->env.hybrid_nodes[i];
>> +
>> +		fprintf(fp, "# %s cpu list : ", n->pmu_name);
>> +		cpu_map__fprintf(n->map, fp);
> 
> do you plan to do anything else with n->map in the future?
> because right now you could just print the stored cpus string no?
> it should be already in the cpumask shape
> 
> jirka
> 

Yes, you are right, we don't need to use n->map at least now.

Following code should be much simpler.

+struct hybrid_node {
+       char    *pmu_name;
+       char    *cpus;
+};

+static int process_hybrid_topology(struct feat_fd *ff,
+                                  void *data __maybe_unused)
+{
+       struct hybrid_node *nodes, *n;
+       u32 nr, i;
+
+       /* nr nodes */
+       if (do_read_u32(ff, &nr))
+               return -1;
+
+       nodes = zalloc(sizeof(*nodes) * nr);
+       if (!nodes)
+               return -ENOMEM;
+
+       for (i = 0; i < nr; i++) {
+               n = &nodes[i];
+
+               n->pmu_name = do_read_string(ff);
+               if (!n->pmu_name)
+                       goto error;
+
+               n->cpus = do_read_string(ff);
+               if (!n->cpus)
+                       goto error;
+       }
+
+       ff->ph->env.nr_hybrid_nodes = nr;
+       ff->ph->env.hybrid_nodes = nodes;
+       return 0;
+
+error:
...

+static void print_hybrid_topology(struct feat_fd *ff, FILE *fp)
+{
+       int i;
+       struct hybrid_node *n;
+
+       for (i = 0; i < ff->ph->env.nr_hybrid_nodes; i++) {
+               n = &ff->ph->env.hybrid_nodes[i];
+               fprintf(fp, "# %s cpu list : %s\n", n->pmu_name, n->cpus);
+       }
+}

Thanks
Jin Yao