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

[Arnaldo Carvalho de Melo]

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,

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
> > > 
> > 

-- 

- Arnaldo