Re: [PATCH V2 1/9] perf pmu: Add support for PMU capabilities

[Liang, Kan]

On 3/10/2020 10:04 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 10, 2020 at 09:53:24AM -0400, Liang, Kan escreveu:
> > On 3/10/2020 9:06 AM, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Mar 09, 2020 at 10:46:31AM -0700, kan.liang@xxxxxxxxxxxxxxx escreveu:
> > > > +static int perf_pmu__new_caps(struct list_head *list, char *name, char *value)
> > > > +{
> > > > +	struct perf_pmu_caps *caps;
> > > > +
> > > > +	caps = zalloc(sizeof(*caps));
> > > > +	if (!caps)
> > > > +		return -ENOMEM;
>
> > > So here you check if zalloc fails and returns a proper error
>
> > > > +	caps->name = strdup(name);
> > > > +	caps->value = strndup(value, strlen(value) - 1);
>
> > > But then you don't check strdup()?
>   
> > Right, I should check strdup(), otherwise the capability information may be
> > incomplete. I will fix it in V3.
>
> Thanks, overall just consider making the patches smaller if possible,
> with prep patches paving the way for more complex changes so that
> reviewing becomes easier, for instance:
>
>    perf machine: Refine the function for LBR call stack reconstruction
>
> Seems to do too many things at once. It was unfortunate, for instance,
> that the pre-existing code had that
>
> resolve_lbr_callchain_sample()
> {
> 	/* LBR only affects the user callchain */
> 	if (i != chain_nr) {
> 		body of the function, long
> 		....
> 		return err;
> 	}
>
> 	return 0;
> }
>
> One of the things you did in this patch was to the more sensible:
>
> 	/* LBR only affects the user callchain */
> 	if (i == chain_nr)
> 		return 0;
>
> 	body of the function
> 	...
> 	return err;
>
> So if you had a prep patch at this point just removing that silly
> indent, then we would see that that is just removing the indent, the
> next patch wouldn't have that check for user callchains, would be
> smaller, I think that would help reduce the patch sizes.
>
> Then if you just moved to a separate function the (callchain_param.order
> == ORDER_CALLEE) part, the patch would again be smaller, etc.
>
> This helps reviewing and usually helps us later, with bisection, when
> some bug is introduced,


Sure, I will go through all patches and see what I can do to reduce the 
size of patches in V3.


Thanks,
Kan

> Regards,
>
> - Arnaldo
>
> > Thanks,
> > Kan
> >
> > > > +	list_add_tail(&caps->list, list);
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Reading/parsing the given pmu capabilities, which should be located at:
> > > > + * /sys/bus/event_source/devices/<dev>/caps as sysfs group attributes.
> > > > + * Return the number of capabilities
> > > > + */
> > > > +int perf_pmu__caps_parse(struct perf_pmu *pmu)
> > > > +{
> > > > +	struct stat st;
> > > > +	char caps_path[PATH_MAX];
> > > > +	const char *sysfs = sysfs__mountpoint();
> > > > +	DIR *caps_dir;
> > > > +	struct dirent *evt_ent;
> > > > +	int nr_caps = 0;
> > > > +
> > > > +	if (!sysfs)
> > > > +		return -1;
> > > > +
> > > > +	snprintf(caps_path, PATH_MAX,
> > > > +		 "%s" EVENT_SOURCE_DEVICE_PATH "%s/caps", sysfs, pmu->name);
> > > > +
> > > > +	if (stat(caps_path, &st) < 0)
> > > > +		return 0;	/* no error if caps does not exist */
> > > > +
> > > > +	caps_dir = opendir(caps_path);
> > > > +	if (!caps_dir)
> > > > +		return -EINVAL;
> > > > +
> > > > +	while ((evt_ent = readdir(caps_dir)) != NULL) {
> > > > +		char path[PATH_MAX + NAME_MAX + 1];
> > > > +		char *name = evt_ent->d_name;
> > > > +		char value[128];
> > > > +		FILE *file;
> > > > +
> > > > +		if (!strcmp(name, ".") || !strcmp(name, ".."))
> > > > +			continue;
> > > > +
> > > > +		snprintf(path, sizeof(path), "%s/%s", caps_path, name);
> > > > +
> > > > +		file = fopen(path, "r");
> > > > +		if (!file)
> > > > +			break;
> > > > +
> > > > +		if (!fgets(value, sizeof(value), file) ||
> > > > +		    (perf_pmu__new_caps(&pmu->caps, name, value) < 0)) {
> > > > +			fclose(file);
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		nr_caps++;
> > > > +		fclose(file);
> > > > +	}
> > > > +
> > > > +	closedir(caps_dir);
> > > > +
> > > > +	return nr_caps;
> > > > +}
> > > > +
> > > > +struct perf_pmu_caps *perf_pmu__scan_caps(struct perf_pmu *pmu,
> > > > +					  struct perf_pmu_caps *caps)
> > > > +{
> > > > +	if (!pmu)
> > > > +		return NULL;
> > > > +
> > > > +	if (!caps)
> > > > +		caps = list_prepare_entry(caps, &pmu->caps, list);
> > > > +
> > > > +	list_for_each_entry_continue(caps, &pmu->caps, list)
> > > > +		return caps;
> > > > +
> > > > +	return NULL;
> > > > +}
> > > > diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> > > > index 6737e3d5d568..a228e27ae462 100644
> > > > --- a/tools/perf/util/pmu.h
> > > > +++ b/tools/perf/util/pmu.h
> > > > @@ -21,6 +21,12 @@ enum {
> > > >    struct perf_event_attr;
> > > > +struct perf_pmu_caps {
> > > > +	char *name;
> > > > +	char *value;
> > > > +	struct list_head list;
> > > > +};
> > > > +
> > > >    struct perf_pmu {
> > > >    	char *name;
> > > >    	__u32 type;
> > > > @@ -32,6 +38,7 @@ struct perf_pmu {
> > > >    	struct perf_cpu_map *cpus;
> > > >    	struct list_head format;  /* HEAD struct perf_pmu_format -> list */
> > > >    	struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
> > > > +	struct list_head caps;    /* HEAD struct perf_pmu_caps -> list */
> > > >    	struct list_head list;    /* ELEM */
> > > >    };
> > > > @@ -102,4 +109,9 @@ struct pmu_events_map *perf_pmu__find_map(struct perf_pmu *pmu);
> > > >    int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
> > > > +int perf_pmu__caps_parse(struct perf_pmu *pmu);
> > > > +
> > > > +struct perf_pmu_caps *perf_pmu__scan_caps(struct perf_pmu *pmu,
> > > > +					  struct perf_pmu_caps *caps);
> > > > +
> > > >    #endif /* __PMU_H */
> > > > --
> > > > 2.17.1