linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Masami Hiramatsu <mhiramat@kernel.org>
To: Arnaldo Carvalho de Melo <acme@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: Sat, 11 Jun 2016 07:18:07 +0900	[thread overview]
Message-ID: <20160611071807.977794966f259430a4aab7a5@kernel.org> (raw)
In-Reply-To: <20160609141631.GN11589@kernel.org>

On Thu, 9 Jun 2016 11:16:31 -0300
Arnaldo Carvalho de Melo <acme@kernel.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@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)
> 
> ?

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

  reply	other threads:[~2016-06-10 22:18 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
2016-06-10 22:18     ` Masami Hiramatsu [this message]
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=20160611071807.977794966f259430a4aab7a5@kernel.org \
    --to=mhiramat@kernel.org \
    --cc=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=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