Message-Id: <20160611071807.977794966f259430a4aab7a5@kernel.org>
Date:	Sat, 11 Jun 2016 07:18:07 +0900
From:	Masami Hiramatsu <mhiramat@...nel.org>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:	linux-kernel@...r.kernel.org, Namhyung Kim <namhyung@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Hemant Kumar <hemant@...ux.vnet.ibm.com>,
	Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>,
	Brendan Gregg <brendan.d.gregg@...il.com>
Subject: Re: [PATCH perf/core v10 06/23] perf probe-file: Introduce
 perf_cache interfaces

On Thu, 9 Jun 2016 11:16:31 -0300
Arnaldo Carvalho de Melo <acme@...nel.org> wrote:

> Em Wed, Jun 08, 2016 at 06:29:59PM +0900, Masami Hiramatsu escreveu:
> > Introduce perf_cache object and interfaces to create,
> > add entry, commit, and delete the object.
> > perf_cache represents a file for the cached perf-probe
> > definitions on one binary file or vmlinux which has its
> > own build id. The probe cache file is located under the
> > build-id cache directory of the target binary, as below;
> > 
> >  <perf-debug-dir>/.build-id/<BU>/<ILDID>/probe
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
> > ---
> >  Changes in v10:
> >   - Splited from "Add --cache option to cache the probe definitions"
> > ---
> >  tools/perf/util/probe-file.c |  307 ++++++++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/probe-file.h |   19 +++
> >  2 files changed, 326 insertions(+)
> > 
> > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > index 3fe6214..689d874 100644
> > --- a/tools/perf/util/probe-file.c
> > +++ b/tools/perf/util/probe-file.c
> > @@ -14,6 +14,7 @@
> >   * GNU General Public License for more details.
> >   *
> >   */
> > +#include <sys/uio.h>
> >  #include "util.h"
> >  #include "event.h"
> >  #include "strlist.h"
> > @@ -324,3 +325,309 @@ int probe_file__del_events(int fd, struct strfilter *filter)
> >  
> >  	return ret;
> >  }
> > +
> > +static void probe_cache_entry__delete(struct probe_cache_entry *node)
> > +{
> > +	if (!list_empty(&node->list))
> > +		list_del(&node->list);
> 
> Humm, shouldn't this be something like:
> 
> BUG_ON(!list_empty(&node->list)
> 
> ?

Would you mean assert? (it seems similart to die()...)

> 
> I.e. whoever inserted this on a list should take care of removing it
> before deleting it, taking locks, whatever is needed to keep the
> integrity of such list.

Ah, I see.

> 
> You may not be using this stuff in a multithreaded app now, but lets not
> make it difficult to :-)
> 
> I noticed why you do it this way and have a suggestion to use a more
> usual model, see below.
> 
> > +	if (node->tevlist)
> > +		strlist__delete(node->tevlist);
> 
> No checking, destructors generally follows the free() model, i.e. they
> eat NULL for breakfast. Lemme see if strlist does that... Yes, they do.

For the strlist, I should clear the pointer. OK.

> 
> > +	clear_perf_probe_event(&node->pev);
> > +	free(node->spev);
> 
> Here you may want to use:
> 
> 	zfree(&node->spev);
> 
> To free and set node->spev to NULL, to help in debugging when this node
> may be still referenced even having being deleted.

OK, I'll clear that.

> 
> > +	free(node);
> > +}
> > +
> > +static struct probe_cache_entry *
> > +probe_cache_entry__new(struct perf_probe_event *pev)
> > +{
> > +	struct probe_cache_entry *ret = zalloc(sizeof(*ret));
> > +
> > +	if (ret) {
> > +		INIT_LIST_HEAD(&ret->list);
> > +		ret->tevlist = strlist__new(NULL, NULL);
> > +		if (!ret->tevlist)
> > +			zfree(&ret);
> > +		if (ret && pev) {
> > +			ret->spev = synthesize_perf_probe_command(pev);
> > +			if (!ret->spev ||
> > +			    perf_probe_event__copy(&ret->pev, pev) < 0) {
> > +				probe_cache_entry__delete(ret);
> > +				return NULL;
> > +			}
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/* 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 = DSO__NAME_KALLSYMS;
> > +		ret = sysfs__sprintf_build_id("/", sbuildid);
> > +	}
> > +	if (ret < 0) {
> > +		pr_debug("Failed to get build-id from %s.\n", target);
> > +		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__cachedir(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, 0644);
> > +	if (fd < 0)
> > +		pr_debug("Failed to open cache(%d): %s\n", fd, cpath);
> > +	free(dir_name);
> > +	pcache->fd = fd;
> > +
> > +	return fd;
> > +}
> > +
> > +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");
> 
> fdopen may return NULL, a check is needed.

Oops, right. I'll add a check.

> 
> > +	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);
> > +			if (!entry) {
> > +				ret = -ENOMEM;
> > +				goto out;
> > +			}
> > +			entry->spev = strdup(buf + 1);
> > +			if (entry->spev)
> > +				ret = parse_perf_probe_command(buf + 1,
> > +								&entry->pev);
> > +			else
> > +				ret = -ENOMEM;
> > +			if (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;
> > +}
> > +
> > +static struct probe_cache *probe_cache__alloc(void)
> > +{
> > +	struct probe_cache *ret = zalloc(sizeof(*ret));
> > +
> > +	if (ret) {
> > +		INIT_LIST_HEAD(&ret->list);
> > +		ret->fd = -EINVAL;
> > +	}
> > +	return ret;
> > +}
> > +
> > +void probe_cache__delete(struct probe_cache *pcache)
> > +{
> > +	struct probe_cache_entry *entry;
> > +
> > +	if (!pcache)
> > +		return;
> 
> see, a good destructor, accepts NULL, does nothing with it.
> 
> > +
> > +	while (!list_empty(&pcache->list)) {
> > +		entry = list_first_entry(&pcache->list, typeof(*entry), list);
> > +		probe_cache_entry__delete(entry);
> > +	}
> 
> the above while is the definition of a "purge()" operation, that may be
> useful outside of a delete operation, please consider adding it, like:
> 
> void probe_cache__purge(struct probe_cache *pcache)
> {
> 	struct probe_cache_entry *entry, *n;
> 	list_for_each_entry_safe(entry, n, &pcache->list, node)
> 		probe_cache_entry__delete(entry);
> }

Hm, I see.

> 
> And please use 'list' for lists and 'node' for entries in a list, i.e.
> you have, at the end of this patch:
> 
> > struct probe_cache_entry {
> >	struct list_head	list;
> 
> This one should be 'node', not list, this way we know that this is an
> entry in a list, not a list of some other structs.

OK.

> 
> >	struct perf_probe_event pev;
> >	char			*spev;
> >	struct strlist		*tevlist;
> > };
> 
> > struct probe_cache {
> > 	int	fd;
> >	struct list_head list;
> 
> This one is ok, but then if you rename it from the generic name 'list'
> to something more informative, for instance 'cache_entries', I think it
> will help people reading your code to grasp what it is doing more
> quickly.

OK.

> 
> > };
> 
> > +	if (pcache->fd > 0)
> > +		close(pcache->fd);
> > +	free(pcache);
> > +}
> > +
> > +struct probe_cache *probe_cache__new(const char *target)
> > +{
> > +	struct probe_cache *pcache = probe_cache__alloc();
> > +	int ret;
> 
> This is odd, what you call "probe_cache__alloc() looks like what a
> "new()" does, if you think that the constructor for 'probe_cache' has to
> always open and load it, then why not just do the zalloc() here and then
> call probe_cache__init() on it?

I just wanted to keep probe_cache always initialized.
But yeah, add probe_cache__init() is OK for me too.

> > +
> > +	if (!pcache)
> > +		return NULL;
> > +
> > +	ret = probe_cache__open(pcache, target);
> > +	if (ret < 0) {
> > +		pr_debug("Cache open error: %d\n", ret);
> > +		goto out_err;
> > +	}
> > +
> > +	ret = probe_cache__load(pcache);
> > +	if (ret < 0) {
> > +		pr_debug("Cache read error: %d\n", ret);
> > +		goto out_err;
> > +	}
> > +
> > +	return pcache;
> > +
> > +out_err:
> > +	probe_cache__delete(pcache);
> > +	return NULL;
> > +}
> > +
> > +static bool streql(const char *a, const char *b)
> > +{
> > +	if (a == b)
> > +		return true;
> > +
> > +	if (!a || !b)
> > +		return false;
> > +
> > +	return !strcmp(a, b);
> > +}
> > +
> > +static struct probe_cache_entry *
> > +probe_cache__find(struct probe_cache *pcache, struct perf_probe_event *pev)
> > +{
> > +	struct probe_cache_entry *entry = NULL;
> > +	char *cmd = NULL;
> > +
> > +	cmd = synthesize_perf_probe_command(pev);
> 
> Why init it to NULL only to immediately init it again to something else?
> Perhaps:
> 
> 	char *cmd = synthesize_perf_probe_command(pev);
> 
> instead?

OK, I'll do.

> 
> > +	if (!cmd)
> > +		return NULL;
> > +
> > +	list_for_each_entry(entry, &pcache->list, list) {
> > +		/* Hit if same event name or same command-string */
> > +		if ((pev->event &&
> > +		     (streql(entry->pev.group, pev->group) &&
> > +		      streql(entry->pev.event, pev->event))) ||
> > +		    (!strcmp(entry->spev, cmd)))
> > +			goto found;
> > +	}
> > +	entry = NULL;
> > +
> > +found:
> > +	free(cmd);
> > +	return entry;
> > +}
> > +
> > +int probe_cache__add_entry(struct probe_cache *pcache,
> > +			   struct perf_probe_event *pev,
> > +			   struct probe_trace_event *tevs, int ntevs)
> > +{
> > +	struct probe_cache_entry *entry = NULL;
> > +	char *command;
> > +	int i, ret = 0;
> > +
> > +	if (!pcache || !pev || !tevs || ntevs <= 0) {
> > +		ret = -EINVAL;
> > +		goto out_err;
> > +	}
> > +
> > +	/* Remove old cache entry */
> > +	entry = probe_cache__find(pcache, pev);
> > +	if (entry)
> > +		probe_cache_entry__delete(entry);
> 
> Here you could be more compact with:
> 
> 	probe_cache_entry__delete(probe_cache__find(pcache, pev));
> 
> Because delete() functions accept NULL?

Ah, right! maybe I forgot that :(

> 
> > +
> > +	ret = -ENOMEM;
> > +	entry = probe_cache_entry__new(pev);
> > +	if (!entry)
> > +		goto out_err;
> > +
> > +	for (i = 0; i < ntevs; i++) {
> > +		if (!tevs[i].point.symbol)
> > +			continue;
> > +
> > +		command = synthesize_probe_trace_command(&tevs[i]);
> > +		if (!command)
> > +			goto out_err;
> > +		strlist__add(entry->tevlist, command);
> > +		free(command);
> > +	}
> > +	list_add_tail(&entry->list, &pcache->list);
> > +	pr_debug("Added probe cache: %d\n", ntevs);
> > +	return 0;
> > +
> > +out_err:
> > +	pr_debug("Failed to add probe caches\n");
> > +	if (entry)
> > +		probe_cache_entry__delete(entry);
> 
> No need to check for NULL, call the destructor directly.

OK.

> 
> > +	return ret;
> > +}
> > +
> > +static int probe_cache_entry__write(struct probe_cache_entry *entry, int fd)
> > +{
> > +	struct str_node *snode;
> > +	struct iovec iov[3];
> > +	int ret;
> > +
> > +	pr_debug("Writing cache: #%s\n", entry->spev);
> > +	iov[0].iov_base = (void *)"#"; iov[0].iov_len = 1;
> > +	iov[1].iov_base = entry->spev; iov[1].iov_len = strlen(entry->spev);
> > +	iov[2].iov_base = (void *)"\n"; iov[2].iov_len = 1;
> > +	ret = writev(fd, iov, 3);
> > +	if (ret < 0)
> > +		return ret;
> 
> Shouldn't we check short writes? writev() returns the number of bytes
> written, isn't it possible to return less than what you asked for?

Yeah, I found this in write(2):

       If a write() is interrupted by a signal handler before  any  bytes  are
       written, then the call fails with the error EINTR; if it is interrupted
       after at least one byte  has  been  written,  the  call  succeeds,  and
       returns the number of bytes written.

Since writev can also be interrupted intermediate of writes,
it can return a smaller number. Let me add a check.

Thanks!!

> 
> > +
> > +	strlist__for_each(snode, entry->tevlist) {
> > +		iov[0].iov_base = (void *)snode->s;
> > +		iov[0].iov_len = strlen(snode->s);
> > +		iov[1].iov_base = (void *)"\n"; iov[1].iov_len = 1;
> > +		ret = writev(fd, iov, 2);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +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;
> > +
> > +	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_head	list;
> > +	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


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists