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

[Masami Hiramatsu <mhiramat@kernel.org>]

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>