Re: [PATCH perf/core v5 04/15] perf probe: Add --cache option to cache the probe definitions
On Thu, Apr 28, 2016 at 03:37:42AM +0900, Masami Hiramatsu wrote: > From: Masami Hiramatsu > > Add --cache option to cache the probe definitions. This > just saves the result of the dwarf analysis to probe cache. > > Signed-off-by: Masami Hiramatsu > Signed-off-by: Masami Hiramatsu > --- > Changes in v5: > - Move probe_cache* definitions. (code cleanup) > > Changes in v4: > - Remove cache saving failure message. > --- [snip] > +static int probe_cache__load(struct probe_cache *pcache) > +{ > + struct probe_cache_entry *entry = NULL; > + char buf[MAX_CMDLEN], *p; > + int ret = 0; > + FILE *fp; > + > + fp = fdopen(dup(pcache->fd), "r"); > + while (!feof(fp)) { > + if (!fgets(buf, MAX_CMDLEN, fp)) > + break; > + p = strchr(buf, '\n'); > + if (p) > + *p = '\0'; > + if (buf[0] == '#') {/* #perf_probe_event */ > + entry = probe_cache_entry__new(NULL); The probe_cache_entry__new() can fail. Thanks, Namhyung > + entry->spev = strdup(buf + 1); > + ret = parse_perf_probe_command(buf + 1, &entry->pev); > + if (!entry->spev || ret < 0) { > + probe_cache_entry__delete(entry); > + goto out; > + } > + list_add_tail(&entry->list, &pcache->list); > + } else {/* trace_probe_event */ > + if (!entry) { > + ret = -EINVAL; > + goto out; > + } > + strlist__add(entry->tevlist, buf); > + } > + } > +out: > + fclose(fp); > + return ret; > +}
Re: [PATCH perf/core v5 04/15] perf probe: Add --cache option to cache the probe definitions
On Thu, Apr 28, 2016 at 03:37:42AM +0900, Masami Hiramatsu wrote: > From: Masami Hiramatsu > > Add --cache option to cache the probe definitions. This > just saves the result of the dwarf analysis to probe cache. > > Signed-off-by: Masami Hiramatsu > Signed-off-by: Masami Hiramatsu > --- > Changes in v5: > - Move probe_cache* definitions. (code cleanup) > > Changes in v4: > - Remove cache saving failure message. > --- [SNIP] > +/* For the kernel probe caches, pass target = NULL */ > +static int probe_cache__open(struct probe_cache *pcache, const char *target) > +{ > + char cpath[PATH_MAX]; > + char sbuildid[SBUILD_ID_SIZE]; > + char *dir_name; > + bool is_kallsyms = !target; > + int ret, fd; > + > + if (target) > + ret = filename__sprintf_build_id(target, sbuildid); > + else { > + target = "[kernel.kallsyms]"; > + ret = sysfs__sprintf_build_id("/", sbuildid); > + } > + if (ret < 0) { > + pr_debug("Failed to get build-id from %s.\n", target ?: > "kernel"); > + return ret; > + } > + > + /* If we have no buildid cache, make it */ > + if (!build_id_cache__cached(sbuildid)) { > + ret = build_id_cache__add_s(sbuildid, target, > + is_kallsyms, NULL); > + if (ret < 0) { > + pr_debug("Failed to add build-id cache: %s\n", target); > + return ret; > + } > + } > + > + dir_name = build_id_cache__dirname_from_path(sbuildid, target, > + is_kallsyms, false); > + if (!dir_name) > + return -ENOMEM; > + > + snprintf(cpath, PATH_MAX, "%s/probes", dir_name); > + fd = open(cpath, O_CREAT | O_RDWR | O_APPEND, 0644); > + if (fd < 0) > + pr_debug("Failed to open cache(%d): %s\n", fd, cpath); > + free(dir_name); > + pcache->fd = fd; > + > + return fd; > +} [SNIP] > + > +int probe_cache__commit(struct probe_cache *pcache) > +{ > + struct probe_cache_entry *entry; > + int ret = 0; > + > + /* TBD: if we do not update existing entries, skip it */ > + ret = lseek(pcache->fd, 0, SEEK_SET); > + if (ret < 0) > + goto out; > + > + ret = ftruncate(pcache->fd, 0); > + if (ret < 0) > + goto out; What is this doing? Does it truncate old contents on write? Opening with O_APPEND looks strange to me.. Thanks, Namhyung > + > + list_for_each_entry(entry, &pcache->list, list) { > + ret = probe_cache_entry__write(entry, pcache->fd); > + pr_debug("Cache committed: %d\n", ret); > + if (ret < 0) > + break; > + } > +out: > + return ret; > +} > diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h > index 18ac9cf..d2b8791d 100644 > --- a/tools/perf/util/probe-file.h > +++ b/tools/perf/util/probe-file.h > @@ -5,6 +5,19 @@ > #include "strfilter.h" > #include "probe-event.h" > > +/* Cache of probe definitions */ > +struct probe_cache_entry { > + struct list_headlist; > + struct perf_probe_event pev; > + char*spev; > + struct strlist *tevlist; > +}; > + > +struct probe_cache { > + int fd; > + struct list_head list; > +}; > + > #define PF_FL_UPROBE 1 > #define PF_FL_RW 2 > > @@ -18,5 +31,11 @@ int probe_file__get_events(int fd, struct strfilter > *filter, > struct strlist *plist); > int probe_file__del_strlist(int fd, struct strlist *namelist); > > +struct probe_cache *probe_cache__new(const char *target); > +int probe_cache__add_entry(struct probe_cache *pcache, > +struct perf_probe_event *pev, > +struct probe_trace_event *tevs, int ntevs); > +int probe_cache__commit(struct probe_cache *pcache); > +void probe_cache__delete(struct probe_cache *pcache); > > #endif >
Re: [PATCH perf/core v5 04/15] perf probe: Add --cache option to cache the probe definitions
On Thu, 28 Apr 2016 11:32:18 +0900 Namhyung Kim wrote: > On Thu, Apr 28, 2016 at 03:37:42AM +0900, Masami Hiramatsu wrote: > > From: Masami Hiramatsu > > > > Add --cache option to cache the probe definitions. This > > just saves the result of the dwarf analysis to probe cache. > > > > Signed-off-by: Masami Hiramatsu > > Signed-off-by: Masami Hiramatsu > > --- > > Changes in v5: > > - Move probe_cache* definitions. (code cleanup) > > > > Changes in v4: > > - Remove cache saving failure message. > > --- > > [SNIP] > > +/* For the kernel probe caches, pass target = NULL */ > > +static int probe_cache__open(struct probe_cache *pcache, const char > > *target) > > +{ > > + char cpath[PATH_MAX]; > > + char sbuildid[SBUILD_ID_SIZE]; > > + char *dir_name; > > + bool is_kallsyms = !target; > > + int ret, fd; > > + > > + if (target) > > + ret = filename__sprintf_build_id(target, sbuildid); > > + else { > > + target = "[kernel.kallsyms]"; > > + ret = sysfs__sprintf_build_id("/", sbuildid); > > + } > > + if (ret < 0) { > > + pr_debug("Failed to get build-id from %s.\n", target ?: > > "kernel"); > > + return ret; > > + } > > + > > + /* If we have no buildid cache, make it */ > > + if (!build_id_cache__cached(sbuildid)) { > > + ret = build_id_cache__add_s(sbuildid, target, > > + is_kallsyms, NULL); > > + if (ret < 0) { > > + pr_debug("Failed to add build-id cache: %s\n", target); > > + return ret; > > + } > > + } > > + > > + dir_name = build_id_cache__dirname_from_path(sbuildid, target, > > +is_kallsyms, false); > > + if (!dir_name) > > + return -ENOMEM; > > + > > + snprintf(cpath, PATH_MAX, "%s/probes", dir_name); > > + fd = open(cpath, O_CREAT | O_RDWR | O_APPEND, 0644); > > + if (fd < 0) > > + pr_debug("Failed to open cache(%d): %s\n", fd, cpath); > > + free(dir_name); > > + pcache->fd = fd; > > + > > + return fd; > > +} > > [SNIP] > > + > > +int probe_cache__commit(struct probe_cache *pcache) > > +{ > > + struct probe_cache_entry *entry; > > + int ret = 0; > > + > > + /* TBD: if we do not update existing entries, skip it */ > > + ret = lseek(pcache->fd, 0, SEEK_SET); > > + if (ret < 0) > > + goto out; > > + > > + ret = ftruncate(pcache->fd, 0); > > + if (ret < 0) > > + goto out; > > What is this doing? Does it truncate old contents on write? Opening > with O_APPEND looks strange to me.. Ah, right. I forget the reason why :(, but I guess it maybe because just changing the design while coding. I'll remove O_APPEND flag then. Thank you! -- Masami Hiramatsu
Re: [PATCH perf/core v5 04/15] perf probe: Add --cache option to cache the probe definitions
On Thu, 28 Apr 2016 11:12:19 +0900 Namhyung Kim wrote: > On Thu, Apr 28, 2016 at 03:37:42AM +0900, Masami Hiramatsu wrote: > > From: Masami Hiramatsu > > > > Add --cache option to cache the probe definitions. This > > just saves the result of the dwarf analysis to probe cache. > > > > Signed-off-by: Masami Hiramatsu > > Signed-off-by: Masami Hiramatsu > > --- > > Changes in v5: > > - Move probe_cache* definitions. (code cleanup) > > > > Changes in v4: > > - Remove cache saving failure message. > > --- > > [snip] > > +static int probe_cache__load(struct probe_cache *pcache) > > +{ > > + struct probe_cache_entry *entry = NULL; > > + char buf[MAX_CMDLEN], *p; > > + int ret = 0; > > + FILE *fp; > > + > > + fp = fdopen(dup(pcache->fd), "r"); > > + while (!feof(fp)) { > > + if (!fgets(buf, MAX_CMDLEN, fp)) > > + break; > > + p = strchr(buf, '\n'); > > + if (p) > > + *p = '\0'; > > + if (buf[0] == '#') {/* #perf_probe_event */ > > + entry = probe_cache_entry__new(NULL); > > The probe_cache_entry__new() can fail. Good catch! > > Thanks, > Namhyung > > > > + entry->spev = strdup(buf + 1); Moreover, if this strdup fail, this goes to return but retvalue is 0... I'll fix this too. Thank you! -- Masami Hiramatsu