Re: [PATCH perf/core v5 04/15] perf probe: Add --cache option to cache the probe definitions

Masami Hiramatsu
2016-04-28T23:06:24Z

On Thu, 28 Apr 2016 11:32:18 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> On Thu, Apr 28, 2016 at 03:37:42AM +0900, Masami Hiramatsu wrote:
> > From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >
> > 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 <masami.hiramatsu.pt@hitachi.com>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> > 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 <mhiramat@kernel.org>