dle-develop — Dependable Linux Effort mailing list for developers

Re: [Dle-develop] [PATCH -mm v6] tracepoint: Add signal coredump tracepoint

[Roland M. <ro...@re...>]

> I think retval decoding will help us to find which condition caused
> failing the coredump, by reading the source code.
> So, I'd like to leave it.

Having a proper -ERR* code for the cases that have one in your patch is
certainly good.  What I meant was using the 0/1 values to distinguish
success vs failure from the binfmt dumper.  If there were separate
tracepoints for success vs failure, then the failure one should certainly
get an error code, which would be 0 when the error (or refusal to dump) was
due to some decision made by the binfmt code rather than a write error.

> Hmm, indeed. it seems that those tracepoints are useful for finding
> unexpected delays from coredump...
> OK, I'll try to add those tracepoints. Would you have any recommended data
> which those tracepoints should record?

Whatever is handy, I suppose.  i.e. of the things you pass into the
tracepoint now, give each tracepoint the subset that makes sense for its
case.  For the tracepoint after synchronization and before dumping, I think
it should be more or less right after format_corename() and it can pass the
ispipe, corename, cprm.limit and binfmt->min_coredump values that affect
the tests immediately thereafter (as well as the full cprm and binfmt
pointers).


Thanks,
Roland

Re: [Dle-develop] [PATCH -tip 1/4] [BUGFIX] perf tools: Initialize dso->node member in dso__new

[Arnaldo C. de M. <ac...@re...>]

Em Wed, Apr 21, 2010 at 03:56:16PM -0400, Masami Hiramatsu escreveu:
> If dso->node member is not initilized, it causes a segmentation
> fault when adding to other lists. It should be initilized in
> dso__new().
> 
> Signed-off-by: Masami Hiramatsu <mhi...@re...>
> Cc: Arnaldo Carvalho de Melo <ac...@re...>
> Cc: Peter Zijlstra <pe...@in...>
> Cc: Paul Mackerras <pa...@sa...>
> Cc: Mike Galbraith <ef...@gm...>
> Cc: Frederic Weisbecker <fwe...@gm...>
> Cc: Ingo Molnar <mi...@el...>
> ---
> 
>  tools/perf/util/symbol.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index e782e7d..e77c33a 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -189,6 +189,7 @@ struct dso *dso__new(const char *name)
>  		self->sorted_by_name = 0;
>  		self->has_build_id = 0;
>  		self->kernel = DSO_TYPE_USER;
> +		INIT_LIST_HEAD(&self->node);
>  	}
>  
>  	return self;

Trying to understand how this would be a problem, as:

static inline void __list_add(struct list_head *new,
                              struct list_head *prev,
                              struct list_head *next)
{
        next->prev = new;
        new->next = next;
        new->prev = prev;
        prev->next = new;
}

This is not a list head, just a node.

Looking at other messages where you described the problem to try to
understand why this would help.

- Arnaldo

Re: [Dle-develop] [PATCH -tip 2/4] perf probe: Fix to use symtab only if no debuginfo

[Arnaldo C. de M. <ac...@re...>]

Em Wed, Apr 21, 2010 at 03:56:24PM -0400, Masami Hiramatsu escreveu:
> Fix perf probe to use symtab only if there is no debuginfo,
> because debuginfo has more precise information than symtab.

"more precise"? Wouldn't it be "more expressive", i.e. with more
information (parameter list with types, return type, location of
parameters, etc), because what is common to both have the same value,
i.e. the function address and size.

> If we can't find a function in debuginfo, we never find it
> in symtab.
> 
> Signed-off-by: Masami Hiramatsu <mhi...@re...>
> Reported-by: Arnaldo Carvalho de Melo <ac...@re...>
> Cc: Paul Mackerras <pa...@sa...>
> Cc: Peter Zijlstra <pe...@in...>
> Cc: Mike Galbraith <ef...@gm...>
> Cc: Frederic Weisbecker <fwe...@gm...>
> Cc: Ingo Molnar <mi...@el...>
> ---
> 
>  tools/perf/util/probe-event.c |   17 +++++++++--------
>  1 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 4fb4803..5d3baec 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -180,15 +180,16 @@ static int try_to_find_kprobe_trace_events(struct perf_probe_event *pev,
>  		return -ENOENT;
>  	}
>  	/* Error path : ntevs < 0 */
> -	if (need_dwarf) {
> -		if (ntevs == -EBADF)
> -			pr_warning("No dwarf info found in the vmlinux - "
> -				"please rebuild with CONFIG_DEBUG_INFO=y.\n");
> -		return ntevs;
> +	pr_debug("An error occurred in debuginfo analysis (%d).\n", ntevs);
> +	if (ntevs == -EBADF) {
> +		pr_warning("Warning: No dwarf info found in the vmlinux - "
> +			"please rebuild kernel with CONFIG_DEBUG_INFO=y.\n");
> +		if (!need_dwarf) {
> +			pr_debug("Trying to use symbols.\nn");
> +			return 0;
> +		}
>  	}
> -	pr_debug("An error occurred in debuginfo analysis."
> -		 " Try to use symbols.\n");
> -	return 0;
> +	return ntevs;
>  }
>  
>  #define LINEBUF_SIZE 256
> 
> 
> -- 
> Masami Hiramatsu
> e-mail: mhi...@re...

Re: [Dle-develop] [PATCH -mm v6] tracepoint: Add signal coredump tracepoint

[Roland M. <ro...@re...>]

> > The "retval" encoding seems a bit arcane.  I wonder if it might be better
> > to just have separate tracepoints for successful and failed dump attempts.
> > Note that whether or not the dump succeeded is already available in
> > (task->signal->group_exit_code & 0x80) as seen at exit or death tracing
> > events.
> 
> OK, please read the previous discussion and tell me what you think about...

I don't have a strong opinion about this particular aspect.

> > The purposes you mention seem to be served well enough by this tracepoint.
> > But I recall having the impression that one of the original motivating
> > interests for tracing core-dump details was to understand when a giant core
> > dump was responsible for huge amounts of i/o and/or memory thrashing.
> > (Once you notice that happening, you might adjust coredump_filter settings
> > to reduce the problem.)  Your new tracepoint doesn't help directly with
> > tracking those sorts of issues, because it only happens after all the work
> > is done.  If you are monitoring trace_signal_deliver, then you can filter
> > those for SIG_DFL cases of sig_kernel_coredump() signals and recognize that
> > as the beginning of the coredump.  Still, it might be preferable to have
> > explicit start-core-dump and end-core-dump tracepoints.
> 
> No, that's not our interests. We are just interested in recording
> coredump parameters when the coredump is done. After dumping core
> (or failing to dump), we'd like to check what was wrong (wrong filter setting?),
> or correctly dumped (and removed after).
> 
> > Furthermore, I can see potential use for tracepoints before and after
> > coredump_wait(), which synchronizes other threads before actually starting
> > to calculate and write the dump.  The window after coredump_wait() and
> > before the post-dump tracepoint is where the actual work of writing the
> > core file takes place, in case you want to monitor i/o load between those
> > marks or suchlike.
> 
> Hmm, currently, we don't mention that.

I know that's not your interest now.  But I do recall someone somewhere at
some point asking about tracepoints in the context of observing unexpected
i/o and paging loads and tracking down that they were due to core dumping.
It's also certainly the case that there have been people spending time
diagnosing long delays due to coredump_wait() logic and/or actual deadlocks
due to bugs in the exit path interactions with coredump_wait().  Having
separate before-wait, after-wait, and after-dump tracepoints could well
make that sort of diagnosis trivial in the future, especially when doing
"flight recorder" style analysis.


Thanks,
Roland

Re: [Dle-develop] [PATCH -mm v6] tracepoint: Add signal coredump tracepoint

[Masami H. <mhi...@re...>]

Roland McGrath wrote:
>>> The "retval" encoding seems a bit arcane.  I wonder if it might be better
>>> to just have separate tracepoints for successful and failed dump attempts.
>>> Note that whether or not the dump succeeded is already available in
>>> (task->signal->group_exit_code & 0x80) as seen at exit or death tracing
>>> events.
>>
>> OK, please read the previous discussion and tell me what you think about...
> 
> I don't have a strong opinion about this particular aspect.

I think retval decoding will help us to find which condition caused
failing the coredump, by reading the source code.
So, I'd like to leave it.

>>> The purposes you mention seem to be served well enough by this tracepoint.
>>> But I recall having the impression that one of the original motivating
>>> interests for tracing core-dump details was to understand when a giant core
>>> dump was responsible for huge amounts of i/o and/or memory thrashing.
>>> (Once you notice that happening, you might adjust coredump_filter settings
>>> to reduce the problem.)  Your new tracepoint doesn't help directly with
>>> tracking those sorts of issues, because it only happens after all the work
>>> is done.  If you are monitoring trace_signal_deliver, then you can filter
>>> those for SIG_DFL cases of sig_kernel_coredump() signals and recognize that
>>> as the beginning of the coredump.  Still, it might be preferable to have
>>> explicit start-core-dump and end-core-dump tracepoints.
>>
>> No, that's not our interests. We are just interested in recording
>> coredump parameters when the coredump is done. After dumping core
>> (or failing to dump), we'd like to check what was wrong (wrong filter setting?),
>> or correctly dumped (and removed after).
>>
>>> Furthermore, I can see potential use for tracepoints before and after
>>> coredump_wait(), which synchronizes other threads before actually starting
>>> to calculate and write the dump.  The window after coredump_wait() and
>>> before the post-dump tracepoint is where the actual work of writing the
>>> core file takes place, in case you want to monitor i/o load between those
>>> marks or suchlike.
>>
>> Hmm, currently, we don't mention that.
> 
> I know that's not your interest now.  But I do recall someone somewhere at
> some point asking about tracepoints in the context of observing unexpected
> i/o and paging loads and tracking down that they were due to core dumping.
> It's also certainly the case that there have been people spending time
> diagnosing long delays due to coredump_wait() logic and/or actual deadlocks
> due to bugs in the exit path interactions with coredump_wait().  Having
> separate before-wait, after-wait, and after-dump tracepoints could well
> make that sort of diagnosis trivial in the future, especially when doing
> "flight recorder" style analysis.

Hmm, indeed. it seems that those tracepoints are useful for finding
unexpected delays from coredump...
OK, I'll try to add those tracepoints. Would you have any recommended data
which those tracepoints should record?

Thank you!


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 3/4] perf probe: Fix to exit callback soon after finding too many probe points

[Masami H. <mhi...@re...>]

Fix to exit callback soon after finding too many probe points.
Don't try to continue searching because it already failed.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Reported-by: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Ingo Molnar <mi...@el...>
---

 tools/perf/util/probe-finder.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 3e79775..57b51ca 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -922,6 +922,8 @@ static int probe_point_inline_cb(Dwarf_Die *in_die, void *data)
 			 (uintmax_t)pf->addr);
 
 		param->retval = convert_probe_point(in_die, pf);
+		if (param->retval < 0)
+			return DWARF_CB_ABORT;
 	}
 
 	return DWARF_CB_OK;
@@ -1157,6 +1159,8 @@ static int line_range_funcdecl_cb(Dwarf_Die *sp_die, void *data)
 		return DWARF_CB_OK;
 
 	param->retval = line_range_add_line(src, lineno, lf->lr);
+	if (param->retval < 0)
+		return DWARF_CB_ABORT;
 	return DWARF_CB_OK;
 }
 


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 1/4] [BUGFIX] perf tools: Initialize dso->node member in dso__new

[Masami H. <mhi...@re...>]

If dso->node member is not initilized, it causes a segmentation
fault when adding to other lists. It should be initilized in
dso__new().

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Ingo Molnar <mi...@el...>
---

 tools/perf/util/symbol.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index e782e7d..e77c33a 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -189,6 +189,7 @@ struct dso *dso__new(const char *name)
 		self->sorted_by_name = 0;
 		self->has_build_id = 0;
 		self->kernel = DSO_TYPE_USER;
+		INIT_LIST_HEAD(&self->node);
 	}
 
 	return self;


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 4/4] perf probe: Add --max-probes option

[Masami H. <mhi...@re...>]

Add --max-probes option to change the maximum limit of
findable probe points per event, since inlined function can be
expanded into thousands of probe points. Default value is 128.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Suggested-by: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Ingo Molnar <mi...@el...>
---

 tools/perf/Documentation/perf-probe.txt |    3 +++
 tools/perf/builtin-probe.c              |    9 ++++++++-
 tools/perf/util/probe-event.c           |   17 ++++++++++-------
 tools/perf/util/probe-event.h           |    4 ++--
 tools/perf/util/probe-finder.c          |   11 ++++++-----
 tools/perf/util/probe-finder.h          |    6 ++++--
 6 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/tools/perf/Documentation/perf-probe.txt b/tools/perf/Documentation/perf-probe.txt
index 63c25d3..94a258c 100644
--- a/tools/perf/Documentation/perf-probe.txt
+++ b/tools/perf/Documentation/perf-probe.txt
@@ -62,6 +62,9 @@ OPTIONS
 	Dry run. With this option, --add and --del doesn't execute actual
 	adding and removal operations.
 
+--max-probes::
+	Set the maximum number of probe points for an event. Default is 128.
+
 PROBE SYNTAX
 ------------
 Probe points are defined by following syntax.
diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index c1e5403..61c6d70 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -54,6 +54,7 @@ static struct {
 	struct perf_probe_event events[MAX_PROBES];
 	struct strlist *dellist;
 	struct line_range line_range;
+	int max_probe_points;
 } params;
 
 
@@ -179,6 +180,8 @@ static const struct option options[] = {
 		   "file", "vmlinux pathname"),
 #endif
 	OPT__DRY_RUN(&probe_event_dry_run),
+	OPT_INTEGER('\0', "max-probes", &params.max_probe_points,
+		 "Set how many probe points can be found for a probe."),
 	OPT_END()
 };
 
@@ -200,6 +203,9 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
 		}
 	}
 
+	if (params.max_probe_points == 0)
+		params.max_probe_points = MAX_PROBES;
+
 	if ((!params.nevents && !params.dellist && !params.list_events &&
 	     !params.show_lines))
 		usage_with_options(probe_usage, options);
@@ -246,7 +252,8 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
 
 	if (params.nevents) {
 		ret = add_perf_probe_events(params.events, params.nevents,
-					    params.force_add);
+					    params.force_add,
+					    params.max_probe_points);
 		if (ret < 0) {
 			pr_err("  Error: Failed to add events. (%d)\n", ret);
 			return ret;
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 5d3baec..9ded38c 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -150,7 +150,8 @@ static int convert_to_perf_probe_point(struct kprobe_trace_point *tp,
 
 /* Try to find perf_probe_event with debuginfo */
 static int try_to_find_kprobe_trace_events(struct perf_probe_event *pev,
-					   struct kprobe_trace_event **tevs)
+					   struct kprobe_trace_event **tevs,
+					   int max_tevs)
 {
 	bool need_dwarf = perf_probe_event_need_dwarf(pev);
 	int fd, ntevs;
@@ -166,7 +167,7 @@ static int try_to_find_kprobe_trace_events(struct perf_probe_event *pev,
 	}
 
 	/* Searching trace events corresponding to probe event */
-	ntevs = find_kprobe_trace_events(fd, pev, tevs);
+	ntevs = find_kprobe_trace_events(fd, pev, tevs, max_tevs);
 	close(fd);
 
 	if (ntevs > 0) {	/* Succeeded to find trace events */
@@ -318,7 +319,8 @@ static int convert_to_perf_probe_point(struct kprobe_trace_point *tp,
 }
 
 static int try_to_find_kprobe_trace_events(struct perf_probe_event *pev,
-				struct kprobe_trace_event **tevs __unused)
+				struct kprobe_trace_event **tevs __unused,
+				int max_tevs __unused)
 {
 	if (perf_probe_event_need_dwarf(pev)) {
 		pr_warning("Debuginfo-analysis is not supported.\n");
@@ -1408,14 +1410,15 @@ static int __add_kprobe_trace_events(struct perf_probe_event *pev,
 }
 
 static int convert_to_kprobe_trace_events(struct perf_probe_event *pev,
-					  struct kprobe_trace_event **tevs)
+					  struct kprobe_trace_event **tevs,
+					  int max_tevs)
 {
 	struct symbol *sym;
 	int ret = 0, i;
 	struct kprobe_trace_event *tev;
 
 	/* Convert perf_probe_event with debuginfo */
-	ret = try_to_find_kprobe_trace_events(pev, tevs);
+	ret = try_to_find_kprobe_trace_events(pev, tevs, max_tevs);
 	if (ret != 0)
 		return ret;
 
@@ -1487,7 +1490,7 @@ struct __event_package {
 };
 
 int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
-			  bool force_add)
+			  bool force_add, int max_tevs)
 {
 	int i, j, ret;
 	struct __event_package *pkgs;
@@ -1506,7 +1509,7 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
 		pkgs[i].pev = &pevs[i];
 		/* Convert with or without debuginfo */
 		ret  = convert_to_kprobe_trace_events(pkgs[i].pev,
-						      &pkgs[i].tevs);
+						      &pkgs[i].tevs, max_tevs);
 		if (ret < 0)
 			goto end;
 		pkgs[i].ntevs = ret;
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index e7ff0d0..e9db1a2 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -115,8 +115,8 @@ extern void clear_kprobe_trace_event(struct kprobe_trace_event *tev);
 extern int parse_line_range_desc(const char *cmd, struct line_range *lr);
 
 
-extern int add_perf_probe_events(struct perf_probe_event *pevs, int ntevs,
-				 bool force_add);
+extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
+				 bool force_add, int max_probe_points);
 extern int del_perf_probe_events(struct strlist *dellist);
 extern int show_perf_probe_events(void);
 extern int show_line_range(struct line_range *lr);
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 57b51ca..e3e3c1e 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -677,8 +677,9 @@ static int convert_probe_point(Dwarf_Die *sp_die, struct probe_finder *pf)
 	Dwarf_Attribute fb_attr;
 	size_t nops;
 
-	if (pf->ntevs == MAX_PROBES) {
-		pr_warning("Too many( > %d) probe point found.\n", MAX_PROBES);
+	if (pf->ntevs == pf->max_tevs) {
+		pr_warning("Too many( > %d) probe point found.\n",
+			   pf->max_tevs);
 		return -ERANGE;
 	}
 	tev = &pf->tevs[pf->ntevs++];
@@ -983,9 +984,9 @@ static int find_probe_point_by_func(struct probe_finder *pf)
 
 /* Find kprobe_trace_events specified by perf_probe_event from debuginfo */
 int find_kprobe_trace_events(int fd, struct perf_probe_event *pev,
-			     struct kprobe_trace_event **tevs)
+			     struct kprobe_trace_event **tevs, int max_tevs)
 {
-	struct probe_finder pf = {.pev = pev};
+	struct probe_finder pf = {.pev = pev, .max_tevs = max_tevs};
 	struct perf_probe_point *pp = &pev->point;
 	Dwarf_Off off, noff;
 	size_t cuhl;
@@ -993,7 +994,7 @@ int find_kprobe_trace_events(int fd, struct perf_probe_event *pev,
 	Dwarf *dbg;
 	int ret = 0;
 
-	pf.tevs = zalloc(sizeof(struct kprobe_trace_event) * MAX_PROBES);
+	pf.tevs = zalloc(sizeof(struct kprobe_trace_event) * max_tevs);
 	if (pf.tevs == NULL)
 		return -ENOMEM;
 	*tevs = pf.tevs;
diff --git a/tools/perf/util/probe-finder.h b/tools/perf/util/probe-finder.h
index 310ce89..66f1980 100644
--- a/tools/perf/util/probe-finder.h
+++ b/tools/perf/util/probe-finder.h
@@ -18,7 +18,8 @@ static inline int is_c_varname(const char *name)
 #ifdef DWARF_SUPPORT
 /* Find kprobe_trace_events specified by perf_probe_event from debuginfo */
 extern int find_kprobe_trace_events(int fd, struct perf_probe_event *pev,
-				    struct kprobe_trace_event **tevs);
+				    struct kprobe_trace_event **tevs,
+				    int max_tevs);
 
 /* Find a perf_probe_point from debuginfo */
 extern int find_perf_probe_point(int fd, unsigned long addr,
@@ -32,7 +33,8 @@ extern int find_line_range(int fd, struct line_range *lr);
 struct probe_finder {
 	struct perf_probe_event	*pev;		/* Target probe event */
 	struct kprobe_trace_event *tevs;	/* Result trace events */
-	int			ntevs;		/* number of trace events */
+	int			ntevs;		/* Number of trace events */
+	int			max_tevs;	/* Max number of trace events */
 
 	/* For function searching */
 	int			lno;		/* Line number */


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 2/4] perf probe: Fix to use symtab only if no debuginfo

[Masami H. <mhi...@re...>]

Fix perf probe to use symtab only if there is no debuginfo,
because debuginfo has more precise information than symtab.
If we can't find a function in debuginfo, we never find it
in symtab.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Reported-by: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Ingo Molnar <mi...@el...>
---

 tools/perf/util/probe-event.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 4fb4803..5d3baec 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -180,15 +180,16 @@ static int try_to_find_kprobe_trace_events(struct perf_probe_event *pev,
 		return -ENOENT;
 	}
 	/* Error path : ntevs < 0 */
-	if (need_dwarf) {
-		if (ntevs == -EBADF)
-			pr_warning("No dwarf info found in the vmlinux - "
-				"please rebuild with CONFIG_DEBUG_INFO=y.\n");
-		return ntevs;
+	pr_debug("An error occurred in debuginfo analysis (%d).\n", ntevs);
+	if (ntevs == -EBADF) {
+		pr_warning("Warning: No dwarf info found in the vmlinux - "
+			"please rebuild kernel with CONFIG_DEBUG_INFO=y.\n");
+		if (!need_dwarf) {
+			pr_debug("Trying to use symbols.\nn");
+			return 0;
+		}
 	}
-	pr_debug("An error occurred in debuginfo analysis."
-		 " Try to use symbols.\n");
-	return 0;
+	return ntevs;
 }
 
 #define LINEBUF_SIZE 256


-- 
Masami Hiramatsu
e-mail: mhi...@re...

Re: [Dle-develop] [PATCH -mm v6] tracepoint: Add signal coredump tracepoint

[Masami H. <mhi...@re...>]

Hi Roland,

Roland McGrath wrote:
>> Add signal coredump tracepoint which shows signal number,
>> mm->flags, core file size limitation, the result of
>> coredump, and core file name.
> 
> The "retval" encoding seems a bit arcane.  I wonder if it might be better
> to just have separate tracepoints for successful and failed dump attempts.
> Note that whether or not the dump succeeded is already available in
> (task->signal->group_exit_code & 0x80) as seen at exit or death tracing
> events.

OK, please read the previous discussion and tell me what you think about...
https://patchwork.kernel.org/patch/64345/
---
> > What do you think this tracepoint's use case?
> 
> Frankly to say, our first attempt was tracing mm->flags because
> it can be changed by users without asking, and they sometimes
> ask why core is not perfect or why core file is so big.
> 
> Perhaps, covering all of those failure cases and succeed cases,
> gives better information for them. In that case, we might better
> tweak execution(goto) path to leave some error code on retval.

This is enough acceptable to me.
---

> 
>> This tracepoint requirement comes mainly from the viewpoint of
>> administrators. [...]
> 
> The purposes you mention seem to be served well enough by this tracepoint.
> But I recall having the impression that one of the original motivating
> interests for tracing core-dump details was to understand when a giant core
> dump was responsible for huge amounts of i/o and/or memory thrashing.
> (Once you notice that happening, you might adjust coredump_filter settings
> to reduce the problem.)  Your new tracepoint doesn't help directly with
> tracking those sorts of issues, because it only happens after all the work
> is done.  If you are monitoring trace_signal_deliver, then you can filter
> those for SIG_DFL cases of sig_kernel_coredump() signals and recognize that
> as the beginning of the coredump.  Still, it might be preferable to have
> explicit start-core-dump and end-core-dump tracepoints.

No, that's not our interests. We are just interested in recording
coredump parameters when the coredump is done. After dumping core
(or failing to dump), we'd like to check what was wrong (wrong filter setting?),
or correctly dumped (and removed after).

> Furthermore, I can see potential use for tracepoints before and after
> coredump_wait(), which synchronizes other threads before actually starting
> to calculate and write the dump.  The window after coredump_wait() and
> before the post-dump tracepoint is where the actual work of writing the
> core file takes place, in case you want to monitor i/o load between those
> marks or suchlike.

Hmm, currently, we don't mention that.

> 
>> -	char corename[CORENAME_MAX_SIZE + 1];
>> +	char corename[CORENAME_MAX_SIZE + 1] = "";
> 
> This initialization of a stack array means the same as:
> 
> 	memset(corename, '\0', sizeof corename);
> 
> So the compiler generates that because those are the semantics.
> All you need is:
> 
> 	corename[0] = '\0';
> 
> since this is a string.

hm, ok. that's nice to fix.

Thank you,

> 
> 
> Thanks,
> Roland

-- 
Masami Hiramatsu
e-mail: mhi...@re...

Re: [Dle-develop] [PATCH -mm v6] tracepoint: Add signal coredump tracepoint

[Roland M. <ro...@re...>]

> Add signal coredump tracepoint which shows signal number,
> mm->flags, core file size limitation, the result of
> coredump, and core file name.

The "retval" encoding seems a bit arcane.  I wonder if it might be better
to just have separate tracepoints for successful and failed dump attempts.
Note that whether or not the dump succeeded is already available in
(task->signal->group_exit_code & 0x80) as seen at exit or death tracing
events.

> This tracepoint requirement comes mainly from the viewpoint of
> administrators. [...]

The purposes you mention seem to be served well enough by this tracepoint.
But I recall having the impression that one of the original motivating
interests for tracing core-dump details was to understand when a giant core
dump was responsible for huge amounts of i/o and/or memory thrashing.
(Once you notice that happening, you might adjust coredump_filter settings
to reduce the problem.)  Your new tracepoint doesn't help directly with
tracking those sorts of issues, because it only happens after all the work
is done.  If you are monitoring trace_signal_deliver, then you can filter
those for SIG_DFL cases of sig_kernel_coredump() signals and recognize that
as the beginning of the coredump.  Still, it might be preferable to have
explicit start-core-dump and end-core-dump tracepoints.

Furthermore, I can see potential use for tracepoints before and after
coredump_wait(), which synchronizes other threads before actually starting
to calculate and write the dump.  The window after coredump_wait() and
before the post-dump tracepoint is where the actual work of writing the
core file takes place, in case you want to monitor i/o load between those
marks or suchlike.

> -	char corename[CORENAME_MAX_SIZE + 1];
> +	char corename[CORENAME_MAX_SIZE + 1] = "";

This initialization of a stack array means the same as:

	memset(corename, '\0', sizeof corename);

So the compiler generates that because those are the semantics.
All you need is:

	corename[0] = '\0';

since this is a string.


Thanks,
Roland

Re: [Dle-develop] [PATCH -tip 0/6] perf-probe bugfixes

[Arnaldo C. de M. <ac...@re...>]

Em Wed, Apr 14, 2010 at 06:39:21PM -0400, Masami Hiramatsu escreveu:
> Hi Arnaldo,
> 
> (Sorry, I've fixed to signed-off-by and send it again)
> Could you pull these patches into your tree too?
> These fixes several bugs on perf probe.
> 
> Thank you,

Sure, I just applied your previous series and will now review this new
one and merge it after that.

[Dle-develop] [PATCH -tip 6/6] perf probe: Show function entry line as probe-able

[Masami H. <mhi...@re...>]

Function entry line should be shown as probe-able line,
because each function has declared line attribute.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Ingo Molnar <mi...@el...>
---

 tools/perf/util/probe-finder.c |   74 ++++++++++++++++++++++++++++++++++------
 1 files changed, 63 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 4f83121..aff8a04 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1127,6 +1127,45 @@ end:
 	return ret;
 }
 
+/* Add a line and store the src path */
+static int line_range_add_line(const char *src, unsigned int lineno,
+			       struct line_range *lr)
+{
+	/* Copy real path */
+	if (!lr->path) {
+		lr->path = strdup(src);
+		if (lr->path == NULL)
+			return -ENOMEM;
+	}
+	return line_list__add_line(&lr->line_list, lineno);
+}
+
+/* Search function declaration lines */
+static int line_range_funcdecl_cb(Dwarf_Die *sp_die, void *data)
+{
+	struct dwarf_callback_param *param = data;
+	struct line_finder *lf = param->data;
+	const char *src;
+	int lineno;
+
+	src = dwarf_decl_file(sp_die);
+	if (src && strtailcmp(src, lf->fname) != 0)
+		return DWARF_CB_OK;
+
+	if (dwarf_decl_line(sp_die, &lineno) != 0 ||
+	    (lf->lno_s > lineno || lf->lno_e < lineno))
+		return DWARF_CB_OK;
+
+	param->retval = line_range_add_line(src, lineno, lf->lr);
+	return DWARF_CB_OK;
+}
+
+static int find_line_range_func_decl_lines(struct line_finder *lf)
+{
+	struct dwarf_callback_param param = {.data = (void *)lf, .retval = 0};
+	dwarf_getfuncs(&lf->cu_die, line_range_funcdecl_cb, &param, 0);
+	return param.retval;
+}
 
 /* Find line range from its line number */
 static int find_line_range_by_line(Dwarf_Die *sp_die, struct line_finder *lf)
@@ -1135,7 +1174,7 @@ static int find_line_range_by_line(Dwarf_Die *sp_die, struct line_finder *lf)
 	Dwarf_Line *line;
 	size_t nlines, i;
 	Dwarf_Addr addr;
-	int lineno;
+	int lineno, ret = 0;
 	const char *src;
 	Dwarf_Die die_mem;
 
@@ -1145,6 +1184,7 @@ static int find_line_range_by_line(Dwarf_Die *sp_die, struct line_finder *lf)
 		return -ENOENT;
 	}
 
+	/* Search probable lines on lines list */
 	for (i = 0; i < nlines; i++) {
 		line = dwarf_onesrcline(lines, i);
 		if (dwarf_lineno(line, &lineno) != 0 ||
@@ -1167,22 +1207,34 @@ static int find_line_range_by_line(Dwarf_Die *sp_die, struct line_finder *lf)
 		if (strtailcmp(src, lf->fname) != 0)
 			continue;
 
-		/* Copy real path */
-		if (!lf->lr->path) {
-			lf->lr->path = strdup(src);
-			if (lf->lr->path == NULL)
-				return -ENOMEM;
-		}
-		line_list__add_line(&lf->lr->line_list, lineno);
+		ret = line_range_add_line(src, lineno, lf->lr);
+		if (ret < 0)
+			return ret;
 	}
+
+	/*
+	 * Dwarf lines doesn't include function declarations. We have to
+	 * check functions list or given function.
+	 */
+	if (sp_die) {
+		src = dwarf_decl_file(sp_die);
+		if (src && dwarf_decl_line(sp_die, &lineno) == 0 &&
+		    (lf->lno_s <= lineno && lf->lno_e >= lineno))
+			ret = line_range_add_line(src, lineno, lf->lr);
+	} else
+		ret = find_line_range_func_decl_lines(lf);
+
 	/* Update status */
-	if (!list_empty(&lf->lr->line_list))
-		lf->found = 1;
+	if (ret >= 0)
+		if (!list_empty(&lf->lr->line_list))
+			ret = lf->found = 1;
+		else
+			ret = 0;	/* Lines are not found */
 	else {
 		free(lf->lr->path);
 		lf->lr->path = NULL;
 	}
-	return lf->found;
+	return ret;
 }
 
 static int line_range_inline_cb(Dwarf_Die *in_die, void *data)


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 5/6] perf probe: Support DW_OP_plus_uconst in DW_AT_data_member_location

[Masami H. <mhi...@re...>]

DW_OP_plus_uconst can be used for DW_AT_data_member_location.
This patch adds DW_OP_plus_uconst support when getting
structure member offset.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Ingo Molnar <mi...@el...>
---

 tools/perf/util/probe-finder.c |   37 ++++++++++++++++++++++++++++++++-----
 1 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index b4c9365..4f83121 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -249,6 +249,33 @@ static int die_get_byte_size(Dwarf_Die *tp_die)
 	return (int)ret;
 }
 
+/* Get data_member_location offset */
+static int die_get_data_member_location(Dwarf_Die *mb_die, Dwarf_Word *offs)
+{
+	Dwarf_Attribute attr;
+	Dwarf_Op *expr;
+	size_t nexpr;
+	int ret;
+
+	if (dwarf_attr(mb_die, DW_AT_data_member_location, &attr) == NULL)
+		return -ENOENT;
+
+	if (dwarf_formudata(&attr, offs) != 0) {
+		/* DW_AT_data_member_location should be DW_OP_plus_uconst */
+		ret = dwarf_getlocation(&attr, &expr, &nexpr);
+		if (ret < 0 || nexpr == 0)
+			return -ENOENT;
+
+		if (expr[0].atom != DW_OP_plus_uconst || nexpr != 1) {
+			pr_debug("Unable to get offset:Unexpected OP %x (%d)\n",
+				 expr[0].atom, nexpr);
+			return -ENOTSUP;
+		}
+		*offs = (Dwarf_Word)expr[0].number;
+	}
+	return 0;
+}
+
 /* Return values for die_find callbacks */
 enum {
 	DIE_FIND_CB_FOUND = 0,		/* End of Search */
@@ -482,9 +509,9 @@ static int convert_variable_fields(Dwarf_Die *vr_die, const char *varname,
 				    Dwarf_Die *die_mem)
 {
 	struct kprobe_trace_arg_ref *ref = *ref_ptr;
-	Dwarf_Attribute attr;
 	Dwarf_Die type;
 	Dwarf_Word offs;
+	int ret;
 
 	pr_debug("converting %s in %s\n", field->name, varname);
 	if (die_get_real_type(vr_die, &type) == NULL) {
@@ -542,17 +569,17 @@ static int convert_variable_fields(Dwarf_Die *vr_die, const char *varname,
 	}
 
 	/* Get the offset of the field */
-	if (dwarf_attr(die_mem, DW_AT_data_member_location, &attr) == NULL ||
-	    dwarf_formudata(&attr, &offs) != 0) {
+	ret = die_get_data_member_location(die_mem, &offs);
+	if (ret < 0) {
 		pr_warning("Failed to get the offset of %s.\n", field->name);
-		return -ENOENT;
+		return ret;
 	}
 	ref->offset += (long)offs;
 
 	/* Converting next field */
 	if (field->next)
 		return convert_variable_fields(die_mem, field->name,
-					       field->next, &ref, die_mem);
+					field->next, &ref, die_mem);
 	else
 		return 0;
 }


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 4/6] perf probe: Fix line range to show end line

[Masami H. <mhi...@re...>]

Line range should reject the range if the number of lines is 0
(e.g. "sched.c:1024+0"), and it should show the lines include
the end of line number (e.g. "sched.c:1024-2048" should show
2048th line).

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Ingo Molnar <mi...@el...>
---

 tools/perf/util/probe-event.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 954ca21..5bf8ab0 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -290,7 +290,7 @@ int show_line_range(struct line_range *lr)
 
 	if (lr->end == INT_MAX)
 		lr->end = l + NR_ADDITIONAL_LINES;
-	while (l < lr->end && !feof(fp) && ret >= 0)
+	while (l <= lr->end && !feof(fp) && ret >= 0)
 		ret = show_one_line(fp, (l++) - lr->offset, false, false);
 end:
 	fclose(fp);
@@ -341,9 +341,15 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
 	ptr = strchr(arg, ':');
 	if (ptr) {
 		lr->start = (int)strtoul(ptr + 1, &tmp, 0);
-		if (*tmp == '+')
+		if (*tmp == '+') {
 			lr->end = lr->start + (int)strtoul(tmp + 1, &tmp, 0);
-		else if (*tmp == '-')
+			lr->end--;	/*
+					 * Adjust the number of lines here.
+					 * If the number of lines == 1, the
+					 * the end of line should be equal to
+					 * the start of line.
+					 */
+		} else if (*tmp == '-')
 			lr->end = (int)strtoul(tmp + 1, &tmp, 0);
 		else
 			lr->end = INT_MAX;


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 3/6] perf probe: Fix a bug that --line range can be overflow

[Masami H. <mhi...@re...>]

Since line_finder.lno_s/e are signed int but line_range.start/end
are unsigned int, it is possible to be overflow when converting
line_range->start/end to line_finder->lno_s/e.
This changes line_range.start/end and line_list.line to signed int
and adds overflow checks when setting line_finder.lno_s/e.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Ingo Molnar <mi...@el...>
---

 tools/perf/util/probe-event.c  |   23 ++++++++++++-----------
 tools/perf/util/probe-event.h  |    6 +++---
 tools/perf/util/probe-finder.c |   19 +++++++++----------
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 6d43839..954ca21 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -189,7 +189,7 @@ static int try_to_find_kprobe_trace_events(struct perf_probe_event *pev,
 #define LINEBUF_SIZE 256
 #define NR_ADDITIONAL_LINES 2
 
-static int show_one_line(FILE *fp, unsigned int l, bool skip, bool show_num)
+static int show_one_line(FILE *fp, int l, bool skip, bool show_num)
 {
 	char buf[LINEBUF_SIZE];
 	const char *color = PERF_COLOR_BLUE;
@@ -198,7 +198,7 @@ static int show_one_line(FILE *fp, unsigned int l, bool skip, bool show_num)
 		goto error;
 	if (!skip) {
 		if (show_num)
-			fprintf(stdout, "%7u  %s", l, buf);
+			fprintf(stdout, "%7d  %s", l, buf);
 		else
 			color_fprintf(stdout, color, "         %s", buf);
 	}
@@ -231,7 +231,7 @@ error:
  */
 int show_line_range(struct line_range *lr)
 {
-	unsigned int l = 1;
+	int l = 1;
 	struct line_node *ln;
 	FILE *fp;
 	int fd, ret;
@@ -340,16 +340,15 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
 	 */
 	ptr = strchr(arg, ':');
 	if (ptr) {
-		lr->start = (unsigned int)strtoul(ptr + 1, &tmp, 0);
+		lr->start = (int)strtoul(ptr + 1, &tmp, 0);
 		if (*tmp == '+')
-			lr->end = lr->start + (unsigned int)strtoul(tmp + 1,
-								    &tmp, 0);
+			lr->end = lr->start + (int)strtoul(tmp + 1, &tmp, 0);
 		else if (*tmp == '-')
-			lr->end = (unsigned int)strtoul(tmp + 1, &tmp, 0);
+			lr->end = (int)strtoul(tmp + 1, &tmp, 0);
 		else
-			lr->end = 0;
-		pr_debug("Line range is %u to %u\n", lr->start, lr->end);
-		if (lr->end && lr->start > lr->end) {
+			lr->end = INT_MAX;
+		pr_debug("Line range is %d to %d\n", lr->start, lr->end);
+		if (lr->start > lr->end) {
 			semantic_error("Start line must be smaller"
 				       " than end line.\n");
 			return -EINVAL;
@@ -360,8 +359,10 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
 			return -EINVAL;
 		}
 		tmp = strndup(arg, (ptr - arg));
-	} else
+	} else {
 		tmp = strdup(arg);
+		lr->end = INT_MAX;
+	}
 
 	if (tmp == NULL)
 		return -ENOMEM;
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index aee60e1..f0831bc 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -74,15 +74,15 @@ struct perf_probe_event {
 /* Line number container */
 struct line_node {
 	struct list_head	list;
-	unsigned int		line;
+	int			line;
 };
 
 /* Line range */
 struct line_range {
 	char			*file;		/* File name */
 	char			*function;	/* Function name */
-	unsigned int		start;		/* Start line number */
-	unsigned int		end;		/* End line number */
+	int			start;		/* Start line number */
+	int			end;		/* End line number */
 	int			offset;		/* Start line offset */
 	char			*path;		/* Real path name */
 	struct list_head	line_list;	/* Visible lines */
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index e443e69..b4c9365 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -111,7 +111,7 @@ static int strtailcmp(const char *s1, const char *s2)
 /* Line number list operations */
 
 /* Add a line to line number list */
-static int line_list__add_line(struct list_head *head, unsigned int line)
+static int line_list__add_line(struct list_head *head, int line)
 {
 	struct line_node *ln;
 	struct list_head *p;
@@ -138,7 +138,7 @@ found:
 }
 
 /* Check if the line in line number list */
-static int line_list__has_line(struct list_head *head, unsigned int line)
+static int line_list__has_line(struct list_head *head, int line)
 {
 	struct line_node *ln;
 
@@ -1146,7 +1146,7 @@ static int find_line_range_by_line(Dwarf_Die *sp_die, struct line_finder *lf)
 			if (lf->lr->path == NULL)
 				return -ENOMEM;
 		}
-		line_list__add_line(&lf->lr->line_list, (unsigned int)lineno);
+		line_list__add_line(&lf->lr->line_list, lineno);
 	}
 	/* Update status */
 	if (!list_empty(&lf->lr->line_list))
@@ -1179,10 +1179,12 @@ static int line_range_search_cb(Dwarf_Die *sp_die, void *data)
 		dwarf_decl_line(sp_die, &lr->offset);
 		pr_debug("fname: %s, lineno:%d\n", lf->fname, lr->offset);
 		lf->lno_s = lr->offset + lr->start;
-		if (!lr->end)
+		if (lf->lno_s < 0)	/* Overflow */
+			lf->lno_s = INT_MAX;
+		lf->lno_e = lr->offset + lr->end;
+		if (lf->lno_e < 0)	/* Overflow */
 			lf->lno_e = INT_MAX;
-		else
-			lf->lno_e = lr->offset + lr->end;
+		pr_debug("New line range: %d to %d\n", lf->lno_s, lf->lno_e);
 		lr->start = lf->lno_s;
 		lr->end = lf->lno_e;
 		if (dwarf_func_inline(sp_die)) {
@@ -1244,10 +1246,7 @@ int find_line_range(int fd, struct line_range *lr)
 				ret = find_line_range_by_func(&lf);
 			else {
 				lf.lno_s = lr->start;
-				if (!lr->end)
-					lf.lno_e = INT_MAX;
-				else
-					lf.lno_e = lr->end;
+				lf.lno_e = lr->end;
 				ret = find_line_range_by_line(NULL, &lf);
 			}
 		}


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 2/6] perf probe: Fix mis-estimation for shortening filename

[Masami H. <mhi...@re...>]

Fix mis-estimation size for making a short filename.
Since the buffer size is 32 bytes and there are '@' prefix and
'\0' termination, maximum shorten filename length should be
30. This means, before searching '/', it should be 31 bytes.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Ingo Molnar <mi...@el...>
---

 tools/perf/util/probe-event.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 1c4a20a..6d43839 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -806,12 +806,12 @@ static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
 			goto error;
 	}
 	if (pp->file) {
-		len = strlen(pp->file) - 32;
+		len = strlen(pp->file) - 31;
 		if (len < 0)
 			len = 0;
 		tmp = strchr(pp->file + len, '/');
 		if (!tmp)
-			tmp = pp->file + len - 1;
+			tmp = pp->file + len;
 		ret = e_snprintf(file, 32, "@%s", tmp + 1);
 		if (ret <= 0)
 			goto error;


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 1/6] perf probe: Fix to use correct debugfs path finder

[Masami H. <mhi...@re...>]

Instead of using debugfs_path, use debugfs_find_mountpoint()
to find actual debugfs path.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Reported-by: Ingo Molnar <mi...@el...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
---

 tools/perf/builtin-probe.c    |    4 ----
 tools/perf/util/probe-event.c |   12 ++++++++++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 5259c5a..4b6dd84 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -40,7 +40,6 @@
 #include "util/debug.h"
 #include "util/debugfs.h"
 #include "util/parse-options.h"
-#include "util/parse-events.h"	/* For debugfs_path */
 #include "util/probe-finder.h"
 #include "util/probe-event.h"
 
@@ -205,9 +204,6 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
 	     !params.show_lines))
 		usage_with_options(probe_usage, options);
 
-	if (debugfs_valid_mountpoint(debugfs_path) < 0)
-		die("Failed to find debugfs path.");
-
 	if (params.list_events) {
 		if (params.nevents != 0 || params.dellist) {
 			pr_err("  Error: Don't use --list with --add/--del.\n");
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index ca108b2..1c4a20a 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -42,8 +42,8 @@
 #include "color.h"
 #include "symbol.h"
 #include "thread.h"
+#include "debugfs.h"
 #include "trace-event.h"	/* For __unused */
-#include "parse-events.h"	/* For debugfs_path */
 #include "probe-event.h"
 #include "probe-finder.h"
 
@@ -1075,10 +1075,18 @@ void clear_kprobe_trace_event(struct kprobe_trace_event *tev)
 static int open_kprobe_events(bool readwrite)
 {
 	char buf[PATH_MAX];
+	const char *__debugfs;
 	int ret;
 
-	ret = e_snprintf(buf, PATH_MAX, "%s/../kprobe_events", debugfs_path);
+	__debugfs = debugfs_find_mountpoint();
+	if (__debugfs == NULL) {
+		pr_warning("Debugfs is not mounted.\n");
+		return -ENOENT;
+	}
+
+	ret = e_snprintf(buf, PATH_MAX, "%stracing/kprobe_events", __debugfs);
 	if (ret >= 0) {
+		pr_debug("Opening %s write=%d\n", buf, readwrite);
 		if (readwrite && !probe_event_dry_run)
 			ret = open(buf, O_RDWR, O_APPEND);
 		else


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 0/6] perf-probe bugfixes

[Masami H. <mhi...@re...>]

Hi Arnaldo,

(Sorry, I've fixed to signed-off-by and send it again)
Could you pull these patches into your tree too?
These fixes several bugs on perf probe.

Thank you,

---

Masami Hiramatsu (6):
      perf probe: Show function entry line as probe-able
      perf probe: Support DW_OP_plus_uconst in DW_AT_data_member_location
      perf probe: Fix line range to show end line
      perf probe: Fix a bug that --line range can be overflow
      perf probe: Fix mis-estimation for shortening filename
      perf probe: Fix to use correct debugfs path finder


 tools/perf/builtin-probe.c     |    4 -
 tools/perf/util/probe-event.c  |   51 ++++++++++------
 tools/perf/util/probe-event.h  |    6 +-
 tools/perf/util/probe-finder.c |  128 ++++++++++++++++++++++++++++++++--------
 4 files changed, 139 insertions(+), 50 deletions(-)

-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 2/6] perf probe: Fix mis-estimation for shortening filename

[Masami H. <mhi...@re...>]

Fix mis-estimation size for making a short filename.
Since the buffer size is 32 bytes and there are '@' prefix and
'\0' termination, maximum shorten filename length should be
30. This means, before searching '/', it should be 31 bytes.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Ingo Molnar <mi...@el...>
---

 tools/perf/util/probe-event.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 1c4a20a..6d43839 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -806,12 +806,12 @@ static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
 			goto error;
 	}
 	if (pp->file) {
-		len = strlen(pp->file) - 32;
+		len = strlen(pp->file) - 31;
 		if (len < 0)
 			len = 0;
 		tmp = strchr(pp->file + len, '/');
 		if (!tmp)
-			tmp = pp->file + len - 1;
+			tmp = pp->file + len;
 		ret = e_snprintf(file, 32, "@%s", tmp + 1);
 		if (ret <= 0)
 			goto error;


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 1/6] perf probe: Fix to use correct debugfs path finder

[Masami H. <mhi...@re...>]

Instead of using debugfs_path, use debugfs_find_mountpoint()
to find actual debugfs path.

Signed-off-by: Masami Hiramatsu <mh...@re...>
Reported-by: Ingo Molnar <mi...@el...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Paul Mackerras <pa...@sa...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Mike Galbraith <ef...@gm...>
Cc: Frederic Weisbecker <fwe...@gm...>
---

 tools/perf/builtin-probe.c    |    4 ----
 tools/perf/util/probe-event.c |   12 ++++++++++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 5259c5a..4b6dd84 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -40,7 +40,6 @@
 #include "util/debug.h"
 #include "util/debugfs.h"
 #include "util/parse-options.h"
-#include "util/parse-events.h"	/* For debugfs_path */
 #include "util/probe-finder.h"
 #include "util/probe-event.h"
 
@@ -205,9 +204,6 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
 	     !params.show_lines))
 		usage_with_options(probe_usage, options);
 
-	if (debugfs_valid_mountpoint(debugfs_path) < 0)
-		die("Failed to find debugfs path.");
-
 	if (params.list_events) {
 		if (params.nevents != 0 || params.dellist) {
 			pr_err("  Error: Don't use --list with --add/--del.\n");
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index ca108b2..1c4a20a 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -42,8 +42,8 @@
 #include "color.h"
 #include "symbol.h"
 #include "thread.h"
+#include "debugfs.h"
 #include "trace-event.h"	/* For __unused */
-#include "parse-events.h"	/* For debugfs_path */
 #include "probe-event.h"
 #include "probe-finder.h"
 
@@ -1075,10 +1075,18 @@ void clear_kprobe_trace_event(struct kprobe_trace_event *tev)
 static int open_kprobe_events(bool readwrite)
 {
 	char buf[PATH_MAX];
+	const char *__debugfs;
 	int ret;
 
-	ret = e_snprintf(buf, PATH_MAX, "%s/../kprobe_events", debugfs_path);
+	__debugfs = debugfs_find_mountpoint();
+	if (__debugfs == NULL) {
+		pr_warning("Debugfs is not mounted.\n");
+		return -ENOENT;
+	}
+
+	ret = e_snprintf(buf, PATH_MAX, "%stracing/kprobe_events", __debugfs);
 	if (ret >= 0) {
+		pr_debug("Opening %s write=%d\n", buf, readwrite);
 		if (readwrite && !probe_event_dry_run)
 			ret = open(buf, O_RDWR, O_APPEND);
 		else


-- 
Masami Hiramatsu
e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 0/6] perf-probe bugfixes

[Masami H. <mhi...@re...>]

Hi Arnaldo,

Could you pull these patches into your tree too?
These fixes several bugs on perf probe.

Thank you,

---

Masami Hiramatsu (6):
      perf probe: Show function entry line as probe-able
      perf probe: Support DW_OP_plus_uconst in DW_AT_data_member_location
      perf probe: Fix line range to show end line
      perf probe: Fix a bug that --line range can be overflow
      perf probe: Fix mis-estimation for shortening filename
      perf probe: Fix to use correct debugfs path finder


 tools/perf/builtin-probe.c     |    4 -
 tools/perf/util/probe-event.c  |   51 ++++++++++------
 tools/perf/util/probe-event.h  |    6 +-
 tools/perf/util/probe-finder.c |  128 ++++++++++++++++++++++++++++++++--------
 4 files changed, 139 insertions(+), 50 deletions(-)

-- 
Masami Hiramatsu
e-mail: mhi...@re...

Re: [Dle-develop] [PATCH -tip v3 00/10] perf-probe updates - data-structure support improvements, etc.

[Ingo M. <mi...@el...>]

* Arnaldo Carvalho de Melo <ac...@re...> wrote:

> Em Mon, Apr 12, 2010 at 01:16:46PM -0400, Masami Hiramatsu escreveu:
> > Hi Ingo and Arnaldo,
> > 
> > Here are several updates of perf-probe. This series improves data 
> > structure accessing. In this version, I added 'removing x*()' patches.
> 
> Ingo, I'm ok with this series, do you want me to pull them or will you?

Sure, feel free to pull it - you are doing many other tools/perf/ changes 
which might interact.

	Ingo

Re: [Dle-develop] [PATCH -tip v3 00/10] perf-probe updates - data-structure support improvements, etc.

[Arnaldo C. de M. <ac...@re...>]

Em Mon, Apr 12, 2010 at 01:16:46PM -0400, Masami Hiramatsu escreveu:
> Hi Ingo and Arnaldo,
> 
> Here are several updates of perf-probe. This series improves
> data structure accessing. In this version, I added 'removing x*()'
> patches.

Ingo, I'm ok with this series, do you want me to pull them or will you?

- Arnaldo

Re: [Dle-develop] [PATCH -tip v3 04/10] perf probe: Query basic types from debuginfo

[Arnaldo C. de M. <ac...@re...>]

Em Mon, Apr 12, 2010 at 01:17:15PM -0400, Masami Hiramatsu escreveu:
> Query the basic type information (byte-size and signed-flag) from
> debuginfo and pass that to kprobe-tracer. This is especially useful
> for tracing the members of data structure, because each member has
> different byte-size on the memory.
> 
> Signed-off-by: Masami Hiramatsu <mhi...@re...>
> Cc: Ingo Molnar <mi...@el...>
> Cc: Paul Mackerras <pa...@sa...>
> Cc: Arnaldo Carvalho de Melo <ac...@re...>
> Cc: Peter Zijlstra <pe...@in...>
> Cc: Mike Galbraith <ef...@gm...>
> Cc: Frederic Weisbecker <fwe...@gm...>
> ---
> 
>  tools/perf/util/probe-event.c  |    9 +++++
>  tools/perf/util/probe-event.h  |    1 +
>  tools/perf/util/probe-finder.c |   78 ++++++++++++++++++++++++++++++++++++----
>  3 files changed, 80 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 19de8b7..05ca4a9 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -740,6 +740,13 @@ static int synthesize_kprobe_trace_arg(struct kprobe_trace_arg *arg,
>  		buf += ret;
>  		buflen -= ret;
>  	}
> +	/* Print argument type */
> +	if (arg->type) {
> +		ret = e_snprintf(buf, buflen, ":%s", arg->type);
> +		if (ret <= 0)
> +			return ret;
> +		buf += ret;
> +	}
>  
>  	return buf - tmp;
>  }
> @@ -848,6 +855,8 @@ void clear_kprobe_trace_event(struct kprobe_trace_event *tev)
>  			free(tev->args[i].name);
>  		if (tev->args[i].value)
>  			free(tev->args[i].value);
> +		if (tev->args[i].type)
> +			free(tev->args[i].type);

It follows the existing style but as kfree, free() also can cope with
NULL pointers, so unconditionally calling free is a shorter form, but
this can be fixed later, possibily in a big cleanup in all of
tools/perf.

- Arnaldo