dle-develop — Dependable Linux Effort mailing list for developers

Re: [Dle-develop] [PATCH -tip 1/3] Pass mm->flags to binfmt core_dump for bitflag consistency

[Andrew M. <ak...@li...>]

On Fri, 13 Nov 2009 17:52:27 -0500
Masami Hiramatsu <mhi...@re...> wrote:

> Pass mm->flags to binfmt core_dump for bitflag consistency.
> Since mm->flags bit flags is not protected by locks, it will be
> changed while dumping core. This patch copies mm->flags to a
> mm_flags local variable at the beginning of do_coredump() and
> use it while dumping. mm_flags also includes dump_filter which
> filters elf sections from core file in elf_core_dump().
> So, this patch also passes mm_flags to each binfmt->core_dump().

I can kind-of guess the answer, but it would be much more reliable if
we were to hear this from yourself:

  Why did you write this patch?  What problem is being observed?

[Dle-develop] [PATCH -tip 1/3] Pass mm->flags to binfmt core_dump for bitflag consistency

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

Pass mm->flags to binfmt core_dump for bitflag consistency.
Since mm->flags bit flags is not protected by locks, it will be
changed while dumping core. This patch copies mm->flags to a
mm_flags local variable at the beginning of do_coredump() and
use it while dumping. mm_flags also includes dump_filter which
filters elf sections from core file in elf_core_dump().
So, this patch also passes mm_flags to each binfmt->core_dump().

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Hidehiro Kawai <hid...@hi...>
Cc: Andrew Morton <ak...@li...>
Cc: Oleg Nesterov <ol...@re...>
Cc: Roland McGrath <ro...@re...>
---

 fs/binfmt_aout.c        |    4 ++--
 fs/binfmt_elf.c         |   12 ++----------
 fs/binfmt_elf_fdpic.c   |   13 +++----------
 fs/binfmt_flat.c        |    4 ++--
 fs/binfmt_som.c         |    2 +-
 fs/exec.c               |   14 ++++++++++----
 include/linux/binfmts.h |    2 +-
 7 files changed, 21 insertions(+), 30 deletions(-)

diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index b639dcf..ef1d4aa 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -32,7 +32,7 @@
 
 static int load_aout_binary(struct linux_binprm *, struct pt_regs * regs);
 static int load_aout_library(struct file*);
-static int aout_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit);
+static int aout_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit, unsigned long mm_flags);
 
 static struct linux_binfmt aout_format = {
 	.module		= THIS_MODULE,
@@ -89,7 +89,7 @@ if (file->f_op->llseek) { \
  * dumping of the process results in another error..
  */
 
-static int aout_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit)
+static int aout_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit, unsigned long mm_flags)
 {
 	mm_segment_t fs;
 	int has_dumped = 0;
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index b9b3bb5..dd7256c 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -45,7 +45,7 @@ static unsigned long elf_map(struct file *, unsigned long, struct elf_phdr *,
  * don't even try.
  */
 #if defined(USE_ELF_CORE_DUMP) && defined(CONFIG_ELF_CORE)
-static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit);
+static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit, unsigned long mm_flags);
 #else
 #define elf_core_dump	NULL
 #endif
@@ -1906,7 +1906,7 @@ static struct vm_area_struct *next_vma(struct vm_area_struct *this_vma,
  * and then they are actually written out.  If we run out of core limit
  * we just truncate.
  */
-static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit)
+static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit, unsigned long mm_flags)
 {
 	int has_dumped = 0;
 	mm_segment_t fs;
@@ -1915,7 +1915,6 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 	struct vm_area_struct *vma, *gate_vma;
 	struct elfhdr *elf = NULL;
 	loff_t offset = 0, dataoff, foffset;
-	unsigned long mm_flags;
 	struct elf_note_info info;
 
 	/*
@@ -1980,13 +1979,6 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 
 	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-	/*
-	 * We must use the same mm->flags while dumping core to avoid
-	 * inconsistency between the program headers and bodies, otherwise an
-	 * unusable core file can be generated.
-	 */
-	mm_flags = current->mm->flags;
-
 	/* Write program headers for segments dump */
 	for (vma = first_vma(current, gate_vma); vma != NULL;
 			vma = next_vma(vma, gate_vma)) {
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 38502c6..8af3633 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -76,7 +76,7 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *,
 					     struct file *, struct mm_struct *);
 
 #if defined(USE_ELF_CORE_DUMP) && defined(CONFIG_ELF_CORE)
-static int elf_fdpic_core_dump(long, struct pt_regs *, struct file *, unsigned long limit);
+static int elf_fdpic_core_dump(long, struct pt_regs *, struct file *, unsigned long limit, unsigned long mm_flags);
 #endif
 
 static struct linux_binfmt elf_fdpic_format = {
@@ -1582,7 +1582,8 @@ static int elf_fdpic_dump_segments(struct file *file, size_t *size,
  * we just truncate.
  */
 static int elf_fdpic_core_dump(long signr, struct pt_regs *regs,
-			       struct file *file, unsigned long limit)
+			       struct file *file, unsigned long limit,
+			       unsigned long mm_flags)
 {
 #define	NUM_NOTES	6
 	int has_dumped = 0;
@@ -1605,7 +1606,6 @@ static int elf_fdpic_core_dump(long signr, struct pt_regs *regs,
 #endif
 	int thread_status_size = 0;
 	elf_addr_t *auxv;
-	unsigned long mm_flags;
 
 	/*
 	 * We no longer stop all VM operations.
@@ -1736,13 +1736,6 @@ static int elf_fdpic_core_dump(long signr, struct pt_regs *regs,
 	/* Page-align dumped data */
 	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-	/*
-	 * We must use the same mm->flags while dumping core to avoid
-	 * inconsistency between the program headers and bodies, otherwise an
-	 * unusable core file can be generated.
-	 */
-	mm_flags = current->mm->flags;
-
 	/* write program headers for segments dump */
 	for (vma = current->mm->mmap; vma; vma = vma->vm_next) {
 		struct elf_phdr phdr;
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index a279665..6477678 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -87,7 +87,7 @@ static int load_flat_shared_library(int id, struct lib_info *p);
 #endif
 
 static int load_flat_binary(struct linux_binprm *, struct pt_regs * regs);
-static int flat_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit);
+static int flat_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit, unsigned long mm_flags);
 
 static struct linux_binfmt flat_format = {
 	.module		= THIS_MODULE,
@@ -102,7 +102,7 @@ static struct linux_binfmt flat_format = {
  * Currently only a stub-function.
  */
 
-static int flat_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit)
+static int flat_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit, unsigned long mm_flags)
 {
 	printk("Process %s:%d received signr %d and should have core dumped\n",
 			current->comm, current->pid, (int) signr);
diff --git a/fs/binfmt_som.c b/fs/binfmt_som.c
index eff74b9..983c01d 100644
--- a/fs/binfmt_som.c
+++ b/fs/binfmt_som.c
@@ -43,7 +43,7 @@ static int load_som_library(struct file *);
  * don't even try.
  */
 #if 0
-static int som_core_dump(long signr, struct pt_regs *regs, unsigned long limit);
+static int som_core_dump(long signr, struct pt_regs *regs, unsigned long limit, unsigned long mm_flags);
 #else
 #define som_core_dump	NULL
 #endif
diff --git a/fs/exec.c b/fs/exec.c
index ba112bd..dc418e3 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1722,7 +1722,7 @@ int get_dumpable(struct mm_struct *mm)
 {
 	int ret;
 
-	ret = mm->flags & 0x3;
+	ret = mm->flags & MMF_DUMPABLE_MASK;
 	return (ret >= 2) ? 2 : ret;
 }
 
@@ -1754,6 +1754,12 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	struct core_state core_state;
 	char corename[CORENAME_MAX_SIZE + 1];
 	struct mm_struct *mm = current->mm;
+	/*
+	 * We must use the same mm->flags while dumping core to avoid
+	 * inconsistency of bit flags, since this flag is not protected
+	 * by any locks.
+	 */
+	unsigned long mm_flags = mm->flags;
 	struct linux_binfmt * binfmt;
 	struct inode * inode;
 	struct file * file;
@@ -1784,7 +1790,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(mm)) {
+	if (mm->core_state || !(mm_flags & MMF_DUMPABLE_MASK)) {
 		up_write(&mm->mmap_sem);
 		put_cred(cred);
 		goto fail;
@@ -1795,7 +1801,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	 *	process nor do we know its entire history. We only know it
 	 *	was tainted so we dump it as root in mode 2.
 	 */
-	if (get_dumpable(mm) == 2) {	/* Setuid core dump mode */
+	if (mm_flags & (1 << MMF_DUMP_SECURELY)) { /* Setuid core dump mode */
 		flag = O_EXCL;		/* Stop rewrite attacks */
 		cred->fsuid = 0;	/* Dump root private */
 	}
@@ -1901,7 +1907,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	if (!ispipe && do_truncate(file->f_path.dentry, 0, 0, file) != 0)
 		goto close_fail;
 
-	retval = binfmt->core_dump(signr, regs, file, core_limit);
+	retval = binfmt->core_dump(signr, regs, file, core_limit, mm_flags);
 
 	if (retval)
 		current->signal->group_exit_code |= 0x80;
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index aece486..6e65c31 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -77,7 +77,7 @@ struct linux_binfmt {
 	struct module *module;
 	int (*load_binary)(struct linux_binprm *, struct  pt_regs * regs);
 	int (*load_shlib)(struct file *);
-	int (*core_dump)(long signr, struct pt_regs *regs, struct file *file, unsigned long limit);
+	int (*core_dump)(long signr, struct pt_regs *regs, struct file *file, unsigned long limit, unsigned long mm_flags);
 	unsigned long min_coredump;	/* minimal dump size */
 	int hasvdso;
 };


-- 
Masami Hiramatsu

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

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

[Dle-develop] [PATCH -tip 3/3] Add get_signal tracepoint

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

Add a tracepoint where a process gets a signal. This tracepoint
shows signal-number, sa-handler and sa-flag.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Roland McGrath <ro...@re...>
---

 include/trace/events/sched.h |   26 ++++++++++++++++++++++++++
 kernel/signal.c              |    3 +++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index ff9d5fb..de8ca7e 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -354,6 +354,32 @@ TRACE_EVENT(sched_process_coredump,
 );
 
 /*
+ * Tracepoint for getting a signal:
+ */
+TRACE_EVENT(sched_process_get_signal,
+
+	TP_PROTO(int sig, struct k_sigaction *ka),
+
+	TP_ARGS(sig, ka),
+
+	TP_STRUCT__entry(
+		__field(	int,		sig		)
+		__field(	unsigned long,	sa_handler	)
+		__field(	unsigned long,	sa_flags	)
+	),
+
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		__entry->sa_handler	= (unsigned long)ka->sa.sa_handler;
+		__entry->sa_flags	= ka->sa.sa_flags;
+	),
+
+	TP_printk("sig: %d sa_handler: %lx sa_flags: %lx",
+		  __entry->sig, __entry->sa_handler, __entry->sa_flags)
+);
+
+/*
  * Tracepoint for sending a signal:
  */
 TRACE_EVENT(sched_signal_send,
diff --git a/kernel/signal.c b/kernel/signal.c
index fe08008..44614fb 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1859,6 +1859,9 @@ relock:
 			ka = &sighand->action[signr-1];
 		}
 
+		/* Trace the actual queued signals including SIG_IGN.*/
+		trace_sched_process_get_signal(signr, ka);
+
 		if (ka->sa.sa_handler == SIG_IGN) /* Do nothing.  */
 			continue;
 		if (ka->sa.sa_handler != SIG_DFL) {


-- 
Masami Hiramatsu

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

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

[Dle-develop] [PATCH -tip 2/3] Add coredump tracepoint

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

Add coredump tracepoint for tracing coredump event. This event shows coredump
caused signal, limit size, dumpable flag, dump-filter, and core name.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Roland McGrath <ro...@re...>
---

 fs/exec.c                    |    9 +++++++++
 include/trace/events/sched.h |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index dc418e3..6b77d10 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -56,6 +56,7 @@
 #include <linux/fsnotify.h>
 #include <linux/fs_struct.h>
 #include <linux/pipe_fs_i.h>
+#include <trace/events/sched.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -1907,6 +1908,14 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	if (!ispipe && do_truncate(file->f_path.dentry, 0, 0, file) != 0)
 		goto close_fail;
 
+	/*
+	 * Trace core dump event.
+	 * Casting signr to int is safe because it comes from
+	 * si->signo int field.
+	 */
+	trace_sched_process_coredump((int)signr, core_limit, mm_flags,
+				     corename);
+
 	retval = binfmt->core_dump(signr, regs, file, core_limit, mm_flags);
 
 	if (retval)
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index b50b985..ff9d5fb 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -320,6 +320,40 @@ TRACE_EVENT(sched_process_fork,
 );
 
 /*
+ * Tracepoint for core_dump:
+ */
+TRACE_EVENT(sched_process_coredump,
+
+	TP_PROTO(int sig, unsigned long core_limit, unsigned long mm_flags,
+		 const char *core_name),
+
+	TP_ARGS(sig, core_limit, mm_flags, core_name),
+
+	TP_STRUCT__entry(
+		__field(	int,		sig		)
+		__field(	unsigned long,	limit		)
+		__field(	unsigned long,	flags		)
+		__string(	name,		core_name	)
+	),
+
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		__entry->limit	= core_limit;
+		__entry->flags	= mm_flags;
+		__assign_str(name,	core_name);
+	),
+
+	TP_printk("sig: %d limit: %lu dumpable: %lx dump_filter: %lx "
+		  "corename: %s",
+		  __entry->sig, __entry->limit,
+		  __entry->flags & MMF_DUMPABLE_MASK,
+		  (__entry->flags & MMF_DUMP_FILTER_MASK) >>
+		  MMF_DUMP_FILTER_SHIFT,
+		  __get_str(name))
+);
+
+/*
  * Tracepoint for sending a signal:
  */
 TRACE_EVENT(sched_signal_send,


-- 
Masami Hiramatsu

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

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

[Dle-develop] [tip:x86/asm] x86: Remove unused thread_return label from switch_to()

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

Commit-ID:  c12a229bc5971534537a7d0e49e44f9f1f5d0336
Gitweb:     http://git.kernel.org/tip/c12a229bc5971534537a7d0e49e44f9f1f5d0336
Author:     Masami Hiramatsu <mhi...@re...>
AuthorDate: Thu, 5 Nov 2009 11:03:59 -0500
Committer:  Ingo Molnar <mi...@el...>
CommitDate: Sun, 8 Nov 2009 11:57:13 +0100

x86: Remove unused thread_return label from switch_to()

Remove unused thread_return label from switch_to() macro on
x86-64. Since this symbol cuts into schedule(), backtrace at the
latter half of schedule() was always shown as thread_return().

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: systemtap <sys...@so...>
Cc: DLE <dle...@li...>
LKML-Reference: <20091105160359.5181.26225.stgit@harusame>
Signed-off-by: Ingo Molnar <mi...@el...>
---
 arch/x86/include/asm/system.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/system.h b/arch/x86/include/asm/system.h
index f08f973..1a953e2 100644
--- a/arch/x86/include/asm/system.h
+++ b/arch/x86/include/asm/system.h
@@ -128,8 +128,6 @@ do {									\
 	     "movq %%rsp,%P[threadrsp](%[prev])\n\t" /* save RSP */	  \
 	     "movq %P[threadrsp](%[next]),%%rsp\n\t" /* restore RSP */	  \
 	     "call __switch_to\n\t"					  \
-	     ".globl thread_return\n"					  \
-	     "thread_return:\n\t"					  \
 	     "movq "__percpu_arg([current_task])",%%rsi\n\t"		  \
 	     __switch_canary						  \
 	     "movq %P[thread_info](%%rsi),%%r8\n\t"			  \

[Dle-develop] [PATCH -tip] [Cleanup] x86/x86-64: Remove unused thread_return label from switch_to()

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

Remove unused thread_return label from switch_to() macro on x86-64.
Since this symbol cuts into schedule(), backtrace at the latter
half of schedule() was always shown as thread_return().

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Ingo Molnar <mi...@el...>
---

 arch/x86/include/asm/system.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/system.h b/arch/x86/include/asm/system.h
index f08f973..1a953e2 100644
--- a/arch/x86/include/asm/system.h
+++ b/arch/x86/include/asm/system.h
@@ -128,8 +128,6 @@ do {									\
 	     "movq %%rsp,%P[threadrsp](%[prev])\n\t" /* save RSP */	  \
 	     "movq %P[threadrsp](%[next]),%%rsp\n\t" /* restore RSP */	  \
 	     "call __switch_to\n\t"					  \
-	     ".globl thread_return\n"					  \
-	     "thread_return:\n\t"					  \
 	     "movq "__percpu_arg([current_task])",%%rsi\n\t"		  \
 	     __switch_canary						  \
 	     "movq %P[thread_info](%%rsi),%%r8\n\t"			  \


-- 
Masami Hiramatsu

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

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

Re: [Dle-develop] [PATCH -tip perf/probes 0/5] perf-probe and kprobe-tracer updates

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

Frederic Weisbecker wrote:
> On Tue, Nov 03, 2009 at 07:12:04PM -0500, Masami Hiramatsu wrote:
>> BTW, I think perf-probe and kprobe-event might better share
>> similar syntax for not confusing users. And for that purpose,
>> perf-probe syntax should introduce event/group specifier,
>> for example,
>
>
> I personally more imagine the debugfs kprobe-event interface as
> something used by higher level applications rather than users.
>
> I've tried to use kprobe events directly by the past to do
> some debugging, and once I wanted to go further a simple function
> probe, like fetching a variable or putting a probe in a given branch,
> it rapidly grew into a pain: I had to read assembly code, guess
> which register was matching which variable, etc... It works, but
> it takes too much time, and printk() rapidly becomes a temptation :)
>
> It too low-level, but its use through perf brings all that to the
> human dimension.
>
> So, I'm not sure we really need to have such tight syntax between
> both, since the debugfs more likely behaves as a gateway, something
> I don't imagine to be used broadly as an end-user interface but mostly
> as a kernel interface.

I see, and I also found that the syntax never be same, since
perf-probe doesn't need argument names etc. kprobe_events
interface may be mostly for higher level scripts or programs.

> Especially we shouldn't break the perf probe syntax simplicity
> just because we want both syntaxes to be tight.

Agreed. OK, so let it be :-)

> (Nothing related to the event/group feature itself, it's just an
> opinion about the need of this similarity between two interfaces).
>
>
>>   perf probe "newgroup:newevnt=func:10 arg1 arg2"
>>
>> adds the newevent under newgroup. On the other hand, ftrace
>> users can also add a new event as below;
>>
>>   echo 'newgroup:newevent=func+0x18 arg1=$a1 arg2=$a2'>  kprobe_events
>>
>> Any thoughts?
>
>
> Yeah, that would probably be nice, especially once we have a good
> collection of probes to handle and to organize in a sensical output.
>
> But it would be better to have that as an optional thing:
>
> 	perf probe "[group:name=]func...."


Sure, of course it should be optional. :-)

> so that we keep the simplicity of:
>
> 	perf probe func
>
> I guess you meant it as optional already, but just in case... :)

Thank you :-)

-- 
Masami Hiramatsu

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

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

Re: [Dle-develop] [PATCH -tip perf/probes 0/5] perf-probe and kprobe-tracer updates

[Frederic W. <fwe...@gm...>]

On Tue, Nov 03, 2009 at 07:12:04PM -0500, Masami Hiramatsu wrote:
> Hi,
> 
> Here are some updates according to previous LKML threads.
>  - Update perf-probe document.
>  - Improve error messages.
>  - Fall back to non-dwarf mode if possible.
>  - Change group name to probe.
>  - Rename kprobe-tracer to kprobe-event.
> 
> BTW, I think perf-probe and kprobe-event might better share
> similar syntax for not confusing users. And for that purpose,
> perf-probe syntax should introduce event/group specifier,
> for example,


I personally more imagine the debugfs kprobe-event interface as
something used by higher level applications rather than users.

I've tried to use kprobe events directly by the past to do
some debugging, and once I wanted to go further a simple function
probe, like fetching a variable or putting a probe in a given branch,
it rapidly grew into a pain: I had to read assembly code, guess
which register was matching which variable, etc... It works, but
it takes too much time, and printk() rapidly becomes a temptation :)

It too low-level, but its use through perf brings all that to the
human dimension.

So, I'm not sure we really need to have such tight syntax between
both, since the debugfs more likely behaves as a gateway, something
I don't imagine to be used broadly as an end-user interface but mostly
as a kernel interface.

Especially we shouldn't break the perf probe syntax simplicity
just because we want both syntaxes to be tight.

(Nothing related to the event/group feature itself, it's just an
opinion about the need of this similarity between two interfaces).


>  perf probe "newgroup:newevnt=func:10 arg1 arg2"
> 
> adds the newevent under newgroup. On the other hand, ftrace
> users can also add a new event as below;
> 
>  echo 'newgroup:newevent=func+0x18 arg1=$a1 arg2=$a2' > kprobe_events
> 
> Any thoughts?


Yeah, that would probably be nice, especially once we have a good
collection of probes to handle and to organize in a sensical output.

But it would be better to have that as an optional thing:

	perf probe "[group:name=]func...."


so that we keep the simplicity of:

	perf probe func

I guess you meant it as optional already, but just in case... :)

Thanks.

[Dle-develop] [PATCH -tip perf/probes 2/5] perf/probes: Improve error messages

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

Improve error messages in perf-probe so that users can figure
out problems easily.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Reported-by: 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...>
---

 tools/perf/builtin-probe.c     |   21 ++++++++++++++-------
 tools/perf/util/probe-finder.c |    3 ++-
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index a99a366..454ddfc 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -294,10 +294,11 @@ static int write_new_event(int fd, const char *buf)
 {
 	int ret;
 
-	printf("Adding new event: %s\n", buf);
 	ret = write(fd, buf, strlen(buf));
 	if (ret <= 0)
-		die("failed to create event.");
+		die("Failed to create event.");
+	else
+		printf("Added new event: %s\n", buf);
 
 	return ret;
 }
@@ -310,7 +311,7 @@ static int synthesize_probe_event(struct probe_point *pp)
 	int i, len, ret;
 	pp->probes[0] = buf = (char *)calloc(MAX_CMDLEN, sizeof(char));
 	if (!buf)
-		die("calloc");
+		die("Failed to allocate memory by calloc.");
 	ret = snprintf(buf, MAX_CMDLEN, "%s+%d", pp->function, pp->offset);
 	if (ret <= 0 || ret >= MAX_CMDLEN)
 		goto error;
@@ -363,7 +364,7 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
 		if (ret == -E2BIG)
 			semantic_error("probe point is too long.");
 		else if (ret < 0)
-			die("snprintf");
+			die("Failed to synthesize a probe point.");
 	}
 
 #ifndef NO_LIBDWARF
@@ -375,7 +376,7 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
 	else
 		fd = open_default_vmlinux();
 	if (fd < 0)
-		die("vmlinux/module file open");
+		die("Could not open vmlinux/module file.");
 
 	/* Searching probe points */
 	for (j = 0; j < session.nr_probe; j++) {
@@ -396,8 +397,14 @@ setup_probes:
 	/* Settng up probe points */
 	snprintf(buf, MAX_CMDLEN, "%s/../kprobe_events", debugfs_path);
 	fd = open(buf, O_WRONLY, O_APPEND);
-	if (fd < 0)
-		die("kprobe_events open");
+	if (fd < 0) {
+		if (errno == ENOENT)
+			die("kprobe_events file does not exist"
+			    " - please rebuild with CONFIG_KPROBE_TRACER.");
+		else
+			die("Could not open kprobe_events file: %s",
+			    strerror(errno));
+	}
 	for (j = 0; j < session.nr_probe; j++) {
 		pp = &session.probes[j];
 		if (pp->found == 1) {
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index db96186..86cead3 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -688,7 +688,8 @@ int find_probepoint(int fd, struct probe_point *pp)
 
 	ret = dwarf_init(fd, DW_DLC_READ, 0, 0, &__dw_debug, &__dw_error);
 	if (ret != DW_DLV_OK)
-		die("Failed to call dwarf_init(). Maybe, not a dwarf file.\n");
+		die("Not found dwarf info in the vmlinux"
+		    " - please rebuild with CONFIG_DEBUG_INFO.\n");
 
 	pp->found = 0;
 	while (++cu_number) {


-- 
Masami Hiramatsu

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

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

[Dle-develop] [PATCH -tip perf/probes 5/5] tracing/kprobes: Rename Kprobe-tracer to kprobe-event

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

Rename Kprobes-based event tracer to kprobes-based tracing event
(kprobe-event), since it is not a tracer but an extensible tracing
event interface.
This also changes CONFIG_KPROBE_TRACER to CONFIG_KPROBE_EVENT
and set y it by default.

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

 Documentation/trace/kprobetrace.txt |   34 ++++++++++++++++------------------
 kernel/trace/Kconfig                |   19 ++++++++++++-------
 kernel/trace/Makefile               |    2 +-
 kernel/trace/trace_kprobe.c         |    6 ++----
 4 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt
index 1541524..bd6a2e2 100644
--- a/Documentation/trace/kprobetrace.txt
+++ b/Documentation/trace/kprobetrace.txt
@@ -1,26 +1,23 @@
-                        Kprobe-based Event Tracer
-                         =========================
+                        Kprobe-based Tracing Event
+                        ==========================
 
                  Documentation is written by Masami Hiramatsu
 
 
 Overview
 --------
-This tracer is similar to the events tracer which is based on Tracepoint
-infrastructure. Instead of Tracepoint, this tracer is based on kprobes(kprobe
-and kretprobe). It probes anywhere where kprobes can probe(this means, all
-functions body except for __kprobes functions).
+This event is similar to the tracepoint based event. Instead of Tracepoint,
+this is based on kprobes (kprobe and kretprobe). So it can probe wherever
+kprobes can probe (this means, all functions body except for __kprobes
+functions). Unlike the Tracepoint based event, this can be added and removed
+on the fly.
 
-Unlike the function tracer, this tracer can probe instructions inside of
-kernel functions. It allows you to check which instruction has been executed.
+For enabling this feature, build your kernel with CONFIG_KPROBE_TRACING=y.
 
-Unlike the Tracepoint based events tracer, this tracer can add and remove
-probe points on the fly.
-
-Similar to the events tracer, this tracer doesn't need to be activated via
-current_tracer, instead of that, just set probe points via
-/sys/kernel/debug/tracing/kprobe_events. And you can set filters on each
-probe events via /sys/kernel/debug/tracing/events/kprobes/<EVENT>/filter.
+Similar to the events tracer, this doesn't need to be activated via
+current_tracer. Instead of that, add probe points via
+/sys/kernel/debug/tracing/kprobe_events, and enable it via
+/sys/kernel/debug/tracing/events/kprobes/<EVENT>/enabled.
 
 
 Synopsis of kprobe_events
@@ -55,9 +52,9 @@ Per-Probe Event Filtering
 -------------------------
  Per-probe event filtering feature allows you to set different filter on each
 probe and gives you what arguments will be shown in trace buffer. If an event
-name is specified right after 'p:' or 'r:' in kprobe_events, the tracer adds
-an event under tracing/events/kprobes/<EVENT>, at the directory you can see
-'id', 'enabled', 'format' and 'filter'.
+name is specified right after 'p:' or 'r:' in kprobe_events, it adds an event
+under tracing/events/kprobes/<EVENT>, at the directory you can see 'id',
+'enabled', 'format' and 'filter'.
 
 enabled:
   You can enable/disable the probe by writing 1 or 0 on it.
@@ -71,6 +68,7 @@ filter:
 id:
   This shows the id of this probe event.
 
+
 Event Profiling
 ---------------
  You can check the total number of probe hits and probe miss-hits via
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 15372a9..d2ad714 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -428,17 +428,22 @@ config BLK_DEV_IO_TRACE
 
 	  If unsure, say N.
 
-config KPROBE_TRACER
+config KPROBE_EVENT
 	depends on KPROBES
 	depends on X86
-	bool "Trace kprobes"
+	bool "Enable kprobes-based dynamic events"
 	select TRACING
-	select GENERIC_TRACER
+	default y
 	help
-	  This tracer probes everywhere where kprobes can probe it, and
-	  records various registers and memories specified by user.
-	  This also allows you to trace kprobe probe points as a dynamic
-	  defined events. It provides per-probe event filtering interface.
+	  This allows user to add tracing events (like tracepoint) on the fly
+	  via the ftrace interface. See Documentation/trace/kprobetrace.txt
+	  for more details.
+
+	  Those events can probe wherever kprobes can probe, and record
+	  various registers and memories.
+
+	  This option is required by perf-probe subcommand of perf tools. If
+	  you want to use perf tools, this option is strongly recommended.
 
 config DYNAMIC_FTRACE
 	bool "enable/disable ftrace tracepoints dynamically"
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index c8cb75d..edc3a3c 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -53,7 +53,7 @@ obj-$(CONFIG_EVENT_TRACING) += trace_export.o
 obj-$(CONFIG_FTRACE_SYSCALLS) += trace_syscalls.o
 obj-$(CONFIG_EVENT_PROFILE) += trace_event_profile.o
 obj-$(CONFIG_EVENT_TRACING) += trace_events_filter.o
-obj-$(CONFIG_KPROBE_TRACER) += trace_kprobe.o
+obj-$(CONFIG_KPROBE_EVENT) += trace_kprobe.o
 obj-$(CONFIG_EVENT_TRACING) += power-traces.o
 
 libftrace-y := ftrace.o
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index a86c3ac..cf17a66 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1,5 +1,5 @@
 /*
- * kprobe based kernel tracer
+ * Kprobes-based tracing events
  *
  * Created by Masami Hiramatsu <mhi...@re...>
  *
@@ -57,8 +57,6 @@ const char *reserved_field_names[] = {
 	FIELD_STRING_FUNC,
 };
 
-/* currently, trace_kprobe only supports X86. */
-
 struct fetch_func {
 	unsigned long (*func)(struct pt_regs *, void *);
 	void *data;
@@ -191,7 +189,7 @@ static __kprobes void free_indirect_fetch_data(struct indirect_fetch_data *data)
 }
 
 /**
- * Kprobe tracer core functions
+ * Kprobe event core functions
  */
 
 struct probe_arg {


-- 
Masami Hiramatsu

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

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

[Dle-develop] [PATCH -tip perf/probes 1/5] perf/probe: Update Documentation/perf-probe.txt

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

Update Documentation/perf-probe.txt accoding to syntax change.

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

 tools/perf/Documentation/perf-probe.txt |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/perf/Documentation/perf-probe.txt b/tools/perf/Documentation/perf-probe.txt
index 6b6c6ae..c47e54e 100644
--- a/tools/perf/Documentation/perf-probe.txt
+++ b/tools/perf/Documentation/perf-probe.txt
@@ -8,7 +8,9 @@ perf-probe - Define new dynamic tracepoints
 SYNOPSIS
 --------
 [verse]
-'perf probe' [-k <file>] -P 'PROBE' [-P 'PROBE' ...]
+'perf probe' [options] --add 'PROBE' [--add 'PROBE' ...]
+or
+'perf probe' [options] 'PROBE' ['PROBE' ...]
 
 
 DESCRIPTION
@@ -21,26 +23,25 @@ and C local variables) with debuginfo.
 OPTIONS
 -------
 -k::
---vmlinux::
+--vmlinux=PATH::
 	Specify vmlinux path which has debuginfo (Dwarf binary).
 
 -v::
 --verbose::
         Be more verbose (show parsed arguments, etc).
 
--P::
---probe::
+-a::
+--add::
 	Define a probe point (see PROBE SYNTAX for detail)
 
 PROBE SYNTAX
 ------------
 Probe points are defined by following syntax.
 
- "TYPE:[GRP/]NAME FUNC[+OFFS][@SRC]|@SRC:LINE [ARG ...]"
+ "FUNC[+OFFS|:RLN|%return][@SRC]|SRC:ALN [ARG ...]"
 
-'TYPE' specifies the type of probe point, you can use either "p" (kprobe) or "r" (kretprobe) for 'TYPE'. 'GRP' specifies the group name of this probe, and 'NAME' specifies the event name. If 'GRP' is omitted, "kprobes" is used for its group name.
-'FUNC' and 'OFFS' specifies function and offset (in byte) where probe will be put. In addition, 'SRC' specifies a source file which has that function (this is mainly for inline functions).
-You can specify a probe point by the source line number by using '@SRC:LINE' syntax, where 'SRC' is the source file path and 'LINE' is the line number.
+'FUNC' specifies a probed function name, and it may have one of following options; '+OFFS' is the offset from function entry address in byte, 'RLN' is the relative-line number from function entry line, and '%return' means that it probes function return. In addition, 'SRC' specifies a source file which has that function.
+It is also possible to specify a probe point by the source line number by using 'SRC:ALN' syntax, where 'SRC' is the source file path and 'ALN' is the line number.
 'ARG' specifies the arguments of this probe point. You can use the name of local variable, or kprobe-tracer argument format (e.g. $retval, %ax, etc).
 
 SEE ALSO


-- 
Masami Hiramatsu

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

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

[Dle-develop] [PATCH -tip perf/probes 0/5] perf-probe and kprobe-tracer updates

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

Hi,

Here are some updates according to previous LKML threads.
 - Update perf-probe document.
 - Improve error messages.
 - Fall back to non-dwarf mode if possible.
 - Change group name to probe.
 - Rename kprobe-tracer to kprobe-event.

BTW, I think perf-probe and kprobe-event might better share
similar syntax for not confusing users. And for that purpose,
perf-probe syntax should introduce event/group specifier,
for example,

 perf probe "newgroup:newevnt=func:10 arg1 arg2"

adds the newevent under newgroup. On the other hand, ftrace
users can also add a new event as below;

 echo 'newgroup:newevent=func+0x18 arg1=$a1 arg2=$a2' > kprobe_events

Any thoughts?

TODO:
 - Support --list option to show probes.
 - Support --del option to remove probes.
 - Simplify probe names.
 - Support --line option to show which lines user can probe.
 - Support lazy string matching.

Thank you,

---

Masami Hiramatsu (5):
      tracing/kprobes: Rename Kprobe-tracer to kprobe-event
      perf/probes: Rename perf probe events group name
      perf/probes: Fall back to non-dwarf if possible
      perf/probes: Improve error messages
      perf/probe: Update Documentation/perf-probe.txt


 Documentation/trace/kprobetrace.txt     |   34 ++++++-------
 kernel/trace/Kconfig                    |   19 ++++---
 kernel/trace/Makefile                   |    2 -
 kernel/trace/trace_kprobe.c             |    6 +-
 tools/perf/Documentation/perf-probe.txt |   17 +++---
 tools/perf/builtin-probe.c              |   84 ++++++++++++++++++-------------
 tools/perf/util/probe-finder.c          |    7 ++-
 7 files changed, 95 insertions(+), 74 deletions(-)

-- 
Masami Hiramatsu

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

[Dle-develop] [PATCH -tip perf/probes 4/5] perf/probes: Rename perf probe events group name

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

Rename the group name of perf probe events to 'probe'.

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

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

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index e44491b..73dd7d1 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -52,7 +52,7 @@ const char *default_search_path[NR_SEARCH_PATH] = {
 #define MAX_PATH_LEN 256
 #define MAX_PROBES 128
 #define MAX_PROBE_ARGS 128
-#define PERFPROBE_GROUP "perfprobe"
+#define PERFPROBE_GROUP "probe"
 
 /* Session management structure */
 static struct {


-- 
Masami Hiramatsu

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

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

[Dle-develop] [PATCH -tip perf/probes 3/5] perf/probes: Fall back to non-dwarf if possible

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

Fall back to non-dwarf probe point if the probe definition may not
need dwarf analysis, when perf can't find vmlinux/debuginfo.
This might skip some inlined code of target function.

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

 tools/perf/builtin-probe.c     |   65 +++++++++++++++++++++++-----------------
 tools/perf/util/probe-finder.c |    8 +++--
 2 files changed, 42 insertions(+), 31 deletions(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 454ddfc..e44491b 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -189,7 +189,7 @@ static void parse_probe_event(const char *str)
 	/* Parse probe point */
 	parse_probe_point(argv[0], pp);
 	free(argv[0]);
-	if (pp->file)
+	if (pp->file || pp->line)
 		session.need_dwarf = 1;
 
 	/* Copy arguments */
@@ -347,36 +347,24 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
 	if (session.nr_probe == 0)
 		usage_with_options(probe_usage, options);
 
-#ifdef NO_LIBDWARF
 	if (session.need_dwarf)
-		semantic_error("Dwarf-analysis is not supported");
-#endif
-
-	/* Synthesize probes without dwarf */
-	for (j = 0; j < session.nr_probe; j++) {
-#ifndef NO_LIBDWARF
-		if (!session.probes[j].retprobe) {
-			session.need_dwarf = 1;
-			continue;
-		}
-#endif
-		ret = synthesize_probe_event(&session.probes[j]);
-		if (ret == -E2BIG)
-			semantic_error("probe point is too long.");
-		else if (ret < 0)
-			die("Failed to synthesize a probe point.");
-	}
-
-#ifndef NO_LIBDWARF
-	if (!session.need_dwarf)
-		goto setup_probes;
+#ifdef NO_LIBDWARF
+		semantic_error("Debuginfo-analysis is not supported");
+#else	/* !NO_LIBDWARF */
+		pr_info("Some probes require debuginfo.\n");
 
 	if (session.vmlinux)
 		fd = open(session.vmlinux, O_RDONLY);
 	else
 		fd = open_default_vmlinux();
-	if (fd < 0)
-		die("Could not open vmlinux/module file.");
+	if (fd < 0) {
+		if (session.need_dwarf)
+			die("Could not open vmlinux/module file.");
+
+		pr_warning("Could not open vmlinux/module file."
+			   " Try to use symbols.\n");
+		goto end_dwarf;
+	}
 
 	/* Searching probe points */
 	for (j = 0; j < session.nr_probe; j++) {
@@ -386,14 +374,35 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
 
 		lseek(fd, SEEK_SET, 0);
 		ret = find_probepoint(fd, pp);
-		if (ret <= 0)
-			die("No probe point found.\n");
+		if (ret < 0) {
+			if (session.need_dwarf)
+				die("Could not analyze debuginfo.");
+
+			pr_warning("An error was occurred in debuginfo"
+				   " analysis. Try to use symbols.\n");
+			break;
+		}
+		if (ret == 0)	/* No error but failed to find probe point. */
+			die("No probe point found.");
 	}
 	close(fd);
 
-setup_probes:
+end_dwarf:
 #endif /* !NO_LIBDWARF */
 
+	/* Synthesize probes without dwarf */
+	for (j = 0; j < session.nr_probe; j++) {
+		pp = &session.probes[j];
+		if (pp->found)	/* This probe is already found. */
+			continue;
+
+		ret = synthesize_probe_event(pp);
+		if (ret == -E2BIG)
+			semantic_error("probe point is too long.");
+		else if (ret < 0)
+			die("Failed to synthesize a probe point.");
+	}
+
 	/* Settng up probe points */
 	snprintf(buf, MAX_CMDLEN, "%s/../kprobe_events", debugfs_path);
 	fd = open(buf, O_WRONLY, O_APPEND);
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 86cead3..f628dee 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -687,9 +687,11 @@ int find_probepoint(int fd, struct probe_point *pp)
 	struct probe_finder pf = {.pp = pp};
 
 	ret = dwarf_init(fd, DW_DLC_READ, 0, 0, &__dw_debug, &__dw_error);
-	if (ret != DW_DLV_OK)
-		die("Not found dwarf info in the vmlinux"
-		    " - please rebuild with CONFIG_DEBUG_INFO.\n");
+	if (ret != DW_DLV_OK) {
+		pr_warning("Not found dwarf info in the vmlinux"
+			   " - please rebuild with CONFIG_DEBUG_INFO.\n");
+		return -ENOENT;
+	}
 
 	pp->found = 0;
 	while (++cu_number) {


-- 
Masami Hiramatsu

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

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

Re: [Dle-develop] [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes and perf-probe syntax changes

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

Ingo Molnar wrote:
> * Masami Hiramatsu<mhi...@re...>  wrote:
>> Masami Hiramatsu wrote:
>>> Masami Hiramatsu wrote:
>>>> Ingo Molnar wrote:
>>>>> What we want here is two fold:
>>>>>
>>>>>      - enable kprobes event support when perf events is enabled and kprobes
>>>>>        is enabled. We dont want another config option for it.
>>>>
>>>> Sure, at least that combination should enable kprobe-tracer forcibly.
>>>
>>> Hmm, someone may not want to enables kprobe-tracer. Perhaps,
>>> "default y if (EVENT_PROFILE)" is enough, isn't it?
>>
>> Oops, this causes recursive dependency error :-(
>>
>> kernel/trace/Kconfig:90:error: found recursive dependency: TRACING ->
>> EVENT_TRACING ->  EVENT_PROFILE ->  KPROBE_TRACER ->  GENERIC_TRACER ->  TRACING
>
> This dependency problem can be resolved by simply making it 'default y'
> - the option itself depends on KPROBES already, which is default-off -
> so no need to also make it depend on EVENT_PROFILE.

OK,

> btw., it would be nice to re-name it to 'KPROBE_EVENTS'. If the probe
> point is used as a count - like in the __switch_to example i cited -
> there's no tracing going on at all.

Sure, it's not a tracer anyway :-)

Thank you,

-- 
Masami Hiramatsu

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

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

[Dle-develop] Using build-ids in perf tools was Re: [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes

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

Em Tue, Nov 03, 2009 at 08:24:39AM +0100, Ingo Molnar escreveu:
> * Roland McGrath <ro...@re...> wrote:
> > As Frank mentioned, the kernel's and modules' allocated ELF notes (and 
> > thus build IDs) are already exposed in /sys.  Tools like "eu-unstrip 
> > -nk" use this information today.
> 
> Ah, i didnt realize we link with --build-id already, unconditonally, 
> since v2.6.23 (if ld supports it):
> 
>  | From 18991197b4b588255ccabf472ebc84db7b66a19c Mon Sep 17 00:00:00 2001
>  | Subject: [PATCH] Use --build-id ld option
> 
> So we have an SHA1 build-id already on the vmlinux and on modules, and 
> it's exposed in /sys/*/*/notes. Just have to make use of it in 
> tools/perf too.

I wasn't aware this was done upstream, ass-umed that it was only on
kernel specfiles, will cook up a patch for consideration.

- Arnaldo

Re: [Dle-develop] [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes and perf-probe syntax changes

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

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

> Masami Hiramatsu wrote:
>> Masami Hiramatsu wrote:
>>> Ingo Molnar wrote:
>>>> What we want here is two fold:
>>>>
>>>>     - enable kprobes event support when perf events is enabled and kprobes
>>>>       is enabled. We dont want another config option for it.
>>>
>>> Sure, at least that combination should enable kprobe-tracer forcibly.
>>
>> Hmm, someone may not want to enables kprobe-tracer. Perhaps,
>> "default y if (EVENT_PROFILE)" is enough, isn't it?
>
> Oops, this causes recursive dependency error :-(
>
> kernel/trace/Kconfig:90:error: found recursive dependency: TRACING ->
> EVENT_TRACING -> EVENT_PROFILE -> KPROBE_TRACER -> GENERIC_TRACER -> TRACING

This dependency problem can be resolved by simply making it 'default y' 
- the option itself depends on KPROBES already, which is default-off - 
so no need to also make it depend on EVENT_PROFILE.

btw., it would be nice to re-name it to 'KPROBE_EVENTS'. If the probe 
point is used as a count - like in the __switch_to example i cited - 
there's no tracing going on at all.

	Ingo

Re: [Dle-develop] [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes

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

* Roland McGrath <ro...@re...> wrote:

> > Thirdly, i think we should expose the build-id of the kernel and the 
> > path to the vmlinux (and modules) via /proc/build-id or so. That way 
> > tooling can find the vmlinux file and can validate that it matches 
> > the kernel's signature. (maybe include a file date as well - that's 
> > a faster check than a full build-id checksum, especially for large 
> > kernels)
> 
> You seem to be confused about what build IDs are.  There is no 
> "checksum validation".  Once the bits are stored there is no 
> calculation ever done again, only exact comparison with an expected 
> build ID bitstring.  The size of an object file is immaterial.
> 
> As Frank mentioned, the kernel's and modules' allocated ELF notes (and 
> thus build IDs) are already exposed in /sys.  Tools like "eu-unstrip 
> -nk" use this information today.

Ah, i didnt realize we link with --build-id already, unconditonally, 
since v2.6.23 (if ld supports it):

 |
 | From 18991197b4b588255ccabf472ebc84db7b66a19c Mon Sep 17 00:00:00 2001
 | From: Roland McGrath <ro...@re...>
 | Date: Thu, 19 Jul 2007 01:48:40 -0700
 | Subject: [PATCH] Use --build-id ld option
 |
 | This change passes the --build-id when linking the kernel and when 
 | linking modules, if ld supports it.  This is a new GNU ld option that 
 | synthesizes an ELF note section inside the read-only data.  The note in 
 | this section contains unique identifying bits called the "build ID", 
 | which are generated so as to be different for any two linked ELF files 
 | that aren't identical.
 |

So we have an SHA1 build-id already on the vmlinux and on modules, and 
it's exposed in /sys/*/*/notes. Just have to make use of it in 
tools/perf too.

The other useful addition i mentioned isnt implemented yet: to emit an 
ELF note of the absolute path of the output directory the kernel was 
built in as well. This information is not available right now, and it 
would be a place to look in to search for the vmlinux and the modules.

What do you think? Also, if we do this, is there a standard way to name 
it , or should i just pick a suitably new, Linux-specific name for that?

Thanks,

	Ingo

Re: [Dle-develop] [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes and perf-probe syntax changes

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

Masami Hiramatsu wrote:
> Masami Hiramatsu wrote:
>> Ingo Molnar wrote:
>>> What we want here is two fold:
>>>
>>>     - enable kprobes event support when perf events is enabled and kprobes
>>>       is enabled. We dont want another config option for it.
>>
>> Sure, at least that combination should enable kprobe-tracer forcibly.
>
> Hmm, someone may not want to enables kprobe-tracer. Perhaps,
> "default y if (EVENT_PROFILE)" is enough, isn't it?

Oops, this causes recursive dependency error :-(

kernel/trace/Kconfig:90:error: found recursive dependency: TRACING ->
EVENT_TRACING -> EVENT_PROFILE -> KPROBE_TRACER -> GENERIC_TRACER -> TRACING

At this time, this kind of weak dependency may not supported by Kbuild yet.

Thank you,

-- 
Masami Hiramatsu

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

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

Re: [Dle-develop] [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes and perf-probe syntax changes

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

Frederic Weisbecker wrote:
> On Mon, Nov 02, 2009 at 04:16:25PM -0500, Masami Hiramatsu wrote:
>> Masami Hiramatsu wrote:
>>> Ingo Molnar wrote:
>>>> What we want here is two fold:
>>>>
>>>>     - enable kprobes event support when perf events is enabled and kprobes
>>>>       is enabled. We dont want another config option for it.
>>>
>>> Sure, at least that combination should enable kprobe-tracer forcibly.
>>
>> Hmm, someone may not want to enables kprobe-tracer. Perhaps,
>> "default y if (EVENT_PROFILE)" is enough, isn't it?
>>
>
>
> I guess it should be sufficient yeah. We want to strongly recommend
> the kprobe events if we have enabled perf, but we don't want to force
> it.
>
> Also in this case we need a verbose runtime report of the lack of this
> tracer in debugfs from perf probe if needed.

Sure, error message should be changed as warning user to enable
CONFIG_KPROBE_TRACER :-)

Thank you,

-- 
Masami Hiramatsu

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

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

Re: [Dle-develop] [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes and perf-probe syntax changes

[Frederic W. <fwe...@gm...>]

On Mon, Nov 02, 2009 at 04:16:25PM -0500, Masami Hiramatsu wrote:
> Masami Hiramatsu wrote:
>> Ingo Molnar wrote:
>>> What we want here is two fold:
>>>
>>>    - enable kprobes event support when perf events is enabled and kprobes
>>>      is enabled. We dont want another config option for it.
>>
>> Sure, at least that combination should enable kprobe-tracer forcibly.
>
> Hmm, someone may not want to enables kprobe-tracer. Perhaps,
> "default y if (EVENT_PROFILE)" is enough, isn't it?
>


I guess it should be sufficient yeah. We want to strongly recommend
the kprobe events if we have enabled perf, but we don't want to force
it.

Also in this case we need a verbose runtime report of the lack of this
tracer in debugfs from perf probe if needed.

Re: [Dle-develop] [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes and perf-probe syntax changes

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

Masami Hiramatsu wrote:
> Ingo Molnar wrote:
>> What we want here is two fold:
>>
>>    - enable kprobes event support when perf events is enabled and kprobes
>>      is enabled. We dont want another config option for it.
>
> Sure, at least that combination should enable kprobe-tracer forcibly.

Hmm, someone may not want to enables kprobe-tracer. Perhaps,
"default y if (EVENT_PROFILE)" is enough, isn't it?


>> A few further (and very small) UI tweaks i'd suggest:
>>
>> Firstly, could we please make the first probe inserted named plain after
>> the symbol it specifies, with no _0 postfix? I.e. instead of:
>>
>>              7358  perfprobe:__switch_to_0  #      0.000 M/sec
>>
>> we'd get:
>>
>>              7358  perfprobe:__switch_to    #      0.000 M/sec
>>
>> Subsequent probes for the same symbol can be named _1, _2 - but the
>> first symbol should not have this needless post-fix.
>
> Ah, this prefix means the offset from the symbol. Of course we can
> remove it if the offset == 0. Or, would you think make the postfix
> sequence number of probes on the same symbol?

If so, we'd better have --list option before that and check the
postfix is already used, since we may not want to overwrite
existing probes, right?

Thank you,

-- 
Masami Hiramatsu

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

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

Re: [Dle-develop] [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes

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

Josh Stone wrote:
> On 10/29/2009 01:10 PM, Masami Hiramatsu wrote:
>> Masami Hiramatsu wrote:
>>> Ingo Molnar wrote:
>>>> Another problem i noticed is that a vmlinux without DEBUG_INFO will fail
>>>> in this way:
>>>>
>>>>     aldebaran:~/linux/linux>    perf probe schedule
>>>>       Fatal: Failed to call dwarf_init(). Maybe, not a dwarf file.
>>>
>>> Ah, really? I think I broke need_dwarf logic somehow...
>>
>> Hmm, I've found that is for searching (implicitly) inlined symbols,
>> this means "the behavior is by (bad) design" :-(
>>
>> I think it should be search the symbol in Elf (or kallsyms) first,
>> and only if it fails, use Dwarf for searching the symbol again.
>>
>> Or, it may be enough that just trying to setup probe and if it fails
>> use Dwarf. This way doesn't require any vmlinux access.
>
> Just beware that functions can exist in the symbol table and as inlines
> at the same time.  For example, we've seen compat_sys_recvmsg get
> inlined into compat_sys_socketcall, while it's still compiled as a
> standalone function too.  So if you have the dwarf, you should still try
> to see if inlined versions exist.

Right, by default, perf-probe should see Dwarf since some static functions
may implicitly compiled as inline, and it is hard to request user checking
whether the symbol is inlined or not.

So, this means current design is not so bad, but practically, we'd better
provide an option which ignores inline functions. e.g. exported functions
will be always not-inlined.

Thank you,

-- 
Masami Hiramatsu

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

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

Re: [Dle-develop] [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes

[Josh S. <ji...@re...>]

On 10/29/2009 01:10 PM, Masami Hiramatsu wrote:
> Masami Hiramatsu wrote:
>> Ingo Molnar wrote:
>>> Another problem i noticed is that a vmlinux without DEBUG_INFO will fail
>>> in this way:
>>>
>>>    aldebaran:~/linux/linux>   perf probe schedule
>>>      Fatal: Failed to call dwarf_init(). Maybe, not a dwarf file.
>>
>> Ah, really? I think I broke need_dwarf logic somehow...
> 
> Hmm, I've found that is for searching (implicitly) inlined symbols,
> this means "the behavior is by (bad) design" :-(
> 
> I think it should be search the symbol in Elf (or kallsyms) first,
> and only if it fails, use Dwarf for searching the symbol again.
> 
> Or, it may be enough that just trying to setup probe and if it fails
> use Dwarf. This way doesn't require any vmlinux access.

Just beware that functions can exist in the symbol table and as inlines
at the same time.  For example, we've seen compat_sys_recvmsg get
inlined into compat_sys_socketcall, while it's still compiled as a
standalone function too.  So if you have the dwarf, you should still try
to see if inlined versions exist.

Josh

Re: [Dle-develop] [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes

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

Masami Hiramatsu wrote:
> Ingo Molnar wrote:
>> Another problem i noticed is that a vmlinux without DEBUG_INFO will fail
>> in this way:
>>
>>    aldebaran:~/linux/linux>   perf probe schedule
>>      Fatal: Failed to call dwarf_init(). Maybe, not a dwarf file.
>
> Ah, really? I think I broke need_dwarf logic somehow...

Hmm, I've found that is for searching (implicitly) inlined symbols,
this means "the behavior is by (bad) design" :-(

I think it should be search the symbol in Elf (or kallsyms) first,
and only if it fails, use Dwarf for searching the symbol again.

Or, it may be enough that just trying to setup probe and if it fails
use Dwarf. This way doesn't require any vmlinux access.

Thank you,

-- 
Masami Hiramatsu

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

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