dle-develop — Dependable Linux Effort mailing list for developers

[Dle-develop] [PATCH -tip 4/8] perf probe: Remove event suffix number _0

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

Remove event suffix number _0 if it is the first.
The first event has no suffix, and from the second,
each event has suffix number counted from _1. This
reduces typing cost :-).

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Ingo Molnar <mi...@el...>
Cc: Steven Rostedt <ro...@go...>
Cc: Jim Keniston <jke...@us...>
Cc: Ananth N Mavinakayanahalli <an...@in...>
Cc: Christoph Hellwig <hc...@in...>
Cc: Frank Ch. Eigler <fc...@re...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Jason Baron <jb...@re...>
Cc: K.Prasad <pr...@li...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Srikar Dronamraju <sr...@li...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
---

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

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 2c4d301..31beedc 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -466,7 +466,16 @@ static void get_new_event_name(char *buf, size_t len, const char *base,
 			       struct strlist *namelist)
 {
 	int i, ret;
-	for (i = 0; i < MAX_EVENT_INDEX; i++) {
+
+	/* Try no suffix */
+	ret = e_snprintf(buf, len, "%s", base);
+	if (ret < 0)
+		die("snprintf() failed: %s", strerror(-ret));
+	if (!strlist__has_entry(namelist, buf))
+		return;
+
+	/* Try to add suffix */
+	for (i = 1; i < MAX_EVENT_INDEX; i++) {
 		ret = e_snprintf(buf, len, "%s_%d", base, i);
 		if (ret < 0)
 			die("snprintf() failed: %s", strerror(-ret));


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

[Dle-develop] [PATCH -tip 1/8] perf probe: Change event list format

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

Change event list format for user readability. perf probe --list
shows event list in "[GROUP:EVENT] EVENT-DEFINITION" format, but
this format is different from the output of perf-list, and
EVENT-DEFINITION is a bit blunt. This patch changes the
format to more user friendly one.

Before:
[probe:shedule_0]	schedule+10 prev cpu

After:
  probe:schedule_0                         (on schedule+10 with prev cpu)

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Ingo Molnar <mi...@el...>
Cc: Steven Rostedt <ro...@go...>
Cc: Jim Keniston <jke...@us...>
Cc: Ananth N Mavinakayanahalli <an...@in...>
Cc: Christoph Hellwig <hc...@in...>
Cc: Frank Ch. Eigler <fc...@re...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Jason Baron <jb...@re...>
Cc: K.Prasad <pr...@li...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Srikar Dronamraju <sr...@li...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
---

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

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 88e1804..a20e382 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -379,11 +379,29 @@ static void clear_probe_point(struct probe_point *pp)
 	memset(pp, 0, sizeof(pp));
 }
 
+/* Show an event */
+static void show_perf_probe_event(const char *group, const char *event,
+				  const char *place, struct probe_point *pp)
+{
+	int i;
+	char buf[128];
+
+	e_snprintf(buf, 128, "%s:%s", group, event);
+	printf("  %-40s (on %s", buf, place);
+
+	if (pp->nr_args > 0) {
+		printf(" with");
+		for (i = 0; i < pp->nr_args; i++)
+			printf(" %s", pp->args[i]);
+	}
+	printf(")\n");
+}
+
 /* List up current perf-probe events */
 void show_perf_probe_events(void)
 {
 	unsigned int i;
-	int fd;
+	int fd, nr;
 	char *group, *event;
 	struct probe_point pp;
 	struct strlist *rawlist;
@@ -396,8 +414,13 @@ void show_perf_probe_events(void)
 	for (i = 0; i < strlist__nr_entries(rawlist); i++) {
 		ent = strlist__entry(rawlist, i);
 		parse_trace_kprobe_event(ent->s, &group, &event, &pp);
+		/* Synthesize only event probe point */
+		nr = pp.nr_args;
+		pp.nr_args = 0;
 		synthesize_perf_probe_event(&pp);
-		printf("[%s:%s]\t%s\n", group, event, pp.probes[0]);
+		pp.nr_args = nr;
+		/* Show an event */
+		show_perf_probe_event(group, event, pp.probes[0], &pp);
 		free(group);
 		free(event);
 		clear_probe_point(&pp);


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

[Dle-develop] [PATCH -tip 2/8] perf probe: Change probe-added message more user-friendly

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

Change probe-added message more user-friendly expression and
show usage of new events.

Before:
Added new event: p:probe/schedule_0 schedule+10 prev=%ax cpu=%bx

After:
Added new event:
  probe:schedule_1                         (on schedule+1 with prev cpu)

You can now use it on all perf tools, such as:

        perf record -e probe:schedule_1 -a sleep 1

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Ingo Molnar <mi...@el...>
Cc: Steven Rostedt <ro...@go...>
Cc: Jim Keniston <jke...@us...>
Cc: Ananth N Mavinakayanahalli <an...@in...>
Cc: Christoph Hellwig <hc...@in...>
Cc: Frank Ch. Eigler <fc...@re...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Jason Baron <jb...@re...>
Cc: K.Prasad <pr...@li...>
Cc: Peter Zijlstra <pe...@in...>
Cc: Srikar Dronamraju <sr...@li...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
---

 tools/perf/util/probe-event.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index a20e382..2c4d301 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -453,17 +453,13 @@ static struct strlist *get_perf_event_names(int fd)
 	return sl;
 }
 
-static int write_trace_kprobe_event(int fd, const char *buf)
+static void write_trace_kprobe_event(int fd, const char *buf)
 {
 	int ret;
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret <= 0)
 		die("Failed to create event.");
-	else
-		printf("Added new event: %s\n", buf);
-
-	return ret;
 }
 
 static void get_new_event_name(char *buf, size_t len, const char *base,
@@ -503,10 +499,19 @@ void add_trace_kprobe_events(struct probe_point *probes, int nr_probes)
 				 PERFPROBE_GROUP, event,
 				 pp->probes[i]);
 			write_trace_kprobe_event(fd, buf);
+			printf("Added new event:\n");
+			/* Get the first parameter (probe-point) */
+			sscanf(pp->probes[i], "%s", buf);
+			show_perf_probe_event(PERFPROBE_GROUP, event,
+					      buf, pp);
 			/* Add added event name to namelist */
 			strlist__add(namelist, event);
 		}
 	}
+	/* Show how to use the event. */
+	printf("\nYou can now use it on all perf tools, such as:\n\n");
+	printf("\tperf record -e %s:%s -a sleep 1\n\n", PERFPROBE_GROUP, event);
+
 	strlist__delete(namelist);
 	close(fd);
 }


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

[Dle-develop] [PATCH -tip 0/8] perf-probe updates

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

Hi Ingo,

I made several usability updates and added --del option
for perf-probe. I think most of basic functions are implemented.
I'm planning to support checking build-id next, because it
can prevent users to use old vmlinux for debuginfo analysis.

Here are the todo list I have (most of them had been requested
on LKML). I'd like to share this list with other developers
who are interested in.

Short-term TODOs:
  - Support checking kernel Build-ID

Long-term TODOs (future features):
  - Support --line option to show which lines user can probe
  - Support lazy string matching(glob?) for selecting probing
    line
  - Support sys_perf_counter_open (for non-root users)
  - Support tracing static variables (non global)
  - Support variable types from debuginfo (e.g. char, int, ...)
  - Support fields of data structures (var->field)
  - Support array (var[N])
  - Support dynamic array-indexing (var[var2])
  - Support string/dynamic arrays (*var, var[N..M])
  - Support force type-casting ((type)var)
  - Support the type of return value

Miscs:
  - Support glob expression with --del option (like --del "*")
  - Support event/group name specifying for new events
  - Better support for probes on modules
  - Symbol search by libelf/kallsyms
  - Move onto libdw/libdwfl
  - Storing file name/line number information in the
    kernel for listing events
  

Thank you,

---

Masami Hiramatsu (8):
      perf probe: Update perf-probe document
      perf probe: Support --del option
      trace-kprobe: Support delete probe syntax
      perf probe: Support vmlinux on cwd by default
      perf probe: Remove event suffix number _0
      perf probe: Fix add-probe command syntax without --add option
      perf probe: Change probe-added message more user-friendly
      perf probe: Change event list format


 kernel/trace/trace_kprobe.c             |   37 +++++++--
 tools/perf/Documentation/perf-probe.txt |   21 ++++-
 tools/perf/builtin-probe.c              |   76 ++++++++++++++++---
 tools/perf/util/probe-event.c           |  122 ++++++++++++++++++++++++++++---
 tools/perf/util/probe-event.h           |    1 
 5 files changed, 217 insertions(+), 40 deletions(-)

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division
e-mail: mhi...@re...

[Dle-develop] [PATCH v3] [RFC] tracepoint: Add signal coredump tracepoint

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

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

This tracepoint requirement comes mainly from the viewpoint of
administrators. Since now we have introduced many coredump
configurations (e.g. dumpable, coredump_filter, core_pattern,
etc) and some of them can be modified by users, it will be hard
to know what was actually dumped (or not dumped) after some
problem happened on the system. For example, a process didn't
generated core, coredump doesn't have some sections, etc.
In those cases, the coredump tracepoint can help us to know
why the core file is so big or small, or not generated, by
recording all configurations for all processes on the system.
That will reduce system-administration cost.

Changes in v3:
 - Move tracepoint at the end of do_coredump() for tracing
   the result of coredump.
 - Use retval to record error-code at every failure points
   for passing the result of coredump to tracepoint.
 - Trace retval instead of cprm->file for recording the
   result of coredump.

Changes in v2:
 - Fix a bug to clear file local variable when
   call_usermodehelper_pipe() is failed.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Oleg Nesterov <ol...@re...>
Cc: Roland McGrath <ro...@re...>
Cc: Jason Baron <jb...@re...>
Cc: Ingo Molnar <mi...@el...>
Cc: Andrew Morton <ak...@li...>
Cc: KOSAKI Motohiro <kos...@jp...>
---

 fs/exec.c                     |   58 ++++++++++++++++++++++++++++++-----------
 include/trace/events/signal.h |   48 ++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 16 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 0a5d944..d67ed5a 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -55,6 +55,7 @@
 #include <linux/fsnotify.h>
 #include <linux/fs_struct.h>
 #include <linux/pipe_fs_i.h>
+#include <trace/events/signal.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -1753,7 +1754,7 @@ static void wait_for_dump_helpers(struct file *file)
 void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 {
 	struct core_state core_state;
-	char corename[CORENAME_MAX_SIZE + 1];
+	char corename[CORENAME_MAX_SIZE + 1] = "";
 	struct mm_struct *mm = current->mm;
 	struct linux_binfmt * binfmt;
 	struct inode * inode;
@@ -1768,6 +1769,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	static atomic_t core_dump_count = ATOMIC_INIT(0);
 	struct coredump_params cprm = {
 		.signr = signr,
+		.file = NULL,
 		.regs = regs,
 		.limit = current->signal->rlim[RLIMIT_CORE].rlim_cur,
 		/*
@@ -1781,8 +1783,10 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	audit_core_dumps(signr);
 
 	binfmt = mm->binfmt;
-	if (!binfmt || !binfmt->core_dump)
+	if (!binfmt || !binfmt->core_dump) {
+		retval = -ENOSYS;
 		goto fail;
+	}
 
 	cred = prepare_creds();
 	if (!cred) {
@@ -1795,6 +1799,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	 * If another thread got here first, or we are not dumpable, bail out.
 	 */
 	if (mm->core_state || !__get_dumpable(cprm.mm_flags)) {
+		/* This is not an error. retval should be 0 */
 		up_write(&mm->mmap_sem);
 		put_cred(cred);
 		goto fail;
@@ -1833,11 +1838,14 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	ispipe = format_corename(corename, signr);
 	unlock_kernel();
 
-	if ((!ispipe) && (cprm.limit < binfmt->min_coredump))
+	if ((!ispipe) && (cprm.limit < binfmt->min_coredump)) {
+		retval = -EFBIG;
 		goto fail_unlock;
+	}
 
  	if (ispipe) {
 		if (cprm.limit == 0) {
+			retval = -EINVAL;
 			/*
 			 * Normally core limits are irrelevant to pipes, since
 			 * we're not writing to the file system, but we use
@@ -1861,6 +1869,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 
 		dump_count = atomic_inc_return(&core_dump_count);
 		if (core_pipe_limit && (core_pipe_limit < dump_count)) {
+			retval = -EFBIG;
 			printk(KERN_WARNING "Pid %d(%s) over core_pipe_limit\n",
 			       task_tgid_vnr(current), current->comm);
 			printk(KERN_WARNING "Skipping core dump\n");
@@ -1869,6 +1878,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 
 		helper_argv = argv_split(GFP_KERNEL, corename+1, &helper_argc);
 		if (!helper_argv) {
+			retval = -ENOMEM;
 			printk(KERN_WARNING "%s failed to allocate memory\n",
 			       __func__);
 			goto fail_dropcount;
@@ -1877,8 +1887,9 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		cprm.limit = RLIM_INFINITY;
 
 		/* SIGPIPE can happen, but it's just never processed */
-		if (call_usermodehelper_pipe(helper_argv[0], helper_argv, NULL,
-				&cprm.file)) {
+		retval = call_usermodehelper_pipe(helper_argv[0], helper_argv,
+						  NULL, &cprm.file);
+		if (retval < 0) {
  			printk(KERN_INFO "Core dump to %s pipe failed\n",
 			       corename);
 			goto fail_dropcount;
@@ -1887,31 +1898,43 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		cprm.file = filp_open(corename,
 				 O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE | flag,
 				 0600);
-	if (IS_ERR(cprm.file))
+	if (IS_ERR(cprm.file)) {
+		retval = (int)PTR_ERR(cprm.file);
 		goto fail_dropcount;
+	}
 	inode = cprm.file->f_path.dentry->d_inode;
-	if (inode->i_nlink > 1)
+	if (inode->i_nlink > 1) {
+		retval = -EMLINK;
 		goto close_fail;	/* multiple links - don't dump */
-	if (!ispipe && d_unhashed(cprm.file->f_path.dentry))
+	}
+	if (!ispipe && d_unhashed(cprm.file->f_path.dentry)) {
+		retval = -EBADF;
 		goto close_fail;
+	}
 
 	/* AK: actually i see no reason to not allow this for named pipes etc.,
 	   but keep the previous behaviour for now. */
-	if (!ispipe && !S_ISREG(inode->i_mode))
+	if (!ispipe && !S_ISREG(inode->i_mode)) {
+		retval = -EBADF;
 		goto close_fail;
+	}
 	/*
 	 * Dont allow local users get cute and trick others to coredump
 	 * into their pre-created files:
 	 */
-	if (inode->i_uid != current_fsuid())
+	if (inode->i_uid != current_fsuid()) {
+		retval = -EPERM;
 		goto close_fail;
-	if (!cprm.file->f_op)
-		goto close_fail;
-	if (!cprm.file->f_op->write)
-		goto close_fail;
-	if (!ispipe &&
-	    do_truncate(cprm.file->f_path.dentry, 0, 0, cprm.file) != 0)
+	}
+	if (!cprm.file->f_op || !cprm.file->f_op->write) {
+		retval = -EINVAL;
 		goto close_fail;
+	}
+	if (!ispipe) {
+		retval = do_truncate(cprm.file->f_path.dentry, 0, 0, cprm.file);
+		if (retval != 0)
+			goto close_fail;
+	}
 
 	retval = binfmt->core_dump(&cprm);
 
@@ -1932,5 +1955,8 @@ fail_unlock:
 	put_cred(cred);
 	coredump_finish(mm);
 fail:
+	/* Trace coredump parameters and return value */
+	trace_signal_coredump(&cprm, corename, retval);
+
 	return;
 }
diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index a510b75..42608f0 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -4,8 +4,10 @@
 #if !defined(_TRACE_SIGNAL_H) || defined(TRACE_HEADER_MULTI_READ)
 #define _TRACE_SIGNAL_H
 
+#include <linux/err.h>
 #include <linux/signal.h>
 #include <linux/sched.h>
+#include <linux/binfmts.h>
 #include <linux/tracepoint.h>
 
 #define TP_STORE_SIGINFO(__entry, info)				\
@@ -167,6 +169,52 @@ TRACE_EVENT(signal_lose_info,
 	TP_printk("sig=%d group=%d errno=%d code=%d",
 		  __entry->sig, __entry->group, __entry->errno, __entry->code)
 );
+
+/**
+ * signal_coredump - called when dumping core by signal
+ * @cprm: pointer to struct coredump_params
+ * @core_name: core-name string
+ * @retval: return value of binfmt->coredump or error-code
+ *
+ * Current process dumps core file to 'core_name' file, because 'cprm->signr'
+ * signal is delivered.
+ * 'retval' is an error code or 0/1. retval == 1 means the core file was
+ * dumped successfully and retval == 0 means binfmt->coredump failed to dump.
+ * If retval < 0, this means do_coredump() failed to dump core file before
+ * calling binfmt->coredump.
+ */
+TRACE_EVENT(signal_coredump,
+
+	TP_PROTO(struct coredump_params *cprm, const char *core_name,
+		 int retval),
+
+	TP_ARGS(cprm, core_name, retval),
+
+	TP_STRUCT__entry(
+		__field(	int,		sig		)
+		__field(	unsigned long,	limit		)
+		__field(	unsigned long,	flags		)
+		__field(	int,		retval		)
+		__string(	name,		core_name	)
+	),
+
+
+	TP_fast_assign(
+		__entry->sig	= (int)cprm->signr;
+		__entry->limit	= cprm->limit;
+		__entry->flags	= cprm->mm_flags;
+		__entry->retval	= retval;
+		__assign_str(name,	core_name);
+	),
+
+	TP_printk("sig=%d limit=%lu dumpable=0x%lx dump_filter=0x%lx "
+		  "corename=\"%s\" retval=%d",
+		  __entry->sig, __entry->limit,
+		  __entry->flags & MMF_DUMPABLE_MASK,
+		  (__entry->flags & MMF_DUMP_FILTER_MASK) >>
+		  MMF_DUMP_FILTER_SHIFT,
+		  __get_str(name), __entry->retval)
+);
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

Re: [Dle-develop] [PATCH v2] [RFC] tracepoint: Add signal coredump tracepoint

[KOSAKI M. <kos...@jp...>]

> KOSAKI Motohiro wrote:
> > 2009/12/3 Masami Hiramatsu<mhi...@re...>:
> >> Add signal coredump tracepoint which shows signal number,
> >> mm->flags, limits, pointer to file structure and core
> >> file name.
> >>
> >> This tracepoint requirement comes mainly from the viewpoint of
> >> administrators. Since now we have introduced many coredump
> >> configurations (e.g. dumpable, coredump_filter, core_pattern,
> >> etc) and some of them can be modified by users, it will be hard
> >> to know what was actually dumped (or not dumped) after some
> >> problem happened on the system. For example, a process didn't
> >> generated core, coredump doesn't have some sections, etc.
> >> In those cases, the coredump tracepoint can help us to know
> >> why the core file is so big or small, or not generated, by
> >> recording all configurations for all processes on the system.
> >> That will reduce system-administration cost.
> >
> > AFAIK, not-dumped case is important than dump successful case.
> > IOW, admin need to know why the crashed process was not dumped.
> 
> Certainly, failure cases are important, but also, the cases
> that dumped-core doesn't or does include some sections
> are also important.

correct.

> > This tracepoint doesn't cover all failure case. especially
> > binfmt->core_dump() et.al.
> > IOW, this tracepoint seems too specialized piped-coredump feature.
> 
> Hmm, so would you mean that after calling binfmt->core_dump()
> is better place?

I think your following use-case desired so. if you have unwritten reason, please correct me.

	For example, a process didn't generated core, coredump doesn't have
	some sections, etc.


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

> 
> e.g.
>          if (IS_ERR(file))
>                  goto fail_dropcount;
> +	retval = -EBADF;
>          inode = file->f_path.dentry->d_inode;
>          if (inode->i_nlink > 1)
>                  goto close_fail;        /* multiple links - don't dump */
>          if (!ispipe && d_unhashed(file->f_path.dentry))
>                  goto close_fail;
> 
>          /* AK: actually i see no reason to not allow this for named pipes etc.,
>             but keep the previous behaviour for now. */
>          if (!ispipe && !S_ISREG(inode->i_mode))
>                  goto close_fail;
>          /*
>           * Dont allow local users get cute and trick others to coredump
>           * into their pre-created files:
>           */
> +	retval = -EPERM;
>          if (inode->i_uid != current_fsuid())
>                  goto close_fail;
> +	retval = -EINVAL;
>          if (!file->f_op)
>                  goto close_fail;
>          if (!file->f_op->write)
>                  goto close_fail;
> +	retval = -EEXIST;
>          if (!ispipe && do_truncate(file->f_path.dentry, 0, 0, file) != 0)
>                  goto close_fail;
> 
> 
> Thank you,
> 
> -- 
> Masami Hiramatsu
> 
> Software Engineer
> Hitachi Computer Products (America), Inc.
> Software Solutions Division
> 
> e-mail: mhi...@re...
> 

[Dle-develop] [tip:perf/urgent] perf probe: Check e_snprintf() format string

[tip-bot f. M. H. <mhi...@re...>]

Commit-ID:  849884508ecbe2d220131840e4cc7c32ca24ebe3
Gitweb:     http://git.kernel.org/tip/849884508ecbe2d220131840e4cc7c32ca24ebe3
Author:     Masami Hiramatsu <mhi...@re...>
AuthorDate: Mon, 7 Dec 2009 12:00:53 -0500
Committer:  Ingo Molnar <mi...@el...>
CommitDate: Mon, 7 Dec 2009 18:33:21 +0100

perf probe: Check e_snprintf() format string

Check e_snprintf() format string by gcc, and fix a bug of
e_snprintf() caller.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: systemtap <sys...@so...>
Cc: DLE <dle...@li...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Frederic Weisbecker <fwe...@gm...>
LKML-Reference: <200...@dh...>
Signed-off-by: Ingo Molnar <mi...@el...>
---
 tools/perf/util/probe-event.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index de0d913..88e1804 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -48,6 +48,9 @@
 
 /* If there is no space to write, returns -E2BIG. */
 static int e_snprintf(char *str, size_t size, const char *format, ...)
+	__attribute__((format(printf, 3, 4)));
+
+static int e_snprintf(char *str, size_t size, const char *format, ...)
 {
 	int ret;
 	va_list ap;
@@ -258,7 +261,7 @@ int synthesize_perf_probe_event(struct probe_point *pp)
 		ret = e_snprintf(buf, MAX_CMDLEN, "%s%s%s%s", pp->function,
 				 offs, pp->retprobe ? "%return" : "", line);
 	else
-		ret = e_snprintf(buf, MAX_CMDLEN, "%s%s%s%s", pp->file, line);
+		ret = e_snprintf(buf, MAX_CMDLEN, "%s%s", pp->file, line);
 	if (ret <= 0)
 		goto error;
 	len = ret;

[Dle-develop] [tip:perf/urgent] perf probe: Use pr_debug for debug message

[tip-bot f. M. H. <mhi...@re...>]

Commit-ID:  d3a2dbf844d81b4b9c9ad6044563c294e7a48cac
Gitweb:     http://git.kernel.org/tip/d3a2dbf844d81b4b9c9ad6044563c294e7a48cac
Author:     Masami Hiramatsu <mhi...@re...>
AuthorDate: Mon, 7 Dec 2009 12:00:59 -0500
Committer:  Ingo Molnar <mi...@el...>
CommitDate: Mon, 7 Dec 2009 18:33:22 +0100

perf probe: Use pr_debug for debug message

Use pr_debug() for "missing vmlinux" debugging message.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: systemtap <sys...@so...>
Cc: DLE <dle...@li...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Frederic Weisbecker <fwe...@gm...>
LKML-Reference: <200...@dh...>
Signed-off-by: Ingo Molnar <mi...@el...>
---
 tools/perf/builtin-probe.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index a58e11b..8993a1f 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -194,8 +194,8 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
 		if (session.need_dwarf)
 			die("Could not open vmlinux/module file.");
 
-		pr_warning("Could not open vmlinux/module file."
-			   " Try to use symbols.\n");
+		pr_debug("Could not open vmlinux/module file."
+			 " Try to use symbols.\n");
 		goto end_dwarf;
 	}
 

[Dle-develop] [tip:perf/urgent] perf probe: Fix event namelist to duplicate string

[tip-bot f. M. H. <mhi...@re...>]

Commit-ID:  e1d2017b24fb31602f1128e6a8b2afc54c9283cd
Gitweb:     http://git.kernel.org/tip/e1d2017b24fb31602f1128e6a8b2afc54c9283cd
Author:     Masami Hiramatsu <mhi...@re...>
AuthorDate: Mon, 7 Dec 2009 12:00:46 -0500
Committer:  Ingo Molnar <mi...@el...>
CommitDate: Mon, 7 Dec 2009 18:33:21 +0100

perf probe: Fix event namelist to duplicate string

Fix event namelist to duplicate string. Without duplicating, adding
multiple probes causes stack overwrite bug, because it reuses a
buffer on stack while the buffer is already added in the namelist.
String duplication solves this bug because only contents of the
buffer is copied to the namelist.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: systemtap <sys...@so...>
Cc: DLE <dle...@li...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
Cc: Frederic Weisbecker <fwe...@gm...>
LKML-Reference: <200...@dh...>
Signed-off-by: Ingo Molnar <mi...@el...>
---
 tools/perf/util/probe-event.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index cd7fbda..de0d913 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -413,12 +413,13 @@ static struct strlist *get_perf_event_names(int fd)
 
 	rawlist = get_trace_kprobe_event_rawlist(fd);
 
-	sl = strlist__new(false, NULL);
+	sl = strlist__new(true, NULL);
 	for (i = 0; i < strlist__nr_entries(rawlist); i++) {
 		ent = strlist__entry(rawlist, i);
 		parse_trace_kprobe_event(ent->s, &group, &event, NULL);
 		strlist__add(sl, event);
 		free(group);
+		free(event);
 	}
 
 	strlist__delete(rawlist);
@@ -480,5 +481,6 @@ void add_trace_kprobe_events(struct probe_point *probes, int nr_probes)
 			strlist__add(namelist, event);
 		}
 	}
+	strlist__delete(namelist);
 	close(fd);
 }

[Dle-develop] [tip:perf/urgent] x86 insn: Delete empty or incomplete inat-tables.c

[tip-bot f. M. H. <mhi...@re...>]

Commit-ID:  d32ba45503acf9c23b301eba2397ca2ee322627b
Gitweb:     http://git.kernel.org/tip/d32ba45503acf9c23b301eba2397ca2ee322627b
Author:     Masami Hiramatsu <mhi...@re...>
AuthorDate: Mon, 7 Dec 2009 12:00:33 -0500
Committer:  Ingo Molnar <mi...@el...>
CommitDate: Mon, 7 Dec 2009 18:33:19 +0100

x86 insn: Delete empty or incomplete inat-tables.c

Delete empty or incomplete inat-tables.c if gen-insn-attr-x86.awk
failed, because it causes a build error if user tries to build
kernel next time.

Reported-by: Arkadiusz Miskiewicz <ar...@ma...>
Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: systemtap <sys...@so...>
Cc: DLE <dle...@li...>
Cc: Jens Axboe <jen...@or...>
Cc: Frederic Weisbecker <fwe...@gm...>
LKML-Reference: <200...@dh...>
Signed-off-by: Ingo Molnar <mi...@el...>
---
 arch/x86/lib/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index 442b3b3..45b20e4 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -5,7 +5,7 @@
 inat_tables_script = $(srctree)/arch/x86/tools/gen-insn-attr-x86.awk
 inat_tables_maps = $(srctree)/arch/x86/lib/x86-opcode-map.txt
 quiet_cmd_inat_tables = GEN     $@
-      cmd_inat_tables = $(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@
+      cmd_inat_tables = $(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@
 
 $(obj)/inat-tables.c: $(inat_tables_script) $(inat_tables_maps)
 	$(call cmd,inat_tables)

[Dle-develop] [tip:perf/urgent] perf probe: Fix strtailcmp() to compare s1and s2[0]

[tip-bot f. J. L. <juh...@lu...>]

Commit-ID:  d56728b8d7fb3e1e5e5f97b88fdf6b43a35b4f5e
Gitweb:     http://git.kernel.org/tip/d56728b8d7fb3e1e5e5f97b88fdf6b43a35b4f5e
Author:     Juha Leppanen <juh...@lu...>
AuthorDate: Mon, 7 Dec 2009 12:00:40 -0500
Committer:  Ingo Molnar <mi...@el...>
CommitDate: Mon, 7 Dec 2009 18:33:20 +0100

perf probe: Fix strtailcmp() to compare s1and s2[0]

Fix strtailcmp() to compare s1[0] and s2[0]. strtailcmp() returns 0
if "a" and "b" or "a" and "ab", it's a wrong behavior. This patch
fixes it.

Signed-off-by: "Juha Leppanen" <juh...@lu...>
Acked-by: Masami Hiramatsu <mhi...@re...>
Cc: systemtap <sys...@so...>
Cc: DLE <dle...@li...>
Cc: Juha Leppanen <juh...@lu...>
Cc: Frederic Weisbecker <fwe...@gm...>
LKML-Reference: <200...@dh...>
Signed-off-by: Ingo Molnar <mi...@el...>
---
 tools/perf/util/probe-finder.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 293cdfc..4585f1d 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -106,7 +106,7 @@ static int strtailcmp(const char *s1, const char *s2)
 {
 	int i1 = strlen(s1);
 	int i2 = strlen(s2);
-	while (--i1 > 0 && --i2 > 0) {
+	while (--i1 >= 0 && --i2 >= 0) {
 		if (s1[i1] != s2[i2])
 			return s1[i1] - s2[i2];
 	}

[Dle-develop] [BUGFIX PATCH -tip 5/5] perf probe: Use pr_debug for debug message

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

Use pr_debug() for "missing vmlinux" debugging message.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Ingo Molnar <mi...@el...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
---

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

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index a58e11b..8993a1f 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -194,8 +194,8 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
 		if (session.need_dwarf)
 			die("Could not open vmlinux/module file.");
 
-		pr_warning("Could not open vmlinux/module file."
-			   " Try to use symbols.\n");
+		pr_debug("Could not open vmlinux/module file."
+			 " Try to use symbols.\n");
 		goto end_dwarf;
 	}
 


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

Re: [Dle-develop] [PATCH v2] [RFC] tracepoint: Add signal coredump tracepoint

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

Ingo Molnar wrote:
>
> * Masami Hiramatsu<mhi...@re...>  wrote:
>
>> Ingo Molnar wrote:
>>>
>>> * Masami Hiramatsu<mhi...@re...>  wrote:
>>>
>>>> Add signal coredump tracepoint which shows signal number, mm->flags,
>>>> limits, pointer to file structure and core file name.
>>>
>>> Why is the kernel pointer to the file structure logged? User-space has
>>> no use for it and the analysis value is low.
>>
>> Ah, if open() or opening pipe fails, it becomes 0 or -ERRNO, so we can
>> check if there is an error.
>
> ok, that wasnt obvious from the patch - worth adding it to the
> changelog.

OK.

>> Perhaps, we can do below in trace_printk for trace users.
>> "open %s", (!file || IS_ERR((void *)file)) ? "failed" : "succeeded"
>
> i'd rather suggest to pass an error code (and keep it 0 if none),
> instead of some ad-hoc string message.

Sure. Or, perhaps, is it enough to show error code? (as block_rq_with_error did)

> But ... the whole issue of VFS event logging and new tracepoints should
> be approached from a more generic direction i think. Do we want to log
> inode_nr:dev pairs as well? Shouldnt there be a generic event-class
> definition via DECLARE_EVENT_CLASS for file related events, with 'core
> dumped' just being a sub-event-code?

Hmm, would you mean that coredump event should be a VFS event? or
handling file open errors should be a VFS event?
There are many other special reasons of failing coredump, as I discussed
with Kosaki-san. So, I think coredump event should be different from VFS
events.

Of course, Failure of file opening event should be handled in VFS events
if possible. In that case, we just need to trace coredump event and
VFS open event, and matching file descriptor or something like that.

> I sense reluctance from the direction of Andrew and disinterest from the
> VFS folks - not a good backdrop in general.
>
> 	Ingo

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

[Dle-develop] [BUGFIX PATCH -tip 0/5] perf-probe/x86 insn decoder bugfixes

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

Hi Ingo,

Here are several bugfixes which are reported on LKML
for perf-probe and x86 insn decoder.

Thank you,

---

Juha Leppanen (1):
      perf probe: Fix strtailcmp() to compare s1[0] and s2[0]

Masami Hiramatsu (4):
      perf probe: Use pr_debug for debug message
      perf probe: Check e_snprintf() format string
      perf probe: Fix event namelist to duplicate string
      x86 insn: Delete empty or incomplete inat-tables.c


 arch/x86/lib/Makefile          |    2 +-
 tools/perf/builtin-probe.c     |    4 ++--
 tools/perf/util/probe-event.c  |    9 +++++++--
 tools/perf/util/probe-finder.c |    2 +-
 4 files changed, 11 insertions(+), 6 deletions(-)

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division
e-mail: mhi...@re...

[Dle-develop] [BUGFIX PATCH -tip 2/5] perf probe: Fix strtailcmp() to compare s1[0] and s2[0]

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

From: Juha Leppanen <juh...@lu...>

Fix strtailcmp() to compare s1[0] and s2[0].
strtailcmp() returns 0 if "a" and "b" or "a" and "ab", it's
a wrong behavior. This patch fixes it.

Signed-off-by: "Juha Leppanen" <juh...@lu...>
Reported-by: "Juha Leppanen" <juh...@lu...>
Acked-by: Masami Hiramatsu <mhi...@re...>
---

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

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 293cdfc..4585f1d 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -106,7 +106,7 @@ static int strtailcmp(const char *s1, const char *s2)
 {
 	int i1 = strlen(s1);
 	int i2 = strlen(s2);
-	while (--i1 > 0 && --i2 > 0) {
+	while (--i1 >= 0 && --i2 >= 0) {
 		if (s1[i1] != s2[i2])
 			return s1[i1] - s2[i2];
 	}


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

[Dle-develop] [BUGFIX PATCH -tip 4/5] perf probe: Check e_snprintf() format string

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

Check e_snprintf() format string by gcc, and fix a bug of
e_snprintf() caller.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Ingo Molnar <mi...@el...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
---

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

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index de0d913..88e1804 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -48,6 +48,9 @@
 
 /* If there is no space to write, returns -E2BIG. */
 static int e_snprintf(char *str, size_t size, const char *format, ...)
+	__attribute__((format(printf, 3, 4)));
+
+static int e_snprintf(char *str, size_t size, const char *format, ...)
 {
 	int ret;
 	va_list ap;
@@ -258,7 +261,7 @@ int synthesize_perf_probe_event(struct probe_point *pp)
 		ret = e_snprintf(buf, MAX_CMDLEN, "%s%s%s%s", pp->function,
 				 offs, pp->retprobe ? "%return" : "", line);
 	else
-		ret = e_snprintf(buf, MAX_CMDLEN, "%s%s%s%s", pp->file, line);
+		ret = e_snprintf(buf, MAX_CMDLEN, "%s%s", pp->file, line);
 	if (ret <= 0)
 		goto error;
 	len = ret;


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

[Dle-develop] [BUGFIX PATCH -tip 3/5] perf probe: Fix event namelist to duplicate string

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

Fix event namelist to duplicate string. Without duplicating,
adding multiple probes causes stack overwrite bug, because
it reuses a buffer on stack while the buffer is already
added in the namelist. String duplication solves this bug
because only contents of the buffer is copied to the namelist.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Ingo Molnar <mi...@el...>
Cc: Frederic Weisbecker <fwe...@gm...>
Cc: Arnaldo Carvalho de Melo <ac...@re...>
---

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

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index cd7fbda..de0d913 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -413,12 +413,13 @@ static struct strlist *get_perf_event_names(int fd)
 
 	rawlist = get_trace_kprobe_event_rawlist(fd);
 
-	sl = strlist__new(false, NULL);
+	sl = strlist__new(true, NULL);
 	for (i = 0; i < strlist__nr_entries(rawlist); i++) {
 		ent = strlist__entry(rawlist, i);
 		parse_trace_kprobe_event(ent->s, &group, &event, NULL);
 		strlist__add(sl, event);
 		free(group);
+		free(event);
 	}
 
 	strlist__delete(rawlist);
@@ -480,5 +481,6 @@ void add_trace_kprobe_events(struct probe_point *probes, int nr_probes)
 			strlist__add(namelist, event);
 		}
 	}
+	strlist__delete(namelist);
 	close(fd);
 }


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

[Dle-develop] [BUGFIX PATCH -tip 1/5] x86 insn: Delete empty or incomplete inat-tables.c

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

Delete empty or incomplete inat-tables.c if gen-insn-attr-x86.awk
failed, because it causes a build error if user tries to build
kernel next time.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Reported-by: Arkadiusz Miskiewicz <ar...@ma...>
Cc: Arkadiusz Miskiewicz <ar...@ma...>
Cc: Jens Axboe <jen...@or...>
Cc: Ingo Molnar <mi...@el...>
---

 arch/x86/lib/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index 442b3b3..45b20e4 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -5,7 +5,7 @@
 inat_tables_script = $(srctree)/arch/x86/tools/gen-insn-attr-x86.awk
 inat_tables_maps = $(srctree)/arch/x86/lib/x86-opcode-map.txt
 quiet_cmd_inat_tables = GEN     $@
-      cmd_inat_tables = $(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@
+      cmd_inat_tables = $(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@
 
 $(obj)/inat-tables.c: $(inat_tables_script) $(inat_tables_maps)
 	$(call cmd,inat_tables)


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

Re: [Dle-develop] [PATCH v2] [RFC] tracepoint: Add signal coredump tracepoint

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

KOSAKI Motohiro wrote:
> 2009/12/3 Masami Hiramatsu<mhi...@re...>:
>> Add signal coredump tracepoint which shows signal number,
>> mm->flags, limits, pointer to file structure and core
>> file name.
>>
>> This tracepoint requirement comes mainly from the viewpoint of
>> administrators. Since now we have introduced many coredump
>> configurations (e.g. dumpable, coredump_filter, core_pattern,
>> etc) and some of them can be modified by users, it will be hard
>> to know what was actually dumped (or not dumped) after some
>> problem happened on the system. For example, a process didn't
>> generated core, coredump doesn't have some sections, etc.
>> In those cases, the coredump tracepoint can help us to know
>> why the core file is so big or small, or not generated, by
>> recording all configurations for all processes on the system.
>> That will reduce system-administration cost.
>
> AFAIK, not-dumped case is important than dump successful case.
> IOW, admin need to know why the crashed process was not dumped.

Certainly, failure cases are important, but also, the cases
that dumped-core doesn't or does include some sections
are also important.

> This tracepoint doesn't cover all failure case. especially
> binfmt->core_dump() et.al.
> IOW, this tracepoint seems too specialized piped-coredump feature.

Hmm, so would you mean that after calling binfmt->core_dump()
is better place?

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

e.g.
         if (IS_ERR(file))
                 goto fail_dropcount;
+	retval = -EBADF;
         inode = file->f_path.dentry->d_inode;
         if (inode->i_nlink > 1)
                 goto close_fail;        /* multiple links - don't dump */
         if (!ispipe && d_unhashed(file->f_path.dentry))
                 goto close_fail;

         /* AK: actually i see no reason to not allow this for named pipes etc.,
            but keep the previous behaviour for now. */
         if (!ispipe && !S_ISREG(inode->i_mode))
                 goto close_fail;
         /*
          * Dont allow local users get cute and trick others to coredump
          * into their pre-created files:
          */
+	retval = -EPERM;
         if (inode->i_uid != current_fsuid())
                 goto close_fail;
+	retval = -EINVAL;
         if (!file->f_op)
                 goto close_fail;
         if (!file->f_op->write)
                 goto close_fail;
+	retval = -EEXIST;
         if (!ispipe && do_truncate(file->f_path.dentry, 0, 0, file) != 0)
                 goto close_fail;


Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

[Dle-develop] Ry strong, and was more like perfume than anything e

[Rousse <tel...@ma...>]

Om had fallen the task of washing the dishes. "Any chanc

Re: [Dle-develop] [PATCH v2] [RFC] tracepoint: Add signal coredump tracepoint

[KOSAKI M. <kos...@jp...>]

2009/12/3 Masami Hiramatsu <mhi...@re...>:
> Add signal coredump tracepoint which shows signal number,
> mm->flags, limits, pointer to file structure and core
> file name.
>
> This tracepoint requirement comes mainly from the viewpoint of
> administrators. Since now we have introduced many coredump
> configurations (e.g. dumpable, coredump_filter, core_pattern,
> etc) and some of them can be modified by users, it will be hard
> to know what was actually dumped (or not dumped) after some
> problem happened on the system. For example, a process didn't
> generated core, coredump doesn't have some sections, etc.
> In those cases, the coredump tracepoint can help us to know
> why the core file is so big or small, or not generated, by
> recording all configurations for all processes on the system.
> That will reduce system-administration cost.

AFAIK, not-dumped case is important than dump successful case.
IOW, admin need to know why the crashed process was not dumped.

This tracepoint doesn't cover all failure case. especially
binfmt->core_dump() et.al.
IOW, this tracepoint seems too specialized piped-coredump feature.

What do you think this tracepoint's use case?

Re: [Dle-develop] [PATCH v2] [RFC] tracepoint: Add signal coredump tracepoint

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

* Masami Hiramatsu <mhi...@re...> wrote:

> Ingo Molnar wrote:
> > 
> > * Masami Hiramatsu <mhi...@re...> wrote:
> > 
> >> Add signal coredump tracepoint which shows signal number, mm->flags, 
> >> limits, pointer to file structure and core file name.
> > 
> > Why is the kernel pointer to the file structure logged? User-space has 
> > no use for it and the analysis value is low.
> 
> Ah, if open() or opening pipe fails, it becomes 0 or -ERRNO, so we can 
> check if there is an error.

ok, that wasnt obvious from the patch - worth adding it to the 
changelog.

> Perhaps, we can do below in trace_printk for trace users.
> "open %s", (!file || IS_ERR((void *)file)) ? "failed" : "succeeded"

i'd rather suggest to pass an error code (and keep it 0 if none), 
instead of some ad-hoc string message.

But ... the whole issue of VFS event logging and new tracepoints should 
be approached from a more generic direction i think. Do we want to log 
inode_nr:dev pairs as well? Shouldnt there be a generic event-class 
definition via DECLARE_EVENT_CLASS for file related events, with 'core 
dumped' just being a sub-event-code?

I sense reluctance from the direction of Andrew and disinterest from the 
VFS folks - not a good backdrop in general.

	Ingo

Re: [Dle-develop] [PATCH v2] [RFC] tracepoint: Add signal coredump tracepoint

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

Ingo Molnar wrote:
> 
> * Masami Hiramatsu <mhi...@re...> wrote:
> 
>> Add signal coredump tracepoint which shows signal number, mm->flags, 
>> limits, pointer to file structure and core file name.
> 
> Why is the kernel pointer to the file structure logged? User-space has 
> no use for it and the analysis value is low.

Ah, if open() or opening pipe fails, it becomes 0 or -ERRNO,
so we can check if there is an error.

Perhaps, we can do below in trace_printk for trace users.
"open %s", (!file || IS_ERR((void *)file)) ? "failed" : "succeeded"

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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

Re: [Dle-develop] [PATCH v2] [RFC] tracepoint: Add signal coredump tracepoint

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

* Masami Hiramatsu <mhi...@re...> wrote:

> Add signal coredump tracepoint which shows signal number, mm->flags, 
> limits, pointer to file structure and core file name.

Why is the kernel pointer to the file structure logged? User-space has 
no use for it and the analysis value is low.

	Ingo

Re: [Dle-develop] [PATCH -tip 0/9] perf-probe updates

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

Ingo Molnar wrote:
>
> * Masami Hiramatsu<mhi...@re...>  wrote:
>
>> Hi,
>>
>> Here are bugfixes and updates for perf-probe and kprobe-tracer. I've
>> fixed some minor bugs and added --list option and simple probe naming.
>
> Applied, thanks Masami!
>
>> TODO:
>>   - Support build-id checking.
>>   - Support --del option to remove probes.
>>   - Support --line option to show which lines user can probe.
>>   - Support lazy string matching.
>
> ok, cool!
>
> One other small detail i noticed wrt. probe naming. Right now if we
> insert a single probe into a function it gets named schedule_0:
>
>   # perf probe schedule
>   Could not open vmlinux/module file. Try to use symbols.
>   Added new event: p:probe/schedule_0 schedule+0
>
> the next one gets named schedule_1, schedule_2, etc.
>
> It would be nice to special-case the first one and name it 'schedule'.
> Most of the time people insert a single probe into a function, so the _0
> postfix is extra and in most cases unnecessary typing for them.

Sure, that's reasonable.

>
> Another small detail is that i dont think we should emit this line:
>
>   Could not open vmlinux/module file. Try to use symbols.
>
> when we can create a probe successfully - it's just unnecessary noise,
> the user does not care how we pulled it off, as long as we were able to
> get a reliable symbol address and the insertion worked fine.

OK, I'll make it a debug message.

> A third detail is this line:
>
>   Added new event: p:probe/schedule_0 schedule+0
>
> If that is pasted to perf stat directly it wont work because the syntax
> is probe:schedule_0. So i'd suggest to print something like:
>
>   Added new event: probe/schedule_0 (on schedule+0)

Sure, perf always use ':' for event group separator, so

Added new event: probe:schedule (on schedule+0 [with ...(args)])

>
> Perhaps even print another line:
>
>    You can now use it on all perf tools, such as:
>
>      perf probe -e probe/schedule_0 -a sleep 1
>      perf record -e probe/schedule_0 -a sleep 1

OK.

Thank you for good advice!

>
> ... to show people how to make use of it.
>
> Thanks,
>
> 	Ingo

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

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