linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: linux-kernel@vger.kernel.org, Namhyung Kim <namhyung@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Hemant Kumar <hemant@linux.vnet.ibm.com>,
	Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>,
	Brendan Gregg <brendan.d.gregg@gmail.com>
Subject: Re: [PATCH perf/core v10 06/23] perf probe-file: Introduce perf_cache interfaces
Date: Thu, 9 Jun 2016 11:16:31 -0300	[thread overview]
Message-ID: <20160609141631.GN11589@kernel.org> (raw)
In-Reply-To: <20160608092959.3116.63627.stgit@devbox>

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@kernel.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)

?

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.

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.

> +	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.

> +	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.

> +	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);
}

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.

>	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.

> };

> +	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?

> +
> +	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?

> +	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?

> +
> +	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.

> +	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?

> +
> +	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

  reply	other threads:[~2016-06-09 14:16 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-08  9:29 [PATCH perf/core v10 00/23] perf-probe --cache and SDT support Masami Hiramatsu
2016-06-08  9:29 ` [PATCH perf/core v10 01/23] perf: util: Fix rm_rf() to handle non-regular files correctly Masami Hiramatsu
2016-06-16  8:31   ` [tip:perf/core] perf tools: " tip-bot for Masami Hiramatsu
2016-06-08  9:29 ` [PATCH perf/core v10 02/23] perf-probe: Fix to add NULL check for strndup Masami Hiramatsu
2016-06-16  8:32   ` [tip:perf/core] perf probe: " tip-bot for Masami Hiramatsu
2016-06-08  9:29 ` [PATCH perf/core v10 03/23] perf-buildid: Rename and export build_id_cache__cachedir() Masami Hiramatsu
2016-06-16  8:32   ` [tip:perf/core] perf buildid: " tip-bot for Masami Hiramatsu
2016-06-08  9:29 ` [PATCH perf/core v10 04/23] perf probe: Add perf_probe_event__copy() Masami Hiramatsu
2016-06-16  8:33   ` [tip:perf/core] " tip-bot for Masami Hiramatsu
2016-06-08  9:29 ` [PATCH perf/core v10 05/23] perf probe: Recover and export synthesize_perf_probe_point() Masami Hiramatsu
2016-06-16  8:33   ` [tip:perf/core] perf probe: Uncomment " tip-bot for Masami Hiramatsu
2016-06-08  9:29 ` [PATCH perf/core v10 06/23] perf probe-file: Introduce perf_cache interfaces Masami Hiramatsu
2016-06-09 14:16   ` Arnaldo Carvalho de Melo [this message]
2016-06-10 22:18     ` Masami Hiramatsu
2016-06-08  9:30 ` [PATCH perf/core v10 07/23] perf probe: Add --cache option to cache the probe definitions Masami Hiramatsu
2016-06-09 14:18   ` Arnaldo Carvalho de Melo
2016-06-10 23:32     ` Masami Hiramatsu
2016-06-08  9:30 ` [PATCH perf/core v10 08/23] perf probe: Use cache entry if possible Masami Hiramatsu
2016-06-08  9:30 ` [PATCH perf/core v10 09/23] perf probe: Show all cached probes Masami Hiramatsu
2016-06-09 14:22   ` Arnaldo Carvalho de Melo
2016-06-11  0:28     ` Masami Hiramatsu
2016-06-12  3:20       ` Masami Hiramatsu
2016-06-08  9:30 ` [PATCH perf/core v10 10/23] perf probe: Remove caches when --cache is given Masami Hiramatsu
2016-06-09 14:28   ` Arnaldo Carvalho de Melo
2016-06-11  1:17     ` Masami Hiramatsu
2016-06-08  9:30 ` [PATCH perf/core v10 11/23] perf/sdt: ELF support for SDT Masami Hiramatsu
2016-06-08  9:31 ` [PATCH perf/core v10 12/23] perf probe: Add group name support Masami Hiramatsu
2016-06-08  9:31 ` [PATCH perf/core v10 13/23] perf buildid-cache: Scan and import user SDT events to probe cache Masami Hiramatsu
2016-06-08  9:31 ` [PATCH perf/core v10 14/23] perf probe: Accept %sdt and %cached event name Masami Hiramatsu
2016-06-08  9:31 ` [PATCH perf/core v10 15/23] perf-list: Show SDT and pre-cached events Masami Hiramatsu
2016-06-08  9:31 ` [PATCH perf/core v10 16/23] perf-list: Skip SDTs placed in invalid binaries Masami Hiramatsu
2016-06-08  9:31 ` [PATCH perf/core v10 17/23] perf: probe-cache: Add for_each_probe_cache_entry() wrapper Masami Hiramatsu
2016-06-08  9:31 ` [PATCH perf/core v10 18/23] perf probe: Allow wildcard for cached events Masami Hiramatsu
2016-06-08  9:32 ` [PATCH perf/core v10 19/23] perf probe: Search SDT/cached event from all probe caches Masami Hiramatsu
2016-06-08  9:32 ` [PATCH perf/core v10 20/23] perf probe: Support @BUILDID or @FILE suffix for SDT events Masami Hiramatsu
2016-06-08  9:32 ` [PATCH perf/core v10 21/23] perf probe: Support a special SDT probe format Masami Hiramatsu
2016-06-08  9:32 ` [PATCH perf/core v10 22/23] perf build: Add sdt feature detection Masami Hiramatsu
2016-06-08  9:32 ` [PATCH perf/core v10 23/23] perf-test: Add a test case for SDT event Masami Hiramatsu
2016-06-08 11:13 ` [PATCH perf/core v10 00/23] perf-probe --cache and SDT support Masami Hiramatsu
2016-06-08 11:15 ` [PATCH perf/core v10 22/23] perf build: Add sdt feature detection Masami Hiramatsu
2016-06-08 11:16 ` [PATCH perf/core v10 23/23] perf-test: Add a test case for SDT event Masami Hiramatsu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160609141631.GN11589@kernel.org \
    --to=acme@kernel.org \
    --cc=ananth@linux.vnet.ibm.com \
    --cc=brendan.d.gregg@gmail.com \
    --cc=hemant@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhiramat@kernel.org \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --subject='Re: [PATCH perf/core v10 06/23] perf probe-file: Introduce perf_cache interfaces' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox