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
