Messages in this thread | | | Subject | Re: [PATCH V2 1/9] perf pmu: Add support for PMU capabilities | From | "Liang, Kan" <> | Date | Tue, 10 Mar 2020 12:54:05 -0400 |
| |
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 >>>> >>> >
| |