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

["Liang, Kan" <kan.liang@linux.intel.com>]

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@linux.intel.com 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
>>>>
>>>
>