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

[Jin, Yao]

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