Re: [PATCH RFC v3 02/12] perf jevents: Add support for system events tables

[John Garry]

On 11/05/2020 12:01, Jiri Olsa wrote:
> On Thu, May 07, 2020 at 07:57:41PM +0800, John Garry wrote:
>
> SNIP
>
> > +				      &sys_event_tables);
> > +		}
> > +
> >   		print_events_table_prefix(eventsfp, tblname);
> >   		return 0;
> >   	}
> > @@ -1180,7 +1253,6 @@ int main(int argc, char *argv[])
> >   	} else if (rc < 0) {
> >   		/* Make build fail */
> >   		fclose(eventsfp);
> > -		free_arch_std_events();
> >   		ret = 1;
> >   		goto out_free_mapfile;
> >   	} else if (rc) {
> > @@ -1206,27 +1278,31 @@ int main(int argc, char *argv[])
> >   	if (close_table)
> >   		print_events_table_suffix(eventsfp);
> >   
> > -	if (!mapfile) {
> > -		pr_info("%s: No CPU->JSON mapping?\n", prog);
> > -		goto empty_map;
> > +	if (mapfile) {
> > +		if (process_mapfile(eventsfp, mapfile)) {
> > +			pr_err("%s: Error processing mapfile %s\n", prog,
> > +			       mapfile);
> > +			/* Make build fail */
> > +			fclose(eventsfp);
> > +			ret = 1;
> > +		}
> > +	} else {
> > +		pr_err("%s: No CPU->JSON mapping?\n", prog);
>
> shouldn't we jump to empty_map in here? there still needs to be a
> mapfile, right?

In theory we could only support sys events :)

But I'll now make this a (empty map) failure case. And I think that 
another error case handling needs fixing in my patch.


As for this:

 +	fprintf(outfp, "struct pmu_sys_events pmu_sys_event_tables[] = {");
>> +
>> +	list_for_each_entry(sys_event_table, &sys_event_tables, list) {
>> +		fprintf(outfp, "\n\t{\n\t\t.table = %s,\n\t},",
>> +			sys_event_table->name);
>> +	}
>> +	fprintf(outfp, "\n\t{\n\t\t.table = 0\n\t},");
>
> this will add extra tabs:
>
>          {
>                  .table = 0
>          },
>
> while the rest of the file starts items without any indent
>

I'll ensure the indent is the same.

BTW, is there anything to be said for removing the empty map feature 
(and always breaking the perf build instead)? I guess that it was just 
an early feature for dealing with unstable JSONs.

Thanks,
john

> jirka
>
> >   	}
> >   
> > -	if (process_mapfile(eventsfp, mapfile)) {
> > -		pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
> > -		/* Make build fail */
> > +	if (process_system_event_tables(eventsfp)) {
> >   		fclose(eventsfp);
> > -		free_arch_std_events();
> >   		ret = 1;
> >   	}
> >   
> > -
> >   	goto out_free_mapfile;
> >   
> >   empty_map:
> >   	fclose(eventsfp);
> >   	create_empty_mapping(output_file);
> > -	free_arch_std_events();
> >   out_free_mapfile:
> > +	free_arch_std_events();
> > +	free_sys_event_tables();
> >   	free(mapfile);
> >   	return ret;
> >   }
>
> SNIP
>
> .