Re: [PATCH 1/2] tracing/kprobes: move free_trace_probe into unregister_trace_probe
On Tue, Jun 25, 2013 at 9:34 PM, Steven Rostedt wrote: > On Tue, 2013-06-25 at 18:37 +0800, zhangwei(Jovi) wrote: >> On 2013/6/25 18:10, Masami Hiramatsu wrote: >> > (2013/06/25 17:15), zhangwei(Jovi) wrote: >> >> There have no good reason to call free_trace_probe >> >> every time when unregister_trace_probe return 0. >> >> >> >> Move free_trace_probe into unregister_trace_probe, >> >> make code simpler. >> > >> > Sorry, nack. For the symmetrical coding reason, I don't like >> > involving "free" and "alloc" into "unregister"/"register" >> > functions. I think those should be just another actions. >> > >> > Thank you, >> >> That's fine, I just saw there have a little inconsistent between >> trace_kprobe.c and trace_uprobe.c. >> > > Is there a place that trace_kprobe.c frees the tp structure in > unregister? > I won't argue put the free operation into unregister function in trace_kprobe.c, as I said, one minor problem is the code pattern between trace_kprobe.c and trace_uprobe.c is so similar on unregister, but with little inconsistent, maybe we can unify those code in trace_probe.c someday. Anyway, this is not big functionality issue, free to leave in there if authors don't argue. jovi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] tracing/kprobes: move free_trace_probe into unregister_trace_probe
On Tue, Jun 25, 2013 at 9:34 PM, Steven Rostedt rost...@goodmis.org wrote: On Tue, 2013-06-25 at 18:37 +0800, zhangwei(Jovi) wrote: On 2013/6/25 18:10, Masami Hiramatsu wrote: (2013/06/25 17:15), zhangwei(Jovi) wrote: There have no good reason to call free_trace_probe every time when unregister_trace_probe return 0. Move free_trace_probe into unregister_trace_probe, make code simpler. Sorry, nack. For the symmetrical coding reason, I don't like involving free and alloc into unregister/register functions. I think those should be just another actions. Thank you, That's fine, I just saw there have a little inconsistent between trace_kprobe.c and trace_uprobe.c. Is there a place that trace_kprobe.c frees the tp structure in unregister? I won't argue put the free operation into unregister function in trace_kprobe.c, as I said, one minor problem is the code pattern between trace_kprobe.c and trace_uprobe.c is so similar on unregister, but with little inconsistent, maybe we can unify those code in trace_probe.c someday. Anyway, this is not big functionality issue, free to leave in there if authors don't argue. jovi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 03/11] tracing: add soft disable for syscall events
On Sat, Jun 22, 2013 at 4:22 AM, Steven Rostedt wrote:
> On Fri, 2013-06-21 at 14:53 +0800, zhangwei(Jovi) wrote:
>> On 2013/6/21 2:31, Tom Zanussi wrote:
>> > Add support for SOFT_DISABLE to syscall events.
>> >
>> > The original SOFT_DISABLE patches didn't add support for soft disable
>> > of syscall events; this adds it and paves the way for future patches
>> > allowing triggers to be added to syscall events, since triggers are
>> > built on top of SOFT_DISABLE.
>> >
>> > Because the trigger and SOFT_DISABLE bits are attached to the
>> > ftrace_event_file associated with the event, pointers to the
>> > ftrace_event_files associated with the event are added to the syscall
>> > metadata entry for the event.
>> >
>> > Signed-off-by: Tom Zanussi
>> > ---
>> > include/linux/syscalls.h | 2 ++
>> > include/trace/syscall.h | 5 +
>> > kernel/trace/trace_syscalls.c | 28
>> > 3 files changed, 31 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> > index 4147d70..b4c2afa 100644
>> > --- a/include/linux/syscalls.h
>> > +++ b/include/linux/syscalls.h
>> > @@ -158,6 +158,8 @@ extern struct trace_event_functions
>> > exit_syscall_print_funcs;
>> > .args = nb ? args_##sname : NULL, \
>> > .enter_event= _enter_##sname, \
>> > .exit_event = _exit_##sname, \
>> > + .enter_file = NULL, /* Filled in at boot */ \
>> > + .exit_file = NULL, /* Filled in at boot */ \
>> > .enter_fields =
>> > LIST_HEAD_INIT(__syscall_meta_##sname.enter_fields), \
>> > }; \
>> > static struct syscall_metadata __used \
>> > diff --git a/include/trace/syscall.h b/include/trace/syscall.h
>> > index fed853f..ba24d3a 100644
>> > --- a/include/trace/syscall.h
>> > +++ b/include/trace/syscall.h
>> > @@ -19,6 +19,8 @@
>> > * @enter_fields: list of fields for syscall_enter trace event
>> > * @enter_event: associated syscall_enter trace event
>> > * @exit_event: associated syscall_exit trace event
>> > + * @enter_file: associated syscall_enter ftrace event file
>> > + * @exit_file: associated syscall_exit ftrace event file
>> > */
>> > struct syscall_metadata {
>> > const char *name;
>> > @@ -30,6 +32,9 @@ struct syscall_metadata {
>> >
>> > struct ftrace_event_call *enter_event;
>> > struct ftrace_event_call *exit_event;
>> > +
>> > + struct ftrace_event_file *enter_file;
>> > + struct ftrace_event_file *exit_file;
>> > };
>>
>> I doubt this could work correctly.
>> struct ftrace_event_file is allocated dynamically, there could
>> have many ftrace_event_file in there, associated with different trace_array,
>> it means there may have many ftrace_event_file linked with same
>> syscall_metadata.
>>
>> The enter_file/exit_file pointer of syscall_metadata will be override if
>> another
>> ftrace_event_file registered in some other trace_array.
>>
>> Perhaps we could use simple list_head in here, which link with all
>> registered ftrace_event_file.
>>
>> Oleg use "struct event_file_link" for trace_kprobe, the structure could be
>> reuse in here.
>>
>>
>> struct event_file_link {
>> struct ftrace_event_file*file;
>> struct list_headlist;
>> };
>>
>> struct syscall_metadata {
>> const char *name;
>> int syscall_nr;
>> int nb_args;
>> const char **types;
>> const char **args;
>> struct list_head enter_fields;
>>
>> struct ftrace_event_call *enter_event;
>> struct ftrace_event_call *exit_event;
>>
>> struct list_headenter_files;
>> struct list_headexit_files;
>> };
>>
>
>
> I would really like to make these smaller. They are made for every
> system call, and I consider tracing to always be added overhead. I hate
> it when we waste more memory for tracing as very few people use it
> (compared to those that use Linux).
>
> Is it possible to allocate this only when its first used?
>
> -- Steve
I think the answer is yes.
Compare with link ftrace_event_file with static syscall_metadata, another option
is put into structure trace_array, just like enabled_enter_syscalls and
enabled_exit_syscalls BITMAP already in there
(need change to dynamic allocated NR_syscalls array, just keep a
pointer in trace_array).
Then in this way, there don't have any extra size overhead for static
syscall_metadata,
but need to allocate a array with NR_syscalls elements when first use
syscall tracing.
which option do you prefer? steven.
.jovi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 03/11] tracing: add soft disable for syscall events
On Sat, Jun 22, 2013 at 4:22 AM, Steven Rostedt rost...@goodmis.org wrote:
On Fri, 2013-06-21 at 14:53 +0800, zhangwei(Jovi) wrote:
On 2013/6/21 2:31, Tom Zanussi wrote:
Add support for SOFT_DISABLE to syscall events.
The original SOFT_DISABLE patches didn't add support for soft disable
of syscall events; this adds it and paves the way for future patches
allowing triggers to be added to syscall events, since triggers are
built on top of SOFT_DISABLE.
Because the trigger and SOFT_DISABLE bits are attached to the
ftrace_event_file associated with the event, pointers to the
ftrace_event_files associated with the event are added to the syscall
metadata entry for the event.
Signed-off-by: Tom Zanussi tom.zanu...@linux.intel.com
---
include/linux/syscalls.h | 2 ++
include/trace/syscall.h | 5 +
kernel/trace/trace_syscalls.c | 28
3 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 4147d70..b4c2afa 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -158,6 +158,8 @@ extern struct trace_event_functions
exit_syscall_print_funcs;
.args = nb ? args_##sname : NULL, \
.enter_event= event_enter_##sname, \
.exit_event = event_exit_##sname, \
+ .enter_file = NULL, /* Filled in at boot */ \
+ .exit_file = NULL, /* Filled in at boot */ \
.enter_fields =
LIST_HEAD_INIT(__syscall_meta_##sname.enter_fields), \
}; \
static struct syscall_metadata __used \
diff --git a/include/trace/syscall.h b/include/trace/syscall.h
index fed853f..ba24d3a 100644
--- a/include/trace/syscall.h
+++ b/include/trace/syscall.h
@@ -19,6 +19,8 @@
* @enter_fields: list of fields for syscall_enter trace event
* @enter_event: associated syscall_enter trace event
* @exit_event: associated syscall_exit trace event
+ * @enter_file: associated syscall_enter ftrace event file
+ * @exit_file: associated syscall_exit ftrace event file
*/
struct syscall_metadata {
const char *name;
@@ -30,6 +32,9 @@ struct syscall_metadata {
struct ftrace_event_call *enter_event;
struct ftrace_event_call *exit_event;
+
+ struct ftrace_event_file *enter_file;
+ struct ftrace_event_file *exit_file;
};
I doubt this could work correctly.
struct ftrace_event_file is allocated dynamically, there could
have many ftrace_event_file in there, associated with different trace_array,
it means there may have many ftrace_event_file linked with same
syscall_metadata.
The enter_file/exit_file pointer of syscall_metadata will be override if
another
ftrace_event_file registered in some other trace_array.
Perhaps we could use simple list_head in here, which link with all
registered ftrace_event_file.
Oleg use struct event_file_link for trace_kprobe, the structure could be
reuse in here.
struct event_file_link {
struct ftrace_event_file*file;
struct list_headlist;
};
struct syscall_metadata {
const char *name;
int syscall_nr;
int nb_args;
const char **types;
const char **args;
struct list_head enter_fields;
struct ftrace_event_call *enter_event;
struct ftrace_event_call *exit_event;
struct list_headenter_files;
struct list_headexit_files;
};
I would really like to make these smaller. They are made for every
system call, and I consider tracing to always be added overhead. I hate
it when we waste more memory for tracing as very few people use it
(compared to those that use Linux).
Is it possible to allocate this only when its first used?
-- Steve
I think the answer is yes.
Compare with link ftrace_event_file with static syscall_metadata, another option
is put into structure trace_array, just like enabled_enter_syscalls and
enabled_exit_syscalls BITMAP already in there
(need change to dynamic allocated NR_syscalls array, just keep a
pointer in trace_array).
Then in this way, there don't have any extra size overhead for static
syscall_metadata,
but need to allocate a array with NR_syscalls elements when first use
syscall tracing.
which option do you prefer? steven.
.jovi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] tracing: Expose event tracing infrastructure
On Wed, Mar 13, 2013 at 6:41 PM, zhangwei(Jovi)
wrote:
> [change from v1: add missed type assignment in ftrace_event_register]
>
> Currently event tracing only can be use for ftrace and perf,
> there don't have any mechanism to let modules(like external tracing tool)
> register callback tracing function.
>
> Event tracing implement based on tracepoint, compare with raw tracepoint,
> event tracing infrastructure provide built-in structured event annotate
> format,
> this feature should expose to external user.
>
> For example, simple pseudo ktap script demonstrate how to use this event
> tracing expose change.
>
> function event_trace(e)
> {
> printf(e.annotate);
> }
>
> os.trace("sched:sched_switch", event_trace);
> os.trace("irq:softirq_raise", event_trace);
>
> The running result:
> sched_switch: prev_comm=rcu_sched prev_pid=10 prev_prio=120 prev_state=S ==>
> next_comm=swapper/1 next_pid=0 next_prio=120
> softirq_raise: vec=1 [action=TIMER]
> ...
>
> This expose change can be use by other tracing tool, like systemtap/lttng,
> if they would implement this.
>
> This patch introduce struct event_trace_ops, it have two function pointers,
> pre_trace and do_trace. when ftrace_raw_event_ function hit,
> it will call all registered event_trace_ops.
>
> Use this unify callback mechanism, ftrace_raw_event_ and
> perf_trace_ is integrated into one function,
> the benefit of this change is kernel size shrink ~52K(with ftrace and perf
> compiled in).
>
> textdata bss dec hex filename
> 7801238 841596 3473408 12116242 b8e112 vmlinux.old
> 7757064 833596 3473408 12064068 b81544 vmlinux.new
>
> Signed-off-by: zhangwei(Jovi)
> ---
> include/linux/ftrace_event.h | 63 +-
> include/trace/ftrace.h | 198
> --
> kernel/trace/trace_events.c | 174 ++---
> 3 files changed, 260 insertions(+), 175 deletions(-)
>
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 13a54d0..4539a79 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -167,9 +167,6 @@ struct ftrace_event_call;
> struct ftrace_event_class {
> char*system;
> void*probe;
> -#ifdef CONFIG_PERF_EVENTS
> - void*perf_probe;
> -#endif
> int (*reg)(struct ftrace_event_call *event,
>enum trace_reg type, void *data);
> int (*define_fields)(struct ftrace_event_call *);
> @@ -199,6 +196,57 @@ enum {
> TRACE_EVENT_FL_IGNORE_ENABLE= (1 <<
> TRACE_EVENT_FL_IGNORE_ENABLE_BIT),
> };
>
> +struct ftrace_trace_descriptor_t {
> + struct ring_buffer_event *event;
> + struct ring_buffer *buffer;
> + unsigned long irq_flags;
> + int pc;
> +};
> +
> +#ifdef CONFIG_PERF_EVENTS
> +struct perf_trace_descriptor_t {
> + struct pt_regs __regs;
> + struct task_struct *__task;
> + u64 __addr;
> + u64 __count;
> + int rctx;
> +};
> +#endif
> +
> +/*
> + * trace_descriptor_t is purpose for passing arguments between
> + * pre_trace and do_trace function.
> + * this definition is ugly, change it in future.
> + */
> +struct trace_descriptor_t {
> + struct ftrace_trace_descriptor_tf;
> +#ifdef CONFIG_PERF_EVENTS
> + struct perf_trace_descriptor_t p;
> +#endif
> + void *data;
> +};
> +
> +enum TRACE_REG_TYPE {
> + TRACE_REG_FTRACE,
> + TRACE_REG_PERF,
> +};
> +
> +/* callback function for tracing */
> +struct event_trace_ops {
> + void *(*pre_trace)(struct ftrace_event_call *event_call,
> + int entry_size, void *data);
> + void (*do_trace)(struct ftrace_event_call *event_call,
> +void *entry, int entry_size, void *data);
> +};
> +
> +struct ftrace_probe {
> + struct list_head list;
> +
> + /* 0: TRACE_REG_FTRACE; 1 : TRACE_REG_PERF */
> + int type;
> + struct event_trace_ops *ops;
> +};
> +
> struct ftrace_event_call {
> struct list_headlist;
> struct ftrace_event_class *class;
> @@ -210,6 +258,10 @@ struct ftrace_event_call {
> void*mod;
> void*data;
>
> + /* list head of "struct ftrace_probe" */
> + struct list_headprobe_ops_list;
> + int probe_count;
> +
> /*
> * 32 bit flags:
> * bit 1: enabled
> @@ -274,6 +326,11 @@ extern int trace_define_field(struct ftrace_event_call
> *call, const char *type,
> extern int trace_add_event_call(struct ftrace_event_call *call);
> extern void trace_remove_event_call(struct ftrace_event_call *call);
>
> +extern int ftrace_event_register(struct ftrace_event_call *call, int type,
> +struct
Re: [PATCH v2] tracing: Expose event tracing infrastructure
On Wed, Mar 13, 2013 at 6:41 PM, zhangwei(Jovi)
jovi.zhang...@huawei.com wrote:
[change from v1: add missed type assignment in ftrace_event_register]
Currently event tracing only can be use for ftrace and perf,
there don't have any mechanism to let modules(like external tracing tool)
register callback tracing function.
Event tracing implement based on tracepoint, compare with raw tracepoint,
event tracing infrastructure provide built-in structured event annotate
format,
this feature should expose to external user.
For example, simple pseudo ktap script demonstrate how to use this event
tracing expose change.
function event_trace(e)
{
printf(e.annotate);
}
os.trace(sched:sched_switch, event_trace);
os.trace(irq:softirq_raise, event_trace);
The running result:
sched_switch: prev_comm=rcu_sched prev_pid=10 prev_prio=120 prev_state=S ==
next_comm=swapper/1 next_pid=0 next_prio=120
softirq_raise: vec=1 [action=TIMER]
...
This expose change can be use by other tracing tool, like systemtap/lttng,
if they would implement this.
This patch introduce struct event_trace_ops, it have two function pointers,
pre_trace and do_trace. when ftrace_raw_event_call function hit,
it will call all registered event_trace_ops.
Use this unify callback mechanism, ftrace_raw_event_call and
perf_trace_call is integrated into one function,
the benefit of this change is kernel size shrink ~52K(with ftrace and perf
compiled in).
textdata bss dec hex filename
7801238 841596 3473408 12116242 b8e112 vmlinux.old
7757064 833596 3473408 12064068 b81544 vmlinux.new
Signed-off-by: zhangwei(Jovi) jovi.zhang...@huawei.com
---
include/linux/ftrace_event.h | 63 +-
include/trace/ftrace.h | 198
--
kernel/trace/trace_events.c | 174 ++---
3 files changed, 260 insertions(+), 175 deletions(-)
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 13a54d0..4539a79 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -167,9 +167,6 @@ struct ftrace_event_call;
struct ftrace_event_class {
char*system;
void*probe;
-#ifdef CONFIG_PERF_EVENTS
- void*perf_probe;
-#endif
int (*reg)(struct ftrace_event_call *event,
enum trace_reg type, void *data);
int (*define_fields)(struct ftrace_event_call *);
@@ -199,6 +196,57 @@ enum {
TRACE_EVENT_FL_IGNORE_ENABLE= (1
TRACE_EVENT_FL_IGNORE_ENABLE_BIT),
};
+struct ftrace_trace_descriptor_t {
+ struct ring_buffer_event *event;
+ struct ring_buffer *buffer;
+ unsigned long irq_flags;
+ int pc;
+};
+
+#ifdef CONFIG_PERF_EVENTS
+struct perf_trace_descriptor_t {
+ struct pt_regs __regs;
+ struct task_struct *__task;
+ u64 __addr;
+ u64 __count;
+ int rctx;
+};
+#endif
+
+/*
+ * trace_descriptor_t is purpose for passing arguments between
+ * pre_trace and do_trace function.
+ * this definition is ugly, change it in future.
+ */
+struct trace_descriptor_t {
+ struct ftrace_trace_descriptor_tf;
+#ifdef CONFIG_PERF_EVENTS
+ struct perf_trace_descriptor_t p;
+#endif
+ void *data;
+};
+
+enum TRACE_REG_TYPE {
+ TRACE_REG_FTRACE,
+ TRACE_REG_PERF,
+};
+
+/* callback function for tracing */
+struct event_trace_ops {
+ void *(*pre_trace)(struct ftrace_event_call *event_call,
+ int entry_size, void *data);
+ void (*do_trace)(struct ftrace_event_call *event_call,
+void *entry, int entry_size, void *data);
+};
+
+struct ftrace_probe {
+ struct list_head list;
+
+ /* 0: TRACE_REG_FTRACE; 1 : TRACE_REG_PERF */
+ int type;
+ struct event_trace_ops *ops;
+};
+
struct ftrace_event_call {
struct list_headlist;
struct ftrace_event_class *class;
@@ -210,6 +258,10 @@ struct ftrace_event_call {
void*mod;
void*data;
+ /* list head of struct ftrace_probe */
+ struct list_headprobe_ops_list;
+ int probe_count;
+
/*
* 32 bit flags:
* bit 1: enabled
@@ -274,6 +326,11 @@ extern int trace_define_field(struct ftrace_event_call
*call, const char *type,
extern int trace_add_event_call(struct ftrace_event_call *call);
extern void trace_remove_event_call(struct ftrace_event_call *call);
+extern int ftrace_event_register(struct ftrace_event_call *call, int type,
+struct event_trace_ops *ops);
+extern void ftrace_event_unregister(struct ftrace_event_call *call,
Re: [PATCH 07/13] tracing/kdb: remove redundant checking
On Mon, Mar 11, 2013 at 10:46 PM, Jason Wessel
wrote:
> On 03/11/2013 09:09 AM, Steven Rostedt wrote:
>> This is Jason's code.
>>
>> Jason, please give an Ack or Nack.
>>
>> Thanks,
>>
>> -- Steve
>>
>>
>> On Mon, 2013-03-11 at 15:13 +0800, zhangwei(Jovi) wrote:
>>> trace_empty is checking in while-loop, so the previous checking
>>> is totally redundant, and more worse, the first trace entry is losted.
>>>
>>> so remove it.
>>>
>>> Signed-off-by: zhangwei(Jovi)
>>> ---
>>> kernel/trace/trace_kdb.c |3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c
>>> index 3c5c5df..6489b2e 100644
>>> --- a/kernel/trace/trace_kdb.c
>>> +++ b/kernel/trace/trace_kdb.c
>>> @@ -57,8 +57,7 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
>>> ring_buffer_read_start(iter.buffer_iter[cpu_file]);
>>> tracing_iter_reset(, cpu_file);
>>> }
>>> -if (!trace_empty())
>>> -trace_find_next_entry_inc();
>>> +
>>> while (!trace_empty()) {
>>> if (!cnt)
>>> kdb_printf("-\n");
>>
>
>
> This was just a copy of part of the logic used for printing the information
> in a similar manner to what you get when you cat the trace buffer to obtain
> the human readable version.
>
> May I ask how you tested it though?
No, just watch the code, and that part looks weird.
kdb_ftdump seems copied from ftrace_dump function, it have similar
functionality,
can we have some way to unify these two function into one common function?
this at least can save trace_iterator static memory ~8k+. (I'm not
sure this can work or not)
>
> As far as I know the patch I sent Stephen quite a while ago got lost and the
> mainline version of the "ftdump" doesn't actually work.
>
> Example:
> [0]kdb> ftdump
> Dumping ftrace buffer:
> 3BUG: sleeping function called from invalid context at
> /space/jw/git/kgdb/linux-2.6-kgdb/mm/slub.c:925
> 3in_atomic(): 1, irqs_disabled(): 1, pid: 81, name: sh
> Pid: 81, comm: sh Not tainted 3.9.0-rc1-WR4.3.0.0_standard+ #568
> Call Trace:
> <#DB> [] __might_sleep+0xde/0x100
> [] kmem_cache_alloc_trace+0xdb/0x170
> [] ring_buffer_read_prepare+0x4d/0x70
> [] kdb_ftdump+0x1e8/0x410
> [] kdb_parse+0x209/0x690
> [] kdb_main_loop+0x67c/0x8c0
> [] kdb_stub+0x1d3/0x420
> [] ? __send_signal+0x1ad/0x3e0
> [] kgdb_cpu_enter+0x27e/0x590
> [] kgdb_handle_exception+0x161/0x1c0
> [] __kgdb_notify+0x31/0xe0
> [] kgdb_ll_trap+0x40/0x50
> [] do_int3+0x52/0x130
> [] int3+0x25/0x40
> [] ? sysrq_handle_dbg+0x32/0x60
> <> [] __handle_sysrq+0x129/0x190
>
>
> I think we need to actually empirically prove the code change is right or
> wrong before merging it, as well as cleaning up the change log slightly.
>
> I'll go hunt down the patch the fixes the oops first.
>
> Cheers,
> Jason.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 12/13] scsi/tracing: include correct header file
On Mon, Mar 11, 2013 at 11:36 PM, Steven Rostedt wrote: > On Mon, 2013-03-11 at 15:24 +, James Bottomley wrote: > >> > In the future, send them as a separate patch series. There's no >> > dependencies on these patches with the rest of the series. They should >> > have been sent directly to James, with me as the Cc. >> >> Actually, if you want them applied, they need to go to the SCSI list >> . It tracks all my pending patches. Stuff >> to me personally has a high probability of getting forgotten. > > Zhangwei, > > As you may be sending patches to other maintainers, please read the > MAINTAINERS file and send appropriate patches in a separate series to > them as directed: > > SCSI SUBSYSTEM > M: "James E.J. Bottomley" > L: linux-s...@vger.kernel.org > T: git > git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6.git > T: git > git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-rc-fixes-2.6.git > T: git > git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-pending-2.6.git > S: Maintained > F: drivers/scsi/ > F: include/scsi/ > > Cc the listed maintainers, as well as the listed "L:" lists. > Thanks, sometimes I forgot cc mailing list in MAINTAINERS file(just attention on M), I will got this in next patch submission. I will resend those two patches with Cc linux-s...@vger.kernel.org > -- Steve > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/13] tracing: remove dump_ran check in __ftrace_dump
On Mon, Mar 11, 2013 at 10:08 PM, Steven Rostedt wrote: > On Mon, 2013-03-11 at 15:13 +0800, zhangwei(Jovi) wrote: >> It's reasonable to call __ftrace_dump function not only once, >> so remove the dump_ran variable checking. > > This needs a little more work. On an oops, I only want it dumped once, > because a crash can cause another crash while its dumping, and without > that check in will corrupt the buffer. For reclusive dumping, it's already under the protection of ftrace_dump_lock spinlock, I missed something? would you explain more on this case? > > Now, we have things like ctrl^z that also does a dump where we don't > want to disable it. Cleaning this up has been on my todo list for a > while. I may go ahead and clean that up myself. Nice, please ignore this patch if that code will be cleanup. Thanks. > > -- Steve > >> >> Signed-off-by: zhangwei(Jovi) >> --- >> kernel/trace/trace.c |6 -- >> 1 file changed, 6 deletions(-) >> >> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c >> index 090eddb..4cec7b8 100644 >> --- a/kernel/trace/trace.c >> +++ b/kernel/trace/trace.c >> @@ -5106,17 +5106,12 @@ __ftrace_dump(bool disable_tracing, enum >> ftrace_dump_mode oops_dump_mode) >> /* use static because iter can be a bit big for the stack */ >> static struct trace_iterator iter; >> unsigned int old_userobj; >> - static int dump_ran; >> unsigned long flags; >> int cnt = 0, cpu; >> >> /* only one dump */ >> local_irq_save(flags); >> arch_spin_lock(_dump_lock); >> - if (dump_ran) >> - goto out; >> - >> - dump_ran = 1; >> >> tracing_off(); >> >> @@ -5206,7 +5201,6 @@ __ftrace_dump(bool disable_tracing, enum >> ftrace_dump_mode oops_dump_mode) >> tracing_on(); >> } >> >> - out: >> arch_spin_unlock(_dump_lock); >> local_irq_restore(flags); >> } > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/13] tracing: remove dump_ran check in __ftrace_dump
On Mon, Mar 11, 2013 at 10:08 PM, Steven Rostedt rost...@goodmis.org wrote: On Mon, 2013-03-11 at 15:13 +0800, zhangwei(Jovi) wrote: It's reasonable to call __ftrace_dump function not only once, so remove the dump_ran variable checking. This needs a little more work. On an oops, I only want it dumped once, because a crash can cause another crash while its dumping, and without that check in will corrupt the buffer. For reclusive dumping, it's already under the protection of ftrace_dump_lock spinlock, I missed something? would you explain more on this case? Now, we have things like ctrl^z that also does a dump where we don't want to disable it. Cleaning this up has been on my todo list for a while. I may go ahead and clean that up myself. Nice, please ignore this patch if that code will be cleanup. Thanks. -- Steve Signed-off-by: zhangwei(Jovi) jovi.zhang...@huawei.com --- kernel/trace/trace.c |6 -- 1 file changed, 6 deletions(-) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 090eddb..4cec7b8 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -5106,17 +5106,12 @@ __ftrace_dump(bool disable_tracing, enum ftrace_dump_mode oops_dump_mode) /* use static because iter can be a bit big for the stack */ static struct trace_iterator iter; unsigned int old_userobj; - static int dump_ran; unsigned long flags; int cnt = 0, cpu; /* only one dump */ local_irq_save(flags); arch_spin_lock(ftrace_dump_lock); - if (dump_ran) - goto out; - - dump_ran = 1; tracing_off(); @@ -5206,7 +5201,6 @@ __ftrace_dump(bool disable_tracing, enum ftrace_dump_mode oops_dump_mode) tracing_on(); } - out: arch_spin_unlock(ftrace_dump_lock); local_irq_restore(flags); } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 12/13] scsi/tracing: include correct header file
On Mon, Mar 11, 2013 at 11:36 PM, Steven Rostedt rost...@goodmis.org wrote: On Mon, 2013-03-11 at 15:24 +, James Bottomley wrote: In the future, send them as a separate patch series. There's no dependencies on these patches with the rest of the series. They should have been sent directly to James, with me as the Cc. Actually, if you want them applied, they need to go to the SCSI list linux-s...@vger.kernel.org. It tracks all my pending patches. Stuff to me personally has a high probability of getting forgotten. Zhangwei, As you may be sending patches to other maintainers, please read the MAINTAINERS file and send appropriate patches in a separate series to them as directed: SCSI SUBSYSTEM M: James E.J. Bottomley jbottom...@parallels.com L: linux-s...@vger.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6.git T: git git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-rc-fixes-2.6.git T: git git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-pending-2.6.git S: Maintained F: drivers/scsi/ F: include/scsi/ Cc the listed maintainers, as well as the listed L: lists. Thanks, sometimes I forgot cc mailing list in MAINTAINERS file(just attention on M), I will got this in next patch submission. I will resend those two patches with Cc linux-s...@vger.kernel.org -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 07/13] tracing/kdb: remove redundant checking
On Mon, Mar 11, 2013 at 10:46 PM, Jason Wessel
jason.wes...@windriver.com wrote:
On 03/11/2013 09:09 AM, Steven Rostedt wrote:
This is Jason's code.
Jason, please give an Ack or Nack.
Thanks,
-- Steve
On Mon, 2013-03-11 at 15:13 +0800, zhangwei(Jovi) wrote:
trace_empty is checking in while-loop, so the previous checking
is totally redundant, and more worse, the first trace entry is losted.
so remove it.
Signed-off-by: zhangwei(Jovi) jovi.zhang...@huawei.com
---
kernel/trace/trace_kdb.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c
index 3c5c5df..6489b2e 100644
--- a/kernel/trace/trace_kdb.c
+++ b/kernel/trace/trace_kdb.c
@@ -57,8 +57,7 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
ring_buffer_read_start(iter.buffer_iter[cpu_file]);
tracing_iter_reset(iter, cpu_file);
}
-if (!trace_empty(iter))
-trace_find_next_entry_inc(iter);
+
while (!trace_empty(iter)) {
if (!cnt)
kdb_printf(-\n);
This was just a copy of part of the logic used for printing the information
in a similar manner to what you get when you cat the trace buffer to obtain
the human readable version.
May I ask how you tested it though?
No, just watch the code, and that part looks weird.
kdb_ftdump seems copied from ftrace_dump function, it have similar
functionality,
can we have some way to unify these two function into one common function?
this at least can save trace_iterator static memory ~8k+. (I'm not
sure this can work or not)
As far as I know the patch I sent Stephen quite a while ago got lost and the
mainline version of the ftdump doesn't actually work.
Example:
[0]kdb ftdump
Dumping ftrace buffer:
3BUG: sleeping function called from invalid context at
/space/jw/git/kgdb/linux-2.6-kgdb/mm/slub.c:925
3in_atomic(): 1, irqs_disabled(): 1, pid: 81, name: sh
Pid: 81, comm: sh Not tainted 3.9.0-rc1-WR4.3.0.0_standard+ #568
Call Trace:
#DB [81069ffe] __might_sleep+0xde/0x100
[8112611b] kmem_cache_alloc_trace+0xdb/0x170
[810c01bd] ring_buffer_read_prepare+0x4d/0x70
[810d4d28] kdb_ftdump+0x1e8/0x410
[810a9a89] kdb_parse+0x209/0x690
[810aad0c] kdb_main_loop+0x67c/0x8c0
[810ad4b3] kdb_stub+0x1d3/0x420
[8104ccfd] ? __send_signal+0x1ad/0x3e0
[810a33be] kgdb_cpu_enter+0x27e/0x590
[810a3981] kgdb_handle_exception+0x161/0x1c0
[81027cf1] __kgdb_notify+0x31/0xe0
[81027e10] kgdb_ll_trap+0x40/0x50
[81002e12] do_int3+0x52/0x130
[81674345] int3+0x25/0x40
[810a2be2] ? sysrq_handle_dbg+0x32/0x60
EOE [813e1e69] __handle_sysrq+0x129/0x190
I think we need to actually empirically prove the code change is right or
wrong before merging it, as well as cleaning up the change log slightly.
I'll go hunt down the patch the fixes the oops first.
Cheers,
Jason.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tracing: fix twice trace iterator init
>From ac499cd340bf2ba47b3d1dd129a3bb7527c195ae Mon Sep 17 00:00:00 2001
From: Jovi Zhang
Date: Fri, 25 Jan 2013 18:03:07 +0800
Subject: [PATCH] tracing: fix twice trace iterator init
trace iterator is already inited in trace_init_global_iter,
so there don't need to assign again.
Signed-off-by: Jovi Zhang
---
kernel/trace/trace.c |5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e512567..8d99728 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -5005,6 +5005,7 @@ __ftrace_dump(bool disable_tracing, enum
ftrace_dump_mode oops_dump_mode)
if (disable_tracing)
ftrace_kill();
+ /* Simulate the iterator */
trace_init_global_iter();
for_each_tracing_cpu(cpu) {
@@ -5016,10 +5017,6 @@ __ftrace_dump(bool disable_tracing, enum
ftrace_dump_mode oops_dump_mode)
/* don't look at user memory in panic mode */
trace_flags &= ~TRACE_ITER_SYM_USEROBJ;
- /* Simulate the iterator */
- iter.tr = _trace;
- iter.trace = current_trace;
-
switch (oops_dump_mode) {
case DUMP_ALL:
iter.cpu_file = TRACE_PIPE_ALL_CPU;
--
1.7.9.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tracing: fix twice trace iterator init
From ac499cd340bf2ba47b3d1dd129a3bb7527c195ae Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Fri, 25 Jan 2013 18:03:07 +0800
Subject: [PATCH] tracing: fix twice trace iterator init
trace iterator is already inited in trace_init_global_iter,
so there don't need to assign again.
Signed-off-by: Jovi Zhang bookj...@gmail.com
---
kernel/trace/trace.c |5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e512567..8d99728 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -5005,6 +5005,7 @@ __ftrace_dump(bool disable_tracing, enum
ftrace_dump_mode oops_dump_mode)
if (disable_tracing)
ftrace_kill();
+ /* Simulate the iterator */
trace_init_global_iter(iter);
for_each_tracing_cpu(cpu) {
@@ -5016,10 +5017,6 @@ __ftrace_dump(bool disable_tracing, enum
ftrace_dump_mode oops_dump_mode)
/* don't look at user memory in panic mode */
trace_flags = ~TRACE_ITER_SYM_USEROBJ;
- /* Simulate the iterator */
- iter.tr = global_trace;
- iter.trace = current_trace;
-
switch (oops_dump_mode) {
case DUMP_ALL:
iter.cpu_file = TRACE_PIPE_ALL_CPU;
--
1.7.9.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[tip:perf/core] tracing: Verify target file before registering a uprobe event
Commit-ID: d24d7dbf3cc49b00a152e55e24f0eeb173c7a971
Gitweb: http://git.kernel.org/tip/d24d7dbf3cc49b00a152e55e24f0eeb173c7a971
Author: Jovi Zhang
AuthorDate: Wed, 18 Jul 2012 18:16:44 +0800
Committer: Steven Rostedt
CommitDate: Mon, 21 Jan 2013 13:22:31 -0500
tracing: Verify target file before registering a uprobe event
Without this patch, we can register a uprobe event for a directory.
Enabling such a uprobe event would anyway fail.
Example:
$ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
However dirctories cannot be valid targets for uprobe.
Hence verify if the target is a regular file during the probe
registration.
Link: http://lkml.kernel.org/r/20130103004212.690763...@goodmis.org
Cc: Namhyung Kim
Signed-off-by: Jovi Zhang
Acked-by: Srikar Dronamraju
[ cleaned up whitespace and removed redundant IS_DIR() check ]
Signed-off-by: Steven Rostedt
---
kernel/trace/trace_uprobe.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index c86e6d4..87b6db4 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -258,6 +258,10 @@ static int create_trace_uprobe(int argc, char **argv)
goto fail_address_parse;
inode = igrab(path.dentry->d_inode);
+ if (!S_ISREG(inode->i_mode)) {
+ ret = -EINVAL;
+ goto fail_address_parse;
+ }
argc -= 2;
argv += 2;
@@ -356,7 +360,7 @@ fail_address_parse:
if (inode)
iput(inode);
- pr_info("Failed to parse address.\n");
+ pr_info("Failed to parse address or file.\n");
return ret;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[tip:perf/core] tracing: Verify target file before registering a uprobe event
Commit-ID: d24d7dbf3cc49b00a152e55e24f0eeb173c7a971
Gitweb: http://git.kernel.org/tip/d24d7dbf3cc49b00a152e55e24f0eeb173c7a971
Author: Jovi Zhang bookj...@gmail.com
AuthorDate: Wed, 18 Jul 2012 18:16:44 +0800
Committer: Steven Rostedt rost...@goodmis.org
CommitDate: Mon, 21 Jan 2013 13:22:31 -0500
tracing: Verify target file before registering a uprobe event
Without this patch, we can register a uprobe event for a directory.
Enabling such a uprobe event would anyway fail.
Example:
$ echo 'p /bin:0x4245c0' /sys/kernel/debug/tracing/uprobe_events
However dirctories cannot be valid targets for uprobe.
Hence verify if the target is a regular file during the probe
registration.
Link: http://lkml.kernel.org/r/20130103004212.690763...@goodmis.org
Cc: Namhyung Kim namhy...@kernel.org
Signed-off-by: Jovi Zhang bookj...@gmail.com
Acked-by: Srikar Dronamraju sri...@linux.vnet.ibm.com
[ cleaned up whitespace and removed redundant IS_DIR() check ]
Signed-off-by: Steven Rostedt rost...@goodmis.org
---
kernel/trace/trace_uprobe.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index c86e6d4..87b6db4 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -258,6 +258,10 @@ static int create_trace_uprobe(int argc, char **argv)
goto fail_address_parse;
inode = igrab(path.dentry-d_inode);
+ if (!S_ISREG(inode-i_mode)) {
+ ret = -EINVAL;
+ goto fail_address_parse;
+ }
argc -= 2;
argv += 2;
@@ -356,7 +360,7 @@ fail_address_parse:
if (inode)
iput(inode);
- pr_info(Failed to parse address.\n);
+ pr_info(Failed to parse address or file.\n);
return ret;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] ktap: Another dynamic tracing tool for Linux
On Fri, Jan 18, 2013 at 11:35 AM, Frank Ch. Eigler wrote: > Hi - > > On Fri, Jan 18, 2013 at 09:24:55AM +0800, Jovi Zhang wrote: >> Let us continue this ktap topic in this thread :). >> ktap code is public available at github, please clone from: >> https://github.com/ktap/ktap.git >> [...] > > Neat stuff. I have one set of initial observations: even with a nice > small bytecode language, the VM has to be skeptical and careful. For > example, some dangers: > > - any dynamic memory allocation from within the context of an > arbitrary tracepoints (table_* functions, stack frames) > - unchecked arithmetic (division-by-zero - OP_DIV, overflows, > function arity limits) > - time-quantity of computation (limit loop iterations / #-bytecodes?, > DO_JMP infinite loops) > - stack-frame usage of interpreter (if internally recursive, or if > too much state kept on stack) > - trusting validity of bytecode (imagine an attacker sending random > or harmful junk, jumping out of bytecode ranges) > - calls out from interpreter into native code - to ensure that *all* > those functions (transitively) are valid to call from general > e.g. tracepoint context (e.g. sleeps often aren't) > - (and these problems get even worse with evaluation from the > context of kprobes) > > > - FChE Yeh, that's why this code is called initial implementation. most of dangers you pointed I have thought about, most of them should not have too much hard to solve it, it just need some time, some of them already listed in Todo-list.txt. Thanks your quick observations, the comments is valueable, and need to be address step by step in future development (I also think most issues is very common for script dynamic tracing tool, not specific for ktap, but ktap have to go through those barrier one by one) Let's waiting for ktap official release 0.1, the code quality would be more better than now :) .jovi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] ktap: Another dynamic tracing tool for Linux
On Fri, Jan 4, 2013 at 11:19 PM, Frank Ch. Eigler wrote: > Hi - > > bookjovi wrote: > >> >> [...] >> >> ktap use lua language syntax and bytecode as initial implementation, >> > >> > Interesting approach. I recall we considered it way back when, but >> > rejected it for a couple of reasons, including the at-the-time >> > perceived unwelcomeness of a serious bytecode interpreter within the >> > kernel. > >> [...] Obviously, the bytecode design is the biggest differences >> with systemtap. [...] many Linux box doesn't deliver with gcc, >> this is very common in embedded device, even there installed gcc, >> but it's hard(impossible) to get matched kernel source. [...] > > Right, that is a strong attraction. > > >> [...] On clear that, ktap is not a replacement to systemtap, it's >> supplementation. > > FWIW, it would be reasonable to have systemtap emit bytecodes as an > alternative backend. All that has been lacking is a powerful and > pervasive enough interpreter. The script language is not tied to its > implementation via C code generation. > > > That reminds me of your item #4 on your ktap-systemtap comparison: > >> 4). ktap is safe, whatever you doing in script file, you will never >> crash your kernel. > > This is roughly as true for systemtap as for anything else. The > scripts are constrainted to be safe. Kernel crashes that occur are > due to occasional bugs in the systemtap runtime library, or down in > the kernel facilities being used. You would likely encounter both > classes of problems in a kernel-side bytecode interpreter, regardless > of the theoretical safety of the scripting language. (This is one of > the reasons we've been building out the pure-userspace dyninst-based > systemtap backend.) > > >> I will try to make ktap develop progress more faster, and make ktap >> source code public available soon. > > Yes, please. (RFC/experimental code need not be complete before > posting; why not develop straight on github?) > > > - FChE Let us continue this ktap topic in this thread :). ktap code is public available at github, please clone from: https://github.com/ktap/ktap.git Brief introduction for initial building ktap: --- ktap is a new dynamic tracing tool for Linux, it is a script environment running in Linux kernel(similar with systemtap and Dtrace). ktap is GPL licensed, the compiler and virtual machine is borrowed from lua language as initial implementation. compiler and loader is included in userspace tool ktap compiling: make ktap--- generate userspace ktap tool make --- generate ktapvm kernel module try to running ktap: ./ktap scripts/tracepoint.ktap This commit is just a initial pulbic code release, it have basic tracepoint and syscall support, please waiting ktap release 1.0. :) I suggest you put this ktap directory into linux/kernel/trace/. (this initial code have one issue: it need to patch ftrace code in Linux kernel, see ftrace.patch, I will try to get rid it sooner.) For any question, please contact the author, Jovi Zhang. ktap is a open source project, welcome hacking and contributing. .jovi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] ktap: Another dynamic tracing tool for Linux
On Fri, Jan 4, 2013 at 11:19 PM, Frank Ch. Eigler f...@redhat.com wrote: Hi - bookjovi wrote: [...] ktap use lua language syntax and bytecode as initial implementation, Interesting approach. I recall we considered it way back when, but rejected it for a couple of reasons, including the at-the-time perceived unwelcomeness of a serious bytecode interpreter within the kernel. [...] Obviously, the bytecode design is the biggest differences with systemtap. [...] many Linux box doesn't deliver with gcc, this is very common in embedded device, even there installed gcc, but it's hard(impossible) to get matched kernel source. [...] Right, that is a strong attraction. [...] On clear that, ktap is not a replacement to systemtap, it's supplementation. FWIW, it would be reasonable to have systemtap emit bytecodes as an alternative backend. All that has been lacking is a powerful and pervasive enough interpreter. The script language is not tied to its implementation via C code generation. That reminds me of your item #4 on your ktap-systemtap comparison: 4). ktap is safe, whatever you doing in script file, you will never crash your kernel. This is roughly as true for systemtap as for anything else. The scripts are constrainted to be safe. Kernel crashes that occur are due to occasional bugs in the systemtap runtime library, or down in the kernel facilities being used. You would likely encounter both classes of problems in a kernel-side bytecode interpreter, regardless of the theoretical safety of the scripting language. (This is one of the reasons we've been building out the pure-userspace dyninst-based systemtap backend.) I will try to make ktap develop progress more faster, and make ktap source code public available soon. Yes, please. (RFC/experimental code need not be complete before posting; why not develop straight on github?) - FChE Let us continue this ktap topic in this thread :). ktap code is public available at github, please clone from: https://github.com/ktap/ktap.git Brief introduction for initial building ktap: --- ktap is a new dynamic tracing tool for Linux, it is a script environment running in Linux kernel(similar with systemtap and Dtrace). ktap is GPL licensed, the compiler and virtual machine is borrowed from lua language as initial implementation. compiler and loader is included in userspace tool ktap compiling: make ktap--- generate userspace ktap tool make --- generate ktapvm kernel module try to running ktap: ./ktap scripts/tracepoint.ktap This commit is just a initial pulbic code release, it have basic tracepoint and syscall support, please waiting ktap release 1.0. :) I suggest you put this ktap directory into linux/kernel/trace/. (this initial code have one issue: it need to patch ftrace code in Linux kernel, see ftrace.patch, I will try to get rid it sooner.) For any question, please contact the author, Jovi Zhang. ktap is a open source project, welcome hacking and contributing. .jovi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] ktap: Another dynamic tracing tool for Linux
On Fri, Jan 18, 2013 at 11:35 AM, Frank Ch. Eigler f...@redhat.com wrote: Hi - On Fri, Jan 18, 2013 at 09:24:55AM +0800, Jovi Zhang wrote: Let us continue this ktap topic in this thread :). ktap code is public available at github, please clone from: https://github.com/ktap/ktap.git [...] Neat stuff. I have one set of initial observations: even with a nice small bytecode language, the VM has to be skeptical and careful. For example, some dangers: - any dynamic memory allocation from within the context of an arbitrary tracepoints (table_* functions, stack frames) - unchecked arithmetic (division-by-zero - OP_DIV, overflows, function arity limits) - time-quantity of computation (limit loop iterations / #-bytecodes?, DO_JMP infinite loops) - stack-frame usage of interpreter (if internally recursive, or if too much state kept on stack) - trusting validity of bytecode (imagine an attacker sending random or harmful junk, jumping out of bytecode ranges) - calls out from interpreter into native code - to ensure that *all* those functions (transitively) are valid to call from general e.g. tracepoint context (e.g. sleeps often aren't) - (and these problems get even worse with evaluation from the context of kprobes) - FChE Yeh, that's why this code is called initial implementation. most of dangers you pointed I have thought about, most of them should not have too much hard to solve it, it just need some time, some of them already listed in Todo-list.txt. Thanks your quick observations, the comments is valueable, and need to be address step by step in future development (I also think most issues is very common for script dynamic tracing tool, not specific for ktap, but ktap have to go through those barrier one by one) Let's waiting for ktap official release 0.1, the code quality would be more better than now :) .jovi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] ktap: Another dynamic tracing tool for Linux
On Fri, Jan 11, 2013 at 10:19 PM, Michel Dagenais wrote: > You may be interested in KGTP which implements a simple bytecode interpreter > in the kernel to accept GDB tracepoints http://code.google.com/p/kgtp/ > > The bytecode is quite limited but would be easy to extend. KGTP is still not meet my requirement on Linux tracing. ktap don't have gcc or gdb dependence, it's build from scratch, with a clean design, this is very important. You will find more differences if you watch ktap and kgtp deeply. :) > > Eventually we should be able to connect LTTng http://lttng.org/ and KGTP in > order to benefit from the efficiency of LTTng for activating probes and > retrieving data. You are right, LTTng should be possible, and I already planed it, also on some functionality of ftrace and systemtap. Let's wait public ktap for a little time, you then can get a basic overview on ktap. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] ktap: Another dynamic tracing tool for Linux
On Fri, Jan 11, 2013 at 10:19 PM, Michel Dagenais michel.dagen...@polymtl.ca wrote: You may be interested in KGTP which implements a simple bytecode interpreter in the kernel to accept GDB tracepoints http://code.google.com/p/kgtp/ The bytecode is quite limited but would be easy to extend. KGTP is still not meet my requirement on Linux tracing. ktap don't have gcc or gdb dependence, it's build from scratch, with a clean design, this is very important. You will find more differences if you watch ktap and kgtp deeply. :) Eventually we should be able to connect LTTng http://lttng.org/ and KGTP in order to benefit from the efficiency of LTTng for activating probes and retrieving data. You are right, LTTng should be possible, and I already planed it, also on some functionality of ftrace and systemtap. Let's wait public ktap for a little time, you then can get a basic overview on ktap. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] ktap: Another dynamic tracing tool for Linux
On Tue, Jan 1, 2013 at 2:58 AM, Frank Ch. Eigler wrote: > > bookjovi wrote: > > >> [...] This mail is RFC for discuss on a new dynamic tracing tool, I >> name it ktap. (only experimental project now) > > Welcome to the problem domain! Thanks very much, frank, I'm so luck to discussing this topic with you. :) > > >> [...] >> what ktap differentiates with Systemtap is: >> [...] >> 2). ktap have good portability, because it compile source file to >> bytecode, like python and Java. > > (From this PoV, systemtap is just as portable as the kernel, as it > generates the same sort of C code the kernel is built from.) > the portability for ktap what I mean here is binary portability, you will not need to compile arm and x86 binary in ktap, it's all bytecode. but as you pointed, systemtap also have good portability in source level, for more clear, I will change the readme text a little bit about this portability. thanks. >> [...] >> 5). ktap will be open source completely, with GPL license, it might be >> merge into mainline in someday, that's very convince for tracing user. > > (systemtap has always been GPLv2, ever since its beginning in 2005.) Yes, you are right, both is GPL licensed. > > >> [...] >> ktap use lua language syntax and bytecode as initial implementation, > > Interesting approach. I recall we considered it way back when, but > rejected it for a couple of reasons, including the at-the-time > perceived unwelcomeness of a serious bytecode interpreter within the > kernel. > ktap is more like Dtrace, without gcc compiling. Obviously, the bytecode design is the biggest differences with systemtap. systemtap really is a valueable tracing tool, but in many case we cannot use it. many Linux box doesn't deverily with gcc, this is very common in embedded device, even there installed gcc, but it's hard(impossible) to get matched kernel source. embeded world have many hardware architectures, arm/x86/mips/ppc, when those different hardware board is combined into a cluster, you need to prepare several systemtap kernel module in development machine, then upload all kernel module to cluster. but if you have a bytecode mechanism, that's totaly not neccessary, just a source script file or bytecode chunk file. This is just one advantage of bytecode mechanism. Of course we must need to afford some penalty on performance compare with raw C design(systemtap), but gaining better convenient, one directing of ktap is reducing bytecode executing overhead to minimal. On clear that, ktap is not a replacement to systemtap, it's supplementation. > >> it could support kprobe, uprobe, userspace probe, etc. > > Great. > >> I wish you can give me some technical architecture pre-review for >> ktap, before ktap release 1.0. >> Any comments is welcome, thanks very much. > > Have you made any source code available yet? > I will try to make ktap develop progress more faster, and make ktap source code public available soon. .jovi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] ktap: Another dynamic tracing tool for Linux
On Tue, Jan 1, 2013 at 2:58 AM, Frank Ch. Eigler f...@redhat.com wrote: bookjovi wrote: [...] This mail is RFC for discuss on a new dynamic tracing tool, I name it ktap. (only experimental project now) Welcome to the problem domain! Thanks very much, frank, I'm so luck to discussing this topic with you. :) [...] what ktap differentiates with Systemtap is: [...] 2). ktap have good portability, because it compile source file to bytecode, like python and Java. (From this PoV, systemtap is just as portable as the kernel, as it generates the same sort of C code the kernel is built from.) the portability for ktap what I mean here is binary portability, you will not need to compile arm and x86 binary in ktap, it's all bytecode. but as you pointed, systemtap also have good portability in source level, for more clear, I will change the readme text a little bit about this portability. thanks. [...] 5). ktap will be open source completely, with GPL license, it might be merge into mainline in someday, that's very convince for tracing user. (systemtap has always been GPLv2, ever since its beginning in 2005.) Yes, you are right, both is GPL licensed. [...] ktap use lua language syntax and bytecode as initial implementation, Interesting approach. I recall we considered it way back when, but rejected it for a couple of reasons, including the at-the-time perceived unwelcomeness of a serious bytecode interpreter within the kernel. ktap is more like Dtrace, without gcc compiling. Obviously, the bytecode design is the biggest differences with systemtap. systemtap really is a valueable tracing tool, but in many case we cannot use it. many Linux box doesn't deverily with gcc, this is very common in embedded device, even there installed gcc, but it's hard(impossible) to get matched kernel source. embeded world have many hardware architectures, arm/x86/mips/ppc, when those different hardware board is combined into a cluster, you need to prepare several systemtap kernel module in development machine, then upload all kernel module to cluster. but if you have a bytecode mechanism, that's totaly not neccessary, just a source script file or bytecode chunk file. This is just one advantage of bytecode mechanism. Of course we must need to afford some penalty on performance compare with raw C design(systemtap), but gaining better convenient, one directing of ktap is reducing bytecode executing overhead to minimal. On clear that, ktap is not a replacement to systemtap, it's supplementation. it could support kprobe, uprobe, userspace probe, etc. Great. I wish you can give me some technical architecture pre-review for ktap, before ktap release 1.0. Any comments is welcome, thanks very much. Have you made any source code available yet? I will try to make ktap develop progress more faster, and make ktap source code public available soon. .jovi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: uprobe: checking probe event include directory
On Wed, Jul 18, 2012 at 7:45 PM, Srikar Dronamraju
wrote:
> * Jovi Zhang [2012-07-18 19:38:27]:
>
>> On Wed, Jul 18, 2012 at 7:07 PM, Srikar Dronamraju
>> wrote:
>> > The patch looks good,
>> >
>> > Can you modify the description a bit. However you are free to ignore
>> > these comments. After knowing your response, I will ack the patch.
>> >
>> > I would probably put this as:
>> >
>> > The subject could be
>> > tracing: Verify target file before registering a uprobe event
>> >
>> > Description:
>> > Without this patch, we can register a uprobe event for a directory.
>> > Enabling such a uprobe event would anyway fail.
>> >
>> > Example:
>> >
>> > $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
>> >
>> > However directories cannot be valid targets for uprobe.
>> > Hence verify if the target is a regular file during the probe
>> > registration.
>>
>> Thanks srikar, your description is more clear than mine.
>>
>>
>> From fd5077196038cc271e2116e1fca359a0011e1669 Mon Sep 17 00:00:00 2001
>> From: Jovi Zhang
>> Date: Wed, 18 Jul 2012 18:16:44 +0800
>> Subject: [PATCH] tracing: Verify target file before registering a uprobe
>> event
>>
>> Without this patch, we can register a uprobe event for a directory.
>> Enabling such a uprobe event would anyway fail.
>>
>> Example:
>> $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
>>
>> However dirctories cannot be valid targets for uprobe.
>> Hence verify if the target is a regular file during the probe
>> registration.
>>
>> Signed-off-by: Jovi Zhang
>> Reviewed-by: Srikar Dronamraju
>> ---
>> kernel/trace/trace_uprobe.c |6 +-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>> index 85158fa..3b5f646 100644
>> --- a/kernel/trace/trace_uprobe.c
>> +++ b/kernel/trace/trace_uprobe.c
>> @@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
>> goto fail_address_parse;
>>
>> inode = igrab(path.dentry->d_inode);
>> + if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
>> + ret = -EINVAL;
>> + goto fail_address_parse;
>> + }
>>
>> argc -= 2;
>> argv += 2;
>> @@ -358,7 +362,7 @@ fail_address_parse:
>> if (inode)
>> iput(inode);
>>
>> - pr_info("Failed to parse address.\n");
>> + pr_info("Failed to parse address or file.\n");
>>
>> return ret;
>> }
>
> Looks good.
>
> Acked-by: Srikar Dronamraju
>
Hi Andrew,
Is this patch ok to go through your tree?
.jovi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobe: fix misleading log entry
On Wed, Jul 18, 2012 at 5:22 PM, Srikar Dronamraju
wrote:
> * Jovi Zhang [2012-07-18 11:08:42]:
>
>> From 68232ef2decae95b807f2f3763e8ea99c1a3b2ae Mon Sep 17 00:00:00 2001
>> From: Jovi Zhang
>> Date: Wed, 18 Jul 2012 17:51:26 +0800
>> Subject: [PATCH] uprobe: fix misleading log entry
>>
>> There don't have any 'r' prefix in uprobe event naming, remove it.
>>
>> Signed-off-by: Jovi Zhang
>> ---
>> kernel/trace/trace_uprobe.c |2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>> index cf382de..852a584 100644
>> --- a/kernel/trace/trace_uprobe.c
>> +++ b/kernel/trace/trace_uprobe.c
>> @@ -191,7 +191,7 @@ static int create_trace_uprobe(int argc, char **argv)
>> if (argv[0][0] == '-')
>> is_delete = true;
>> else if (argv[0][0] != 'p') {
>> - pr_info("Probe definition must be started with 'p', 'r' or" "
>> '-'.\n");
>> + pr_info("Probe definition must be started with 'p' or '-'.\n");
>> return -EINVAL;
>> }
>>
>
> Yes, uprobes doesnt support return probes. So we should not have
> mentioned about r.
>
> Acked-by: Srikar Dronamraju
>
Hi Andrew,
Is this patch ok to go through your mm tree? mainline still don't merge it.
.jovi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobe: fix misleading log entry
On Wed, Jul 18, 2012 at 5:22 PM, Srikar Dronamraju
sri...@linux.vnet.ibm.com wrote:
* Jovi Zhang bookj...@gmail.com [2012-07-18 11:08:42]:
From 68232ef2decae95b807f2f3763e8ea99c1a3b2ae Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Wed, 18 Jul 2012 17:51:26 +0800
Subject: [PATCH] uprobe: fix misleading log entry
There don't have any 'r' prefix in uprobe event naming, remove it.
Signed-off-by: Jovi Zhang bookj...@gmail.com
---
kernel/trace/trace_uprobe.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index cf382de..852a584 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -191,7 +191,7 @@ static int create_trace_uprobe(int argc, char **argv)
if (argv[0][0] == '-')
is_delete = true;
else if (argv[0][0] != 'p') {
- pr_info(Probe definition must be started with 'p', 'r' or
'-'.\n);
+ pr_info(Probe definition must be started with 'p' or '-'.\n);
return -EINVAL;
}
Yes, uprobes doesnt support return probes. So we should not have
mentioned about r.
Acked-by: Srikar Dronamraju sri...@linux.vnet.ibm.com
Hi Andrew,
Is this patch ok to go through your mm tree? mainline still don't merge it.
.jovi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: uprobe: checking probe event include directory
On Wed, Jul 18, 2012 at 7:45 PM, Srikar Dronamraju
sri...@linux.vnet.ibm.com wrote:
* Jovi Zhang bookj...@gmail.com [2012-07-18 19:38:27]:
On Wed, Jul 18, 2012 at 7:07 PM, Srikar Dronamraju
sri...@linux.vnet.ibm.com wrote:
The patch looks good,
Can you modify the description a bit. However you are free to ignore
these comments. After knowing your response, I will ack the patch.
I would probably put this as:
The subject could be
tracing: Verify target file before registering a uprobe event
Description:
Without this patch, we can register a uprobe event for a directory.
Enabling such a uprobe event would anyway fail.
Example:
$ echo 'p /bin:0x4245c0' /sys/kernel/debug/tracing/uprobe_events
However directories cannot be valid targets for uprobe.
Hence verify if the target is a regular file during the probe
registration.
Thanks srikar, your description is more clear than mine.
From fd5077196038cc271e2116e1fca359a0011e1669 Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Wed, 18 Jul 2012 18:16:44 +0800
Subject: [PATCH] tracing: Verify target file before registering a uprobe
event
Without this patch, we can register a uprobe event for a directory.
Enabling such a uprobe event would anyway fail.
Example:
$ echo 'p /bin:0x4245c0' /sys/kernel/debug/tracing/uprobe_events
However dirctories cannot be valid targets for uprobe.
Hence verify if the target is a regular file during the probe
registration.
Signed-off-by: Jovi Zhang bookj...@gmail.com
Reviewed-by: Srikar Dronamraju sri...@linux.vnet.ibm.com
---
kernel/trace/trace_uprobe.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 85158fa..3b5f646 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
goto fail_address_parse;
inode = igrab(path.dentry-d_inode);
+ if (!S_ISREG(inode-i_mode) || S_ISDIR(inode-i_mode)) {
+ ret = -EINVAL;
+ goto fail_address_parse;
+ }
argc -= 2;
argv += 2;
@@ -358,7 +362,7 @@ fail_address_parse:
if (inode)
iput(inode);
- pr_info(Failed to parse address.\n);
+ pr_info(Failed to parse address or file.\n);
return ret;
}
Looks good.
Acked-by: Srikar Dronamraju sri...@linux.vnet.ibm.com
Hi Andrew,
Is this patch ok to go through your tree?
.jovi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: powerpc/perf: hw breakpoints return ENOSPC
On Thu, Aug 16, 2012 at 12:23 PM, Michael Neuling wrote: > Hi, > > I've been trying to get hardware breakpoints with perf to work on POWER7 > but I'm getting the following: > > % perf record -e mem:0x1000 true > > Error: sys_perf_event_open() syscall returned with 28 (No space left on > device). /bin/dmesg may provide additional information. > > Fatal: No CONFIG_PERF_EVENTS=y kernel support configured? > > true: Terminated > > (FWIW adding -a and it works fine) > > Debugging it seems that __reserve_bp_slot() is returning ENOSPC because > it thinks there are no free breakpoint slots on this CPU. > > I have a 2 CPUs, so perf userspace is doing two perf_event_open syscalls > to add a counter to each CPU [1]. The first syscall succeeds but the > second is failing. > > On this second syscall, fetch_bp_busy_slots() sets slots.pinned to be 1, > despite there being no breakpoint on this CPU. This is because the call > the task_bp_pinned, checks all CPUs, rather than just the current CPU. > POWER7 only has one hardware breakpoint per CPU (ie. HBP_NUM=1), so we > return ENOSPC. > > The following patch fixes this by checking the associated CPU for each > breakpoint in task_bp_pinned. I'm not familiar with this code, so it's > provided as a reference to the above issue. > > Mikey > > 1. not sure why it doesn't just do one syscall and specify all CPUs, but > that's another issue. Using two syscalls should work. > This problem let me recall what I reported several months ago. https://lkml.org/lkml/2012/6/27/631 At that time, I thought it is caused by uses_mmap field in record sub command which added by commit d1cb9f(perf target: Add uses_mmap field). In that testcase, it's fine to use stat sub command, but failed with record sub command. As Namhyung metioned in that thread, [perf record xxx] use per-task-per-cpu for fix scalability issues. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: powerpc/perf: hw breakpoints return ENOSPC
On Thu, Aug 16, 2012 at 12:23 PM, Michael Neuling mi...@neuling.org wrote: Hi, I've been trying to get hardware breakpoints with perf to work on POWER7 but I'm getting the following: % perf record -e mem:0x1000 true Error: sys_perf_event_open() syscall returned with 28 (No space left on device). /bin/dmesg may provide additional information. Fatal: No CONFIG_PERF_EVENTS=y kernel support configured? true: Terminated (FWIW adding -a and it works fine) Debugging it seems that __reserve_bp_slot() is returning ENOSPC because it thinks there are no free breakpoint slots on this CPU. I have a 2 CPUs, so perf userspace is doing two perf_event_open syscalls to add a counter to each CPU [1]. The first syscall succeeds but the second is failing. On this second syscall, fetch_bp_busy_slots() sets slots.pinned to be 1, despite there being no breakpoint on this CPU. This is because the call the task_bp_pinned, checks all CPUs, rather than just the current CPU. POWER7 only has one hardware breakpoint per CPU (ie. HBP_NUM=1), so we return ENOSPC. The following patch fixes this by checking the associated CPU for each breakpoint in task_bp_pinned. I'm not familiar with this code, so it's provided as a reference to the above issue. Mikey 1. not sure why it doesn't just do one syscall and specify all CPUs, but that's another issue. Using two syscalls should work. This problem let me recall what I reported several months ago. https://lkml.org/lkml/2012/6/27/631 At that time, I thought it is caused by uses_mmap field in record sub command which added by commit d1cb9f(perf target: Add uses_mmap field). In that testcase, it's fine to use stat sub command, but failed with record sub command. As Namhyung metioned in that thread, [perf record xxx] use per-task-per-cpu for fix scalability issues. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] MIPS/mm: add compound tail page _mapcount when mapped
>From 3dc19ea2b535719d0b4177f17bbbff9cbf257b23 Mon Sep 17 00:00:00 2001
From: Jovi Zhang
Date: Wed, 22 Aug 2012 10:34:08 +0800
Subject: [PATCH] MIPS/mm: add compound tail page _mapcount when mapped
see commit b6999b191 which target for x86 mm/gup, let it align with
mips architecture.
Quote from commit b6999b191:
"If compound pages are used and the page is a
tail page, gup_huge_pmd() increases _mapcount to record tail page are
mapped while gup_huge_pud does not do that."
Signed-off-by: Jovi Zhang
Cc: Youquan Song
Cc: Andi Kleen
Cc:
---
arch/mips/mm/gup.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/mips/mm/gup.c b/arch/mips/mm/gup.c
index 33aadbc..dcfd573 100644
--- a/arch/mips/mm/gup.c
+++ b/arch/mips/mm/gup.c
@@ -152,6 +152,8 @@ static int gup_huge_pud(pud_t pud, unsigned long
addr, unsigned long end,
do {
VM_BUG_ON(compound_head(page) != head);
pages[*nr] = page;
+ if (PageTail(page))
+ get_huge_page_tail(page);
(*nr)++;
page++;
refs++;
--
1.7.9.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] MIPS/mm: add compound tail page _mapcount when mapped
From 3dc19ea2b535719d0b4177f17bbbff9cbf257b23 Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Wed, 22 Aug 2012 10:34:08 +0800
Subject: [PATCH] MIPS/mm: add compound tail page _mapcount when mapped
see commit b6999b191 which target for x86 mm/gup, let it align with
mips architecture.
Quote from commit b6999b191:
If compound pages are used and the page is a
tail page, gup_huge_pmd() increases _mapcount to record tail page are
mapped while gup_huge_pud does not do that.
Signed-off-by: Jovi Zhang booj...@gmail.com
Cc: Youquan Song youquan.s...@intel.com
Cc: Andi Kleen a...@firstfloor.org
Cc: sta...@vger.kernel.org
---
arch/mips/mm/gup.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/mips/mm/gup.c b/arch/mips/mm/gup.c
index 33aadbc..dcfd573 100644
--- a/arch/mips/mm/gup.c
+++ b/arch/mips/mm/gup.c
@@ -152,6 +152,8 @@ static int gup_huge_pud(pud_t pud, unsigned long
addr, unsigned long end,
do {
VM_BUG_ON(compound_head(page) != head);
pages[*nr] = page;
+ if (PageTail(page))
+ get_huge_page_tail(page);
(*nr)++;
page++;
refs++;
--
1.7.9.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] pstore: add missed platform_device_unregister
>From 152373a6262045d19023cf45de84ad3c69316a45 Mon Sep 17 00:00:00 2001
From: Jovi Zhang
Date: Mon, 20 Aug 2012 14:20:01 +0800
Subject: [PATCH] pstore: add missed platform_device_unregister
we need unregister platform device when module exit, add it.
Signed-off-by: Jovi Zhang
---
fs/pstore/ram.c |1 +
1 file changed, 1 insertion(+)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 0b311bc..adb218a 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -537,6 +537,7 @@ postcore_initcall(ramoops_init);
static void __exit ramoops_exit(void)
{
platform_driver_unregister(_driver);
+ platform_device_unregister(dummy);
kfree(dummy_data);
}
module_exit(ramoops_exit);
--
1.7.9.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] pstore: add missed platform_device_unregister
From 152373a6262045d19023cf45de84ad3c69316a45 Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Mon, 20 Aug 2012 14:20:01 +0800
Subject: [PATCH] pstore: add missed platform_device_unregister
we need unregister platform device when module exit, add it.
Signed-off-by: Jovi Zhang bookj...@gmail.com
---
fs/pstore/ram.c |1 +
1 file changed, 1 insertion(+)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 0b311bc..adb218a 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -537,6 +537,7 @@ postcore_initcall(ramoops_init);
static void __exit ramoops_exit(void)
{
platform_driver_unregister(ramoops_driver);
+ platform_device_unregister(dummy);
kfree(dummy_data);
}
module_exit(ramoops_exit);
--
1.7.9.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[tip:perf/core] perf/x86: Fix missing struct before structure name
Commit-ID: 35d56ca9d401d9d0ac8d91e4db1485af5f38f6fd
Gitweb: http://git.kernel.org/tip/35d56ca9d401d9d0ac8d91e4db1485af5f38f6fd
Author: Jovi Zhang
AuthorDate: Tue, 17 Jul 2012 10:14:41 +0800
Committer: Ingo Molnar
CommitDate: Thu, 26 Jul 2012 15:04:34 +0200
perf/x86: Fix missing struct before structure name
When CONFIG_PERF_EVENTS disabled, there will have a compiliation
error, because missing struct before structure name.
Signed-off-by: Jovi Zhang
Cc: Peter Zijlstra
Cc: Jiri Kosina
Link:
http://lkml.kernel.org/r/cacv3sbkf%3dcx%2b2jwewesfca6rboq3wdm4-5ac9mubtvbctmr...@mail.gmail.com
Signed-off-by: Ingo Molnar
---
arch/x86/include/asm/perf_event.h |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/include/asm/perf_event.h
b/arch/x86/include/asm/perf_event.h
index c78f14a..dab3935 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -234,7 +234,7 @@ extern struct perf_guest_switch_msr
*perf_guest_get_msrs(int *nr);
extern void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap);
extern void perf_check_microcode(void);
#else
-static inline perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
+static inline struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
{
*nr = 0;
return NULL;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[tip:perf/core] perf/x86: Fix missing struct before structure name
Commit-ID: 35d56ca9d401d9d0ac8d91e4db1485af5f38f6fd
Gitweb: http://git.kernel.org/tip/35d56ca9d401d9d0ac8d91e4db1485af5f38f6fd
Author: Jovi Zhang bookj...@gmail.com
AuthorDate: Tue, 17 Jul 2012 10:14:41 +0800
Committer: Ingo Molnar mi...@kernel.org
CommitDate: Thu, 26 Jul 2012 15:04:34 +0200
perf/x86: Fix missing struct before structure name
When CONFIG_PERF_EVENTS disabled, there will have a compiliation
error, because missing struct before structure name.
Signed-off-by: Jovi Zhang bookj...@gmail.com
Cc: Peter Zijlstra a.p.zijls...@chello.nl
Cc: Jiri Kosina triv...@kernel.org
Link:
http://lkml.kernel.org/r/cacv3sbkf%3dcx%2b2jwewesfca6rboq3wdm4-5ac9mubtvbctmr...@mail.gmail.com
Signed-off-by: Ingo Molnar mi...@kernel.org
---
arch/x86/include/asm/perf_event.h |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/include/asm/perf_event.h
b/arch/x86/include/asm/perf_event.h
index c78f14a..dab3935 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -234,7 +234,7 @@ extern struct perf_guest_switch_msr
*perf_guest_get_msrs(int *nr);
extern void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap);
extern void perf_check_microcode(void);
#else
-static inline perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
+static inline struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
{
*nr = 0;
return NULL;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[tip:perf/core] perf tools: Make the breakpoint events sample period default to 1
Commit-ID: 4a841d650ea435c69e60675537f158a620697290 Gitweb: http://git.kernel.org/tip/4a841d650ea435c69e60675537f158a620697290 Author: Jovi Zhang AuthorDate: Sun, 15 Jul 2012 03:03:10 +0800 Committer: Arnaldo Carvalho de Melo CommitDate: Wed, 25 Jul 2012 11:37:46 -0300 perf tools: Make the breakpoint events sample period default to 1 There have one problem about hw_breakpoint perf event, as watched, the events reported to userspace is not correctly, sometime one trigger bp_event report several events, sometime bp_event cannot go through to user. The root cause is attr->freq is 1 passed to kernel defaultly in bp events, this make kernel calculate event period not as expect, make sample period to 1 will change attr->freq to 0, to fix this problem. This patch is similar with commit f92128 about tracepoint events: perf: Make the trace events sample period default to 1 Signed-off-by: Jovi Zhang Cc: Ingo Molnar Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/cacv3sblf8taicq_vyw-sgrjyupemzg58c7zxfme3xzuih_m...@mail.gmail.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/parse-events.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index a729945..74a5af4 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -490,6 +490,7 @@ int parse_events_add_breakpoint(struct list_head **list, int *idx, attr.bp_len = HW_BREAKPOINT_LEN_4; attr.type = PERF_TYPE_BREAKPOINT; + attr.sample_period = 1; return add_event(list, idx, , NULL); } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[tip:perf/core] perf tools: Make the breakpoint events sample period default to 1
Commit-ID: 4a841d650ea435c69e60675537f158a620697290 Gitweb: http://git.kernel.org/tip/4a841d650ea435c69e60675537f158a620697290 Author: Jovi Zhang bookj...@gmail.com AuthorDate: Sun, 15 Jul 2012 03:03:10 +0800 Committer: Arnaldo Carvalho de Melo a...@redhat.com CommitDate: Wed, 25 Jul 2012 11:37:46 -0300 perf tools: Make the breakpoint events sample period default to 1 There have one problem about hw_breakpoint perf event, as watched, the events reported to userspace is not correctly, sometime one trigger bp_event report several events, sometime bp_event cannot go through to user. The root cause is attr-freq is 1 passed to kernel defaultly in bp events, this make kernel calculate event period not as expect, make sample period to 1 will change attr-freq to 0, to fix this problem. This patch is similar with commit f92128 about tracepoint events: perf: Make the trace events sample period default to 1 Signed-off-by: Jovi Zhang bookj...@gmail.com Cc: Ingo Molnar mi...@kernel.org Cc: Peter Zijlstra a.p.zijls...@chello.nl Link: http://lkml.kernel.org/r/cacv3sblf8taicq_vyw-sgrjyupemzg58c7zxfme3xzuih_m...@mail.gmail.com Signed-off-by: Arnaldo Carvalho de Melo a...@redhat.com --- tools/perf/util/parse-events.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index a729945..74a5af4 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -490,6 +490,7 @@ int parse_events_add_breakpoint(struct list_head **list, int *idx, attr.bp_len = HW_BREAKPOINT_LEN_4; attr.type = PERF_TYPE_BREAKPOINT; + attr.sample_period = 1; return add_event(list, idx, attr, NULL); } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobe: fix misleading log entry
On Wed, Jul 18, 2012 at 5:22 PM, Srikar Dronamraju
wrote:
> * Jovi Zhang [2012-07-18 11:08:42]:
>
>> From 68232ef2decae95b807f2f3763e8ea99c1a3b2ae Mon Sep 17 00:00:00 2001
>> From: Jovi Zhang
>> Date: Wed, 18 Jul 2012 17:51:26 +0800
>> Subject: [PATCH] uprobe: fix misleading log entry
>>
>> There don't have any 'r' prefix in uprobe event naming, remove it.
>>
>> Signed-off-by: Jovi Zhang
>> ---
>> kernel/trace/trace_uprobe.c |2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>> index cf382de..852a584 100644
>> --- a/kernel/trace/trace_uprobe.c
>> +++ b/kernel/trace/trace_uprobe.c
>> @@ -191,7 +191,7 @@ static int create_trace_uprobe(int argc, char **argv)
>> if (argv[0][0] == '-')
>> is_delete = true;
>> else if (argv[0][0] != 'p') {
>> - pr_info("Probe definition must be started with 'p', 'r' or" "
>> '-'.\n");
>> + pr_info("Probe definition must be started with 'p' or '-'.\n");
>> return -EINVAL;
>> }
>>
>
> Yes, uprobes doesnt support return probes. So we should not have
> mentioned about r.
Hmm, Does this have specific reason? or just not implemented?
>
> Acked-by: Srikar Dronamraju
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [offlist] uprobe: checking probe event include directory
On Wed, Jul 18, 2012 at 7:07 PM, Srikar Dronamraju
wrote:
> The patch looks good,
>
> Can you modify the description a bit. However you are free to ignore
> these comments. After knowing your response, I will ack the patch.
>
> I would probably put this as:
>
> The subject could be
> tracing: Verify target file before registering a uprobe event
>
> Description:
> Without this patch, we can register a uprobe event for a directory.
> Enabling such a uprobe event would anyway fail.
>
> Example:
>
> $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
>
> However directories cannot be valid targets for uprobe.
> Hence verify if the target is a regular file during the probe
> registration.
Thanks srikar, your description is more clear than mine.
>From fd5077196038cc271e2116e1fca359a0011e1669 Mon Sep 17 00:00:00 2001
From: Jovi Zhang
Date: Wed, 18 Jul 2012 18:16:44 +0800
Subject: [PATCH] tracing: Verify target file before registering a uprobe
event
Without this patch, we can register a uprobe event for a directory.
Enabling such a uprobe event would anyway fail.
Example:
$ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
However dirctories cannot be valid targets for uprobe.
Hence verify if the target is a regular file during the probe
registration.
Signed-off-by: Jovi Zhang
Reviewed-by: Srikar Dronamraju
---
kernel/trace/trace_uprobe.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 85158fa..3b5f646 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
goto fail_address_parse;
inode = igrab(path.dentry->d_inode);
+if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
+ ret = -EINVAL;
+ goto fail_address_parse;
+ }
argc -= 2;
argv += 2;
@@ -358,7 +362,7 @@ fail_address_parse:
if (inode)
iput(inode);
- pr_info("Failed to parse address.\n");
+ pr_info("Failed to parse address or file.\n");
return ret;
}
--
1.7.9.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [offlist] uprobe: checking probe event include directory
On Wed, Jul 18, 2012 at 7:07 PM, Srikar Dronamraju
sri...@linux.vnet.ibm.com wrote:
The patch looks good,
Can you modify the description a bit. However you are free to ignore
these comments. After knowing your response, I will ack the patch.
I would probably put this as:
The subject could be
tracing: Verify target file before registering a uprobe event
Description:
Without this patch, we can register a uprobe event for a directory.
Enabling such a uprobe event would anyway fail.
Example:
$ echo 'p /bin:0x4245c0' /sys/kernel/debug/tracing/uprobe_events
However directories cannot be valid targets for uprobe.
Hence verify if the target is a regular file during the probe
registration.
Thanks srikar, your description is more clear than mine.
From fd5077196038cc271e2116e1fca359a0011e1669 Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Wed, 18 Jul 2012 18:16:44 +0800
Subject: [PATCH] tracing: Verify target file before registering a uprobe
event
Without this patch, we can register a uprobe event for a directory.
Enabling such a uprobe event would anyway fail.
Example:
$ echo 'p /bin:0x4245c0' /sys/kernel/debug/tracing/uprobe_events
However dirctories cannot be valid targets for uprobe.
Hence verify if the target is a regular file during the probe
registration.
Signed-off-by: Jovi Zhang bookj...@gmail.com
Reviewed-by: Srikar Dronamraju sri...@linux.vnet.ibm.com
---
kernel/trace/trace_uprobe.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 85158fa..3b5f646 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
goto fail_address_parse;
inode = igrab(path.dentry-d_inode);
+if (!S_ISREG(inode-i_mode) || S_ISDIR(inode-i_mode)) {
+ ret = -EINVAL;
+ goto fail_address_parse;
+ }
argc -= 2;
argv += 2;
@@ -358,7 +362,7 @@ fail_address_parse:
if (inode)
iput(inode);
- pr_info(Failed to parse address.\n);
+ pr_info(Failed to parse address or file.\n);
return ret;
}
--
1.7.9.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobe: fix misleading log entry
On Wed, Jul 18, 2012 at 5:22 PM, Srikar Dronamraju
sri...@linux.vnet.ibm.com wrote:
* Jovi Zhang bookj...@gmail.com [2012-07-18 11:08:42]:
From 68232ef2decae95b807f2f3763e8ea99c1a3b2ae Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Wed, 18 Jul 2012 17:51:26 +0800
Subject: [PATCH] uprobe: fix misleading log entry
There don't have any 'r' prefix in uprobe event naming, remove it.
Signed-off-by: Jovi Zhang bookj...@gmail.com
---
kernel/trace/trace_uprobe.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index cf382de..852a584 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -191,7 +191,7 @@ static int create_trace_uprobe(int argc, char **argv)
if (argv[0][0] == '-')
is_delete = true;
else if (argv[0][0] != 'p') {
- pr_info(Probe definition must be started with 'p', 'r' or
'-'.\n);
+ pr_info(Probe definition must be started with 'p' or '-'.\n);
return -EINVAL;
}
Yes, uprobes doesnt support return probes. So we should not have
mentioned about r.
Hmm, Does this have specific reason? or just not implemented?
Acked-by: Srikar Dronamraju sri...@linux.vnet.ibm.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] uprobe: fix misleading log entry
>From 68232ef2decae95b807f2f3763e8ea99c1a3b2ae Mon Sep 17 00:00:00 2001
From: Jovi Zhang
Date: Wed, 18 Jul 2012 17:51:26 +0800
Subject: [PATCH] uprobe: fix misleading log entry
There don't have any 'r' prefix in uprobe event naming, remove it.
Signed-off-by: Jovi Zhang
---
kernel/trace/trace_uprobe.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index cf382de..852a584 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -191,7 +191,7 @@ static int create_trace_uprobe(int argc, char **argv)
if (argv[0][0] == '-')
is_delete = true;
else if (argv[0][0] != 'p') {
- pr_info("Probe definition must be started with 'p', 'r' or" "
'-'.\n");
+ pr_info("Probe definition must be started with 'p' or '-'.\n");
return -EINVAL;
}
--
1.7.9.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: uprobe: checking probe event include directory
On Wed, Jul 18, 2012 at 1:27 AM, Srikar Dronamraju
wrote:
> * Frederic Weisbecker [2012-07-17 12:59:39]:
>
>> On Tue, Jul 17, 2012 at 06:12:28PM +0800, Jovi Zhang wrote:
>> > From 16ed13ee9098ae01705e8456005d1ad6d9909128 Mon Sep 17 00:00:00 2001
>> > From: Jovi Zhang
>> > Date: Wed, 18 Jul 2012 01:16:23 +0800
>> > Subject: [PATCH] uprobe: checking probe event include directory
>> >
>> > Currently below command run successful:
>> > $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
>
> good catch.
>
>> >
>> > this don't make sense, because /bin is a directory,
>> > so make this checking earily, not report error untill probe enable.
>> >
>> > Signed-off-by: Jovi Zhang
>>
>> Adding Srikar in Cc.
>>
>> > ---
>> > kernel/trace/trace_uprobe.c |5 +
>> > 1 file changed, 5 insertions(+)
>> >
>> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>> > index 85158fa..cf382de 100644
>> > --- a/kernel/trace/trace_uprobe.c
>> > +++ b/kernel/trace/trace_uprobe.c
>> > @@ -259,6 +259,11 @@ static int create_trace_uprobe(int argc, char **argv)
>> > goto fail_address_parse;
>> >
>> > inode = igrab(path.dentry->d_inode);
>> > +if (S_ISDIR(inode->i_mode)) {
>
> How about checking for regular files but not directory.
> i.e it should avoid tracing special files
>
> Something like:
>
> if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
>
>> > + ret = -EINVAL;
>> > + pr_info("probe file cannot be directory.\n");
>
> we could drop the pr_info line here, since we would any print we
> failed to parse the address.
>
> Probably you could change the last pr_info from
>
> pr_info("Failed to parse address.\n");
>
> to
>
> pr_info("Failed to parse address or file.\n");
>
>
Thanks srikar, try below patch:
>From dd37d3cc563e2619ec325d6c11d769a32def411b Mon Sep 17 00:00:00 2001
From: Jovi Zhang
Date: Wed, 18 Jul 2012 18:16:44 +0800
Subject: [PATCH] uprobe: checking uprobe event inode valid
Currently below command run successful:
$ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
this don't make sense, because /bin is a directory,
we need to check uprobe event inode valid more earily.
Signed-off-by: Jovi Zhang
Reviewed-by: Srikar Dronamraju
---
kernel/trace/trace_uprobe.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 85158fa..3b5f646 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
goto fail_address_parse;
inode = igrab(path.dentry->d_inode);
+if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
+ ret = -EINVAL;
+ goto fail_address_parse;
+ }
argc -= 2;
argv += 2;
@@ -358,7 +362,7 @@ fail_address_parse:
if (inode)
iput(inode);
- pr_info("Failed to parse address.\n");
+ pr_info("Failed to parse address or file.\n");
return ret;
}
--
1.7.9.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ftrace: using pr_fmt for better printk output
>From cea5f76c3ad9f42b85a1a71b75035fe96317187a Mon Sep 17 00:00:00 2001
From: Jovi Zhang
Date: Tue, 17 Jul 2012 22:43:11 +0800
Subject: [PATCH] ftrace: using pr_fmt for better printk output
There don't have subsystem name output in front ot ftrace related log entry,
so use pr_fmt to enable better printk output, for output subsystem name in
log entry.
Signed-off-by: Jovi Zhang
---
kernel/trace/blktrace.c |2 ++
kernel/trace/ftrace.c| 11 ++-
kernel/trace/trace.c |5 -
kernel/trace/trace_events.c |2 ++
kernel/trace/trace_functions_graph.c |9 ++---
kernel/trace/trace_kprobe.c |2 ++
kernel/trace/trace_mmiotrace.c |2 ++
kernel/trace/trace_probe.c |2 ++
kernel/trace/trace_selftest.c|6 +++---
kernel/trace/trace_syscalls.c|8
kernel/trace/trace_uprobe.c |2 ++
11 files changed, 35 insertions(+), 16 deletions(-)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index c0bd030..65c521f 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -15,6 +15,8 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include
#include
#include
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a008663..1198ebd 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -13,6 +13,8 @@
* Copyright (C) 2004 William Lee Irwin III
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include
#include
#include
@@ -2178,7 +2180,7 @@ ftrace_allocate_pages(unsigned long num_to_init)
kfree(pg);
pg = start_pg;
}
- pr_info("ftrace: FAILED to allocate memory for functions\n");
+ pr_info("FAILED to allocate memory for functions\n");
return NULL;
}
@@ -2187,12 +2189,12 @@ static int __init
ftrace_dyn_table_alloc(unsigned long num_to_init)
int cnt;
if (!num_to_init) {
- pr_info("ftrace: No functions to be traced?\n");
+ pr_info("No functions to be traced?\n");
return -1;
}
cnt = num_to_init / ENTRIES_PER_PAGE;
- pr_info("ftrace: allocating %ld entries in %d pages\n",
+ pr_info("allocating %ld entries in %d pages\n",
num_to_init, cnt + 1);
return 0;
@@ -4495,8 +4497,7 @@ static int start_graph_tracing(void)
if (!ret) {
ret =
register_trace_sched_switch(ftrace_graph_probe_sched_switch, NULL);
if (ret)
- pr_info("ftrace_graph: Couldn't activate tracepoint"
- " probe to kernel_sched_switch\n");
+ pr_info("Couldn't activate tracepoint probe to
kernel_sched_switch\n");
}
kfree(ret_stack_list);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index a7fa070..265f4a5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -11,6 +11,9 @@
* Copyright (C) 2004-2006 Ingo Molnar
* Copyright (C) 2004 William Lee Irwin III
*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include
#include
#include
@@ -1566,7 +1569,7 @@ void trace_printk_init_buffers(void)
if (alloc_percpu_trace_buffer())
return;
- pr_info("ftrace: Allocated trace_printk buffers\n");
+ pr_info("Allocated trace_printk buffers\n");
buffers_allocated = 1;
}
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 29111da..11dd55c 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -8,6 +8,8 @@
*
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include
#include
#include
diff --git a/kernel/trace/trace_functions_graph.c
b/kernel/trace/trace_functions_graph.c
index a7d2a4c..a2be82b 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -6,6 +6,9 @@
* is Copyright (c) Steven Rostedt
*
*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include
#include
#include
@@ -1401,7 +1404,7 @@ void graph_trace_open(struct trace_iterator *iter)
out_err_free:
kfree(data);
out_err:
- pr_warning("function graph tracer: not enough memory\n");
+ pr_warn("not enough memory\n");
}
void graph_trace_close(struct trace_iterator *iter)
@@ -1459,12 +1462,12 @@ static __init int init_graph_trace(void)
max_bytes_for_cpu = snprintf(NULL, 0, "%d", nr_cpu_ids - 1);
if (!register_ftrace_event(_trace_entry_event)) {
- pr_warning("Warning: could not register graph trace events\n");
+ pr_warn("Warning: could not register graph trace
Re: [PATCH] ftrace: using pr_fmt for better printk output
From cea5f76c3ad9f42b85a1a71b75035fe96317187a Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Tue, 17 Jul 2012 22:43:11 +0800
Subject: [PATCH] ftrace: using pr_fmt for better printk output
There don't have subsystem name output in front ot ftrace related log entry,
so use pr_fmt to enable better printk output, for output subsystem name in
log entry.
Signed-off-by: Jovi Zhang bookj...@gmail.com
---
kernel/trace/blktrace.c |2 ++
kernel/trace/ftrace.c| 11 ++-
kernel/trace/trace.c |5 -
kernel/trace/trace_events.c |2 ++
kernel/trace/trace_functions_graph.c |9 ++---
kernel/trace/trace_kprobe.c |2 ++
kernel/trace/trace_mmiotrace.c |2 ++
kernel/trace/trace_probe.c |2 ++
kernel/trace/trace_selftest.c|6 +++---
kernel/trace/trace_syscalls.c|8
kernel/trace/trace_uprobe.c |2 ++
11 files changed, 35 insertions(+), 16 deletions(-)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index c0bd030..65c521f 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -15,6 +15,8 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
*/
+#define pr_fmt(fmt) KBUILD_MODNAME : fmt
+
#include linux/kernel.h
#include linux/blkdev.h
#include linux/blktrace_api.h
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a008663..1198ebd 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -13,6 +13,8 @@
* Copyright (C) 2004 William Lee Irwin III
*/
+#define pr_fmt(fmt) KBUILD_MODNAME : fmt
+
#include linux/stop_machine.h
#include linux/clocksource.h
#include linux/kallsyms.h
@@ -2178,7 +2180,7 @@ ftrace_allocate_pages(unsigned long num_to_init)
kfree(pg);
pg = start_pg;
}
- pr_info(ftrace: FAILED to allocate memory for functions\n);
+ pr_info(FAILED to allocate memory for functions\n);
return NULL;
}
@@ -2187,12 +2189,12 @@ static int __init
ftrace_dyn_table_alloc(unsigned long num_to_init)
int cnt;
if (!num_to_init) {
- pr_info(ftrace: No functions to be traced?\n);
+ pr_info(No functions to be traced?\n);
return -1;
}
cnt = num_to_init / ENTRIES_PER_PAGE;
- pr_info(ftrace: allocating %ld entries in %d pages\n,
+ pr_info(allocating %ld entries in %d pages\n,
num_to_init, cnt + 1);
return 0;
@@ -4495,8 +4497,7 @@ static int start_graph_tracing(void)
if (!ret) {
ret =
register_trace_sched_switch(ftrace_graph_probe_sched_switch, NULL);
if (ret)
- pr_info(ftrace_graph: Couldn't activate tracepoint
-probe to kernel_sched_switch\n);
+ pr_info(Couldn't activate tracepoint probe to
kernel_sched_switch\n);
}
kfree(ret_stack_list);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index a7fa070..265f4a5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -11,6 +11,9 @@
* Copyright (C) 2004-2006 Ingo Molnar
* Copyright (C) 2004 William Lee Irwin III
*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME : fmt
+
#include linux/ring_buffer.h
#include generated/utsrelease.h
#include linux/stacktrace.h
@@ -1566,7 +1569,7 @@ void trace_printk_init_buffers(void)
if (alloc_percpu_trace_buffer())
return;
- pr_info(ftrace: Allocated trace_printk buffers\n);
+ pr_info(Allocated trace_printk buffers\n);
buffers_allocated = 1;
}
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 29111da..11dd55c 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -8,6 +8,8 @@
*
*/
+#define pr_fmt(fmt) KBUILD_MODNAME : fmt
+
#include linux/workqueue.h
#include linux/spinlock.h
#include linux/kthread.h
diff --git a/kernel/trace/trace_functions_graph.c
b/kernel/trace/trace_functions_graph.c
index a7d2a4c..a2be82b 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -6,6 +6,9 @@
* is Copyright (c) Steven Rostedt srost...@redhat.com
*
*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME : fmt
+
#include linux/debugfs.h
#include linux/uaccess.h
#include linux/ftrace.h
@@ -1401,7 +1404,7 @@ void graph_trace_open(struct trace_iterator *iter)
out_err_free:
kfree(data);
out_err:
- pr_warning(function graph tracer: not enough memory\n);
+ pr_warn(not enough memory\n);
}
void graph_trace_close(struct trace_iterator *iter)
@@ -1459,12 +1462,12 @@ static __init int init_graph_trace(void)
max_bytes_for_cpu = snprintf(NULL, 0, %d, nr_cpu_ids - 1);
if (!register_ftrace_event(graph_trace_entry_event)) {
- pr_warning(Warning: could not register graph trace events\n
Re: uprobe: checking probe event include directory
On Wed, Jul 18, 2012 at 1:27 AM, Srikar Dronamraju
sri...@linux.vnet.ibm.com wrote:
* Frederic Weisbecker fweis...@gmail.com [2012-07-17 12:59:39]:
On Tue, Jul 17, 2012 at 06:12:28PM +0800, Jovi Zhang wrote:
From 16ed13ee9098ae01705e8456005d1ad6d9909128 Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Wed, 18 Jul 2012 01:16:23 +0800
Subject: [PATCH] uprobe: checking probe event include directory
Currently below command run successful:
$ echo 'p /bin:0x4245c0' /sys/kernel/debug/tracing/uprobe_events
good catch.
this don't make sense, because /bin is a directory,
so make this checking earily, not report error untill probe enable.
Signed-off-by: Jovi Zhang bookj...@gmail.com
Adding Srikar in Cc.
---
kernel/trace/trace_uprobe.c |5 +
1 file changed, 5 insertions(+)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 85158fa..cf382de 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -259,6 +259,11 @@ static int create_trace_uprobe(int argc, char **argv)
goto fail_address_parse;
inode = igrab(path.dentry-d_inode);
+if (S_ISDIR(inode-i_mode)) {
How about checking for regular files but not directory.
i.e it should avoid tracing special files
Something like:
if (!S_ISREG(inode-i_mode) || S_ISDIR(inode-i_mode)) {
+ ret = -EINVAL;
+ pr_info(probe file cannot be directory.\n);
we could drop the pr_info line here, since we would any print we
failed to parse the address.
Probably you could change the last pr_info from
pr_info(Failed to parse address.\n);
to
pr_info(Failed to parse address or file.\n);
Thanks srikar, try below patch:
From dd37d3cc563e2619ec325d6c11d769a32def411b Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Wed, 18 Jul 2012 18:16:44 +0800
Subject: [PATCH] uprobe: checking uprobe event inode valid
Currently below command run successful:
$ echo 'p /bin:0x4245c0' /sys/kernel/debug/tracing/uprobe_events
this don't make sense, because /bin is a directory,
we need to check uprobe event inode valid more earily.
Signed-off-by: Jovi Zhang bookj...@gmail.com
Reviewed-by: Srikar Dronamraju sri...@linux.vnet.ibm.com
---
kernel/trace/trace_uprobe.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 85158fa..3b5f646 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
goto fail_address_parse;
inode = igrab(path.dentry-d_inode);
+if (!S_ISREG(inode-i_mode) || S_ISDIR(inode-i_mode)) {
+ ret = -EINVAL;
+ goto fail_address_parse;
+ }
argc -= 2;
argv += 2;
@@ -358,7 +362,7 @@ fail_address_parse:
if (inode)
iput(inode);
- pr_info(Failed to parse address.\n);
+ pr_info(Failed to parse address or file.\n);
return ret;
}
--
1.7.9.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] uprobe: fix misleading log entry
From 68232ef2decae95b807f2f3763e8ea99c1a3b2ae Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Wed, 18 Jul 2012 17:51:26 +0800
Subject: [PATCH] uprobe: fix misleading log entry
There don't have any 'r' prefix in uprobe event naming, remove it.
Signed-off-by: Jovi Zhang bookj...@gmail.com
---
kernel/trace/trace_uprobe.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index cf382de..852a584 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -191,7 +191,7 @@ static int create_trace_uprobe(int argc, char **argv)
if (argv[0][0] == '-')
is_delete = true;
else if (argv[0][0] != 'p') {
- pr_info(Probe definition must be started with 'p', 'r' or
'-'.\n);
+ pr_info(Probe definition must be started with 'p' or '-'.\n);
return -EINVAL;
}
--
1.7.9.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ftrace: using pr_fmt for better printk output
On Tue, Jul 17, 2012 at 1:07 PM, Joe Perches wrote: > On Tue, 2012-07-17 at 00:25 -0400, Steven Rostedt wrote: >> On Mon, 2012-07-16 at 20:42 -0700, Joe Perches wrote: >> >> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c >> > [] >> > > @@ -13,6 +13,8 @@ >> > > * Copyright (C) 2004 William Lee Irwin III >> > > */ >> > > >> > > +#define pr_fmt(fmt) "ftrace: " fmt >> > >> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> >> Wouldn't a nicer patch be to move this into a header file and then >> remove all the defines throughout the kernel tree? > > Maybe. There are modules that use common header files > like you suggest. It does mean that header must be the > #included before any other #include that might > #include or printk.h. > > Right now, if pr_fmt isn't #defined, printk.h > has a default definition of: > > #ifndef pr_fmt > #define pr_fmt(fmt) fmt > #endif > > My goal is to change that to: > > #ifndef pr_fmt > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > #endif > > in 3.8 (maybe 3.7) and remove all of these then > useless, duplicate #defines shortly afterward. > > https://lkml.org/lkml/2012/3/27/247 > >> Also, what is KBUILD_MODNAME defined as for non-modules? As ftrace is >> not a module. > > It depends on the Makefile. > > scripts/Makefile.lib:# $(modname_flags) #defines KBUILD_MODNAME as the name > of the module it will > scripts/Makefile.lib-# end up in (or would, if it gets compiled in) > scripts/Makefile.lib-# Note: Files that end up in two or more modules are > compiled without the > scripts/Makefile.lib:# KBUILD_MODNAME definition. The reason is that > any made-up name would > scripts/Makefile.lib-# differ in different configs. > scripts/Makefile.lib-name-fix = $(subst $(comma),_,$(subst -,_,$1)) > scripts/Makefile.lib-basename_flags = -D"KBUILD_BASENAME=KBUILD_STR($(call > name-fix,$(basetarget)))" > scripts/Makefile.lib-modname_flags = $(if $(filter 1,$(words $(modname))),\ > scripts/Makefile.lib: -D"KBUILD_MODNAME=KBUILD_STR($(call > name-fix,$(modname)))") > Hmm, that would make sense, get subsystem name from Makefile. Joe, there will delete all this pr_fmt definition in .c file in 3.8(or 3.7) as you metioned, so we can ingnore this patch. .Jovi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ftrace: using pr_fmt for better printk output
On Tue, Jul 17, 2012 at 12:25 PM, Steven Rostedt wrote: > On Mon, 2012-07-16 at 20:42 -0700, Joe Perches wrote: > >> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c >> [] >> > @@ -13,6 +13,8 @@ >> > * Copyright (C) 2004 William Lee Irwin III >> > */ >> > >> > +#define pr_fmt(fmt) "ftrace: " fmt >> >> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > Wouldn't a nicer patch be to move this into a header file and then > remove all the defines throughout the kernel tree? Maybe it's hard to achieve that. subsystem name is unique with each other, it should be visible in source file, if include into header file, then each .c file might need a own header file for include pr_fmt definition, then that header file cannot be reusable(avoid subsystem name conflicts). > > Also, what is KBUILD_MODNAME defined as for non-modules? As ftrace is > not a module. Yes, that's why I cannot use KBUILD_MODNAME in patch. > > -- Steve I don't make sure if there have some method or skill to let GCC knows subsystem name automatically, use built-in macro __FILE__? but this need condition of subsystem name is same as file name, not so easily to guarantee that. .jovi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [TRIVIAL] perf: missing struct before structure name
>From 3abcb73682893ed2bde318d17f1cc3430bf70224 Mon Sep 17 00:00:00 2001
From: Jovi Zhang
Date: Tue, 17 Jul 2012 17:21:56 +0800
Subject: [PATCH] [TRIVIAL] perf: missing struct before structure name
when CONFIG_PERF_EVENTS disabled, there will have a compiliation error,
because missing struct before structure name.
Signed-off-by: Jovi Zhang
---
arch/x86/include/asm/perf_event.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/perf_event.h
b/arch/x86/include/asm/perf_event.h
index 588f52e..2643877 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -235,7 +235,7 @@ struct perf_guest_switch_msr {
extern struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr);
extern void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap);
#else
-static inline perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
+static inline struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
{
*nr = 0;
return NULL;
--
1.7.9.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ftrace: using pr_fmt for better printk output
>From fe42b2f29e5968482b3129c71f81a58a0559cf04 Mon Sep 17 00:00:00 2001
From: Jovi Zhang
Date: Tue, 17 Jul 2012 17:10:15 +0800
Subject: [PATCH] ftrace: using pr_fmt for better printk output
There don't have subsystem name output in front ot ftrace related log entry,
so use pr_fmt to enable better printk output, for output subsystem name in
log entry.
Signed-off-by: Jovi Zhang
---
kernel/trace/blktrace.c |2 ++
kernel/trace/ftrace.c| 10 ++
kernel/trace/trace.c |5 -
kernel/trace/trace_events.c |2 ++
kernel/trace/trace_functions_graph.c |9 ++---
kernel/trace/trace_kprobe.c |2 ++
kernel/trace/trace_mmiotrace.c |2 ++
kernel/trace/trace_probe.c |2 ++
kernel/trace/trace_selftest.c|6 +++---
kernel/trace/trace_syscalls.c|9 +
kernel/trace/trace_uprobe.c |2 ++
11 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index c0bd030..940b5d7 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -15,6 +15,8 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
*/
+#define pr_fmt(fmt) "blktrace: " fmt
+
#include
#include
#include
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a008663..c138ad7 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -13,6 +13,8 @@
* Copyright (C) 2004 William Lee Irwin III
*/
+#define pr_fmt(fmt) "ftrace: " fmt
+
#include
#include
#include
@@ -2178,7 +2180,7 @@ ftrace_allocate_pages(unsigned long num_to_init)
kfree(pg);
pg = start_pg;
}
- pr_info("ftrace: FAILED to allocate memory for functions\n");
+ pr_info("FAILED to allocate memory for functions\n");
return NULL;
}
@@ -2187,12 +2189,12 @@ static int __init
ftrace_dyn_table_alloc(unsigned long num_to_init)
int cnt;
if (!num_to_init) {
- pr_info("ftrace: No functions to be traced?\n");
+ pr_info("No functions to be traced?\n");
return -1;
}
cnt = num_to_init / ENTRIES_PER_PAGE;
- pr_info("ftrace: allocating %ld entries in %d pages\n",
+ pr_info("allocating %ld entries in %d pages\n",
num_to_init, cnt + 1);
return 0;
@@ -4495,7 +4497,7 @@ static int start_graph_tracing(void)
if (!ret) {
ret =
register_trace_sched_switch(ftrace_graph_probe_sched_switch, NULL);
if (ret)
- pr_info("ftrace_graph: Couldn't activate tracepoint"
+ pr_info("Couldn't activate tracepoint"
" probe to kernel_sched_switch\n");
}
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index a7fa070..dc06661 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -11,6 +11,9 @@
* Copyright (C) 2004-2006 Ingo Molnar
* Copyright (C) 2004 William Lee Irwin III
*/
+
+#define pr_fmt(fmt) "trace: " fmt
+
#include
#include
#include
@@ -1566,7 +1569,7 @@ void trace_printk_init_buffers(void)
if (alloc_percpu_trace_buffer())
return;
- pr_info("ftrace: Allocated trace_printk buffers\n");
+ pr_info("Allocated trace_printk buffers\n");
buffers_allocated = 1;
}
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 29111da..0abdf37 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -8,6 +8,8 @@
*
*/
+#define pr_fmt(fmt) "trace_events: " fmt
+
#include
#include
#include
diff --git a/kernel/trace/trace_functions_graph.c
b/kernel/trace/trace_functions_graph.c
index a7d2a4c..97ed861 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -6,6 +6,9 @@
* is Copyright (c) Steven Rostedt
*
*/
+
+#define pr_fmt(fmt) "function graph tracer: " fmt
+
#include
#include
#include
@@ -1401,7 +1404,7 @@ void graph_trace_open(struct trace_iterator *iter)
out_err_free:
kfree(data);
out_err:
- pr_warning("function graph tracer: not enough memory\n");
+ pr_warn("not enough memory\n");
}
void graph_trace_close(struct trace_iterator *iter)
@@ -1459,12 +1462,12 @@ static __init int init_graph_trace(void)
max_bytes_for_cpu = snprintf(NULL, 0, "%d", nr_cpu_ids - 1);
if (!register_ftrace_event(_trace_entry_event)) {
- pr_warning("Warning: could not register graph trace events\n");
+ pr_warn("Warning: could not register graph trace events\n");
return 1;
}
if (!register_ftrace_event(_trace
[PATCH] ftrace: using pr_fmt for better printk output
From fe42b2f29e5968482b3129c71f81a58a0559cf04 Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Tue, 17 Jul 2012 17:10:15 +0800
Subject: [PATCH] ftrace: using pr_fmt for better printk output
There don't have subsystem name output in front ot ftrace related log entry,
so use pr_fmt to enable better printk output, for output subsystem name in
log entry.
Signed-off-by: Jovi Zhang bookj...@gmail.com
---
kernel/trace/blktrace.c |2 ++
kernel/trace/ftrace.c| 10 ++
kernel/trace/trace.c |5 -
kernel/trace/trace_events.c |2 ++
kernel/trace/trace_functions_graph.c |9 ++---
kernel/trace/trace_kprobe.c |2 ++
kernel/trace/trace_mmiotrace.c |2 ++
kernel/trace/trace_probe.c |2 ++
kernel/trace/trace_selftest.c|6 +++---
kernel/trace/trace_syscalls.c|9 +
kernel/trace/trace_uprobe.c |2 ++
11 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index c0bd030..940b5d7 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -15,6 +15,8 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
*/
+#define pr_fmt(fmt) blktrace: fmt
+
#include linux/kernel.h
#include linux/blkdev.h
#include linux/blktrace_api.h
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a008663..c138ad7 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -13,6 +13,8 @@
* Copyright (C) 2004 William Lee Irwin III
*/
+#define pr_fmt(fmt) ftrace: fmt
+
#include linux/stop_machine.h
#include linux/clocksource.h
#include linux/kallsyms.h
@@ -2178,7 +2180,7 @@ ftrace_allocate_pages(unsigned long num_to_init)
kfree(pg);
pg = start_pg;
}
- pr_info(ftrace: FAILED to allocate memory for functions\n);
+ pr_info(FAILED to allocate memory for functions\n);
return NULL;
}
@@ -2187,12 +2189,12 @@ static int __init
ftrace_dyn_table_alloc(unsigned long num_to_init)
int cnt;
if (!num_to_init) {
- pr_info(ftrace: No functions to be traced?\n);
+ pr_info(No functions to be traced?\n);
return -1;
}
cnt = num_to_init / ENTRIES_PER_PAGE;
- pr_info(ftrace: allocating %ld entries in %d pages\n,
+ pr_info(allocating %ld entries in %d pages\n,
num_to_init, cnt + 1);
return 0;
@@ -4495,7 +4497,7 @@ static int start_graph_tracing(void)
if (!ret) {
ret =
register_trace_sched_switch(ftrace_graph_probe_sched_switch, NULL);
if (ret)
- pr_info(ftrace_graph: Couldn't activate tracepoint
+ pr_info(Couldn't activate tracepoint
probe to kernel_sched_switch\n);
}
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index a7fa070..dc06661 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -11,6 +11,9 @@
* Copyright (C) 2004-2006 Ingo Molnar
* Copyright (C) 2004 William Lee Irwin III
*/
+
+#define pr_fmt(fmt) trace: fmt
+
#include linux/ring_buffer.h
#include generated/utsrelease.h
#include linux/stacktrace.h
@@ -1566,7 +1569,7 @@ void trace_printk_init_buffers(void)
if (alloc_percpu_trace_buffer())
return;
- pr_info(ftrace: Allocated trace_printk buffers\n);
+ pr_info(Allocated trace_printk buffers\n);
buffers_allocated = 1;
}
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 29111da..0abdf37 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -8,6 +8,8 @@
*
*/
+#define pr_fmt(fmt) trace_events: fmt
+
#include linux/workqueue.h
#include linux/spinlock.h
#include linux/kthread.h
diff --git a/kernel/trace/trace_functions_graph.c
b/kernel/trace/trace_functions_graph.c
index a7d2a4c..97ed861 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -6,6 +6,9 @@
* is Copyright (c) Steven Rostedt srost...@redhat.com
*
*/
+
+#define pr_fmt(fmt) function graph tracer: fmt
+
#include linux/debugfs.h
#include linux/uaccess.h
#include linux/ftrace.h
@@ -1401,7 +1404,7 @@ void graph_trace_open(struct trace_iterator *iter)
out_err_free:
kfree(data);
out_err:
- pr_warning(function graph tracer: not enough memory\n);
+ pr_warn(not enough memory\n);
}
void graph_trace_close(struct trace_iterator *iter)
@@ -1459,12 +1462,12 @@ static __init int init_graph_trace(void)
max_bytes_for_cpu = snprintf(NULL, 0, %d, nr_cpu_ids - 1);
if (!register_ftrace_event(graph_trace_entry_event)) {
- pr_warning(Warning: could not register graph trace events\n);
+ pr_warn(Warning: could not register graph trace events\n
[PATCH] [TRIVIAL] perf: missing struct before structure name
From 3abcb73682893ed2bde318d17f1cc3430bf70224 Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Tue, 17 Jul 2012 17:21:56 +0800
Subject: [PATCH] [TRIVIAL] perf: missing struct before structure name
when CONFIG_PERF_EVENTS disabled, there will have a compiliation error,
because missing struct before structure name.
Signed-off-by: Jovi Zhang bookj...@gmail.com
---
arch/x86/include/asm/perf_event.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/perf_event.h
b/arch/x86/include/asm/perf_event.h
index 588f52e..2643877 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -235,7 +235,7 @@ struct perf_guest_switch_msr {
extern struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr);
extern void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap);
#else
-static inline perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
+static inline struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
{
*nr = 0;
return NULL;
--
1.7.9.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ftrace: using pr_fmt for better printk output
On Tue, Jul 17, 2012 at 12:25 PM, Steven Rostedt rost...@goodmis.org wrote: On Mon, 2012-07-16 at 20:42 -0700, Joe Perches wrote: diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c [] @@ -13,6 +13,8 @@ * Copyright (C) 2004 William Lee Irwin III */ +#define pr_fmt(fmt) ftrace: fmt #define pr_fmt(fmt) KBUILD_MODNAME : fmt Wouldn't a nicer patch be to move this into a header file and then remove all the defines throughout the kernel tree? Maybe it's hard to achieve that. subsystem name is unique with each other, it should be visible in source file, if include into header file, then each .c file might need a own header file for include pr_fmt definition, then that header file cannot be reusable(avoid subsystem name conflicts). Also, what is KBUILD_MODNAME defined as for non-modules? As ftrace is not a module. Yes, that's why I cannot use KBUILD_MODNAME in patch. -- Steve I don't make sure if there have some method or skill to let GCC knows subsystem name automatically, use built-in macro __FILE__? but this need condition of subsystem name is same as file name, not so easily to guarantee that. .jovi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ftrace: using pr_fmt for better printk output
On Tue, Jul 17, 2012 at 1:07 PM, Joe Perches j...@perches.com wrote: On Tue, 2012-07-17 at 00:25 -0400, Steven Rostedt wrote: On Mon, 2012-07-16 at 20:42 -0700, Joe Perches wrote: diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c [] @@ -13,6 +13,8 @@ * Copyright (C) 2004 William Lee Irwin III */ +#define pr_fmt(fmt) ftrace: fmt #define pr_fmt(fmt) KBUILD_MODNAME : fmt Wouldn't a nicer patch be to move this into a header file and then remove all the defines throughout the kernel tree? Maybe. There are modules that use common header files like you suggest. It does mean that header must be the #included before any other #include that might #include linux/kernel.h or printk.h. Right now, if pr_fmt isn't #defined, printk.h has a default definition of: #ifndef pr_fmt #define pr_fmt(fmt) fmt #endif My goal is to change that to: #ifndef pr_fmt #define pr_fmt(fmt) KBUILD_MODNAME : fmt #endif in 3.8 (maybe 3.7) and remove all of these then useless, duplicate #defines shortly afterward. https://lkml.org/lkml/2012/3/27/247 Also, what is KBUILD_MODNAME defined as for non-modules? As ftrace is not a module. It depends on the Makefile. scripts/Makefile.lib:# $(modname_flags) #defines KBUILD_MODNAME as the name of the module it will scripts/Makefile.lib-# end up in (or would, if it gets compiled in) scripts/Makefile.lib-# Note: Files that end up in two or more modules are compiled without the scripts/Makefile.lib:# KBUILD_MODNAME definition. The reason is that any made-up name would scripts/Makefile.lib-# differ in different configs. scripts/Makefile.lib-name-fix = $(subst $(comma),_,$(subst -,_,$1)) scripts/Makefile.lib-basename_flags = -DKBUILD_BASENAME=KBUILD_STR($(call name-fix,$(basetarget))) scripts/Makefile.lib-modname_flags = $(if $(filter 1,$(words $(modname))),\ scripts/Makefile.lib: -DKBUILD_MODNAME=KBUILD_STR($(call name-fix,$(modname Hmm, that would make sense, get subsystem name from Makefile. Joe, there will delete all this pr_fmt definition in .c file in 3.8(or 3.7) as you metioned, so we can ingnore this patch. .Jovi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf: make the breakpoint events sample period default to 1
>From cb06e8f21d3875f4ffef46a7627dca88b2c74836 Mon Sep 17 00:00:00 2001 From: Jovi Zhang Date: Sun, 15 Jul 2012 03:03:10 +0800 Subject: [PATCH] perf: make the breakpoint events sample period default to 1 There have one problem about hw_breakpoint perf event, as watched, the events reported to userspace is not correctly, sometime one trigger bp_event report several events, sometime bp_event cannot go through to user. The root cause is attr->freq is 1 passed to kernel defaultly in bp events, this make kernel calculate event period not as expect, make sample period to 1 will change attr->freq to 0, to fix this problem. This patch is similar with commit f92128 about tracepoint events: perf: Make the trace events sample period default to 1 Signed-off-by: Jovi Zhang --- tools/perf/util/parse-events.c |1 + 1 file changed, 1 insertion(+) diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index 05dbc8b..631aec6 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -592,6 +592,7 @@ int parse_events_add_breakpoint(struct list_head **list, int *idx, attr.bp_len = HW_BREAKPOINT_LEN_4; attr.type = PERF_TYPE_BREAKPOINT; + attr.sample_period = 1; snprintf(name, MAX_NAME_LEN, "mem:%p:%s", ptr, type ? type : "rw"); return add_event(list, idx, , name); -- 1.7.9.7 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf: make the breakpoint events sample period default to 1
From cb06e8f21d3875f4ffef46a7627dca88b2c74836 Mon Sep 17 00:00:00 2001 From: Jovi Zhang bookj...@gmail.com Date: Sun, 15 Jul 2012 03:03:10 +0800 Subject: [PATCH] perf: make the breakpoint events sample period default to 1 There have one problem about hw_breakpoint perf event, as watched, the events reported to userspace is not correctly, sometime one trigger bp_event report several events, sometime bp_event cannot go through to user. The root cause is attr-freq is 1 passed to kernel defaultly in bp events, this make kernel calculate event period not as expect, make sample period to 1 will change attr-freq to 0, to fix this problem. This patch is similar with commit f92128 about tracepoint events: perf: Make the trace events sample period default to 1 Signed-off-by: Jovi Zhang bookj...@gmail.com --- tools/perf/util/parse-events.c |1 + 1 file changed, 1 insertion(+) diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index 05dbc8b..631aec6 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -592,6 +592,7 @@ int parse_events_add_breakpoint(struct list_head **list, int *idx, attr.bp_len = HW_BREAKPOINT_LEN_4; attr.type = PERF_TYPE_BREAKPOINT; + attr.sample_period = 1; snprintf(name, MAX_NAME_LEN, mem:%p:%s, ptr, type ? type : rw); return add_event(list, idx, attr, name); -- 1.7.9.7 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf: fix perf-lock report coredump
On Wed, Jul 11, 2012 at 10:20 AM, David Ahern wrote: > On 7/10/12 7:06 PM, Jovi Zhang wrote: >> >> From 4b363bf16c12b76788fbace1475123b7214ae58d Mon Sep 17 00:00:00 2001 >> From: Jovi Zhang >> Date: Wed, 11 Jul 2012 16:53:57 +0800 >> Subject: [PATCH] perf: fix perf-lock report coredump >> >> Check sample type event raw_data is existed in perf.data firstly, >> then invoke process_raw_event, otherwise it will coredump. > > > Does this fix it for you: > http://lkml.org/lkml/2012/7/6/405 > Yeah, same problem. But the question is if there have some sample event with raw data in perf.data, are we still just exit(1)? or let perf-lock only report those sample events with raw data? > Also does 'perf lock record' work for you? If so can you send me your kernel > config file? Not working. I assume LOCKDEP is not configured in my test machine. > > Thanks, > > David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf: fix perf-lock report coredump
>From 4b363bf16c12b76788fbace1475123b7214ae58d Mon Sep 17 00:00:00 2001 From: Jovi Zhang Date: Wed, 11 Jul 2012 16:53:57 +0800 Subject: [PATCH] perf: fix perf-lock report coredump Check sample type event raw_data is existed in perf.data firstly, then invoke process_raw_event, otherwise it will coredump. Signed-off-by: Jovi Zhang --- tools/perf/builtin-lock.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index fd53319..4b0c230 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -860,7 +860,9 @@ static int process_sample_event(struct perf_tool *tool __used, return -1; } - process_raw_event(sample->raw_data, sample->cpu, sample->time, thread); + if (sample->raw_data) + process_raw_event(sample->raw_data, sample->cpu, sample->time, + thread); return 0; } -- 1.7.9.7 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf: fix perf-report display ip string incorrectly
>From bfc2eb1ac844b6d5f64e36eb01464a1cfba39663 Mon Sep 17 00:00:00 2001
From: Jovi Zhang
Date: Wed, 11 Jul 2012 04:32:15 +0800
Subject: [PATCH] perf: fix perf-report display ip string incorrectly
This patch fix perf ui browser cannot display ip string correctly
when using perf-report.
"char ipstr[BITS_PER_LONG / 4 + 1]" cannot reserve enough space
for ip string, for example string 0x80049ede need 13 bytes,
so change it to "BITS_PER_LONG / 4 + 3".
Signed-off-by: Jovi Zhang
---
tools/perf/ui/browsers/hists.c |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 53f6697..a17c9c7 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -386,6 +386,8 @@ static char *callchain_list__sym_name(struct
callchain_list *cl,
#define LEVEL_OFFSET_STEP 3
+#define IPSTR_LEN (BITS_PER_LONG / 4 + 3)
+
static int hist_browser__show_callchain_node_rb_tree(struct
hist_browser *browser,
struct callchain_node
*chain_node,
u64 total, int level,
@@ -416,7 +418,7 @@ static int
hist_browser__show_callchain_node_rb_tree(struct hist_browser *browse
remaining -= cumul;
list_for_each_entry(chain, >val, list) {
- char ipstr[BITS_PER_LONG / 4 + 1], *alloc_str;
+ char ipstr[IPSTR_LEN], *alloc_str;
const char *str;
int color;
bool was_first = first;
@@ -492,7 +494,7 @@ static int
hist_browser__show_callchain_node(struct hist_browser *browser,
char folded_sign = ' ';
list_for_each_entry(chain, >val, list) {
- char ipstr[BITS_PER_LONG / 4 + 1], *s;
+ char ipstr[IPSTR_LEN], *s;
int color;
folded_sign = callchain_list__folded(chain);
--
1.7.9.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf: fix perf-report display ip string incorrectly
From bfc2eb1ac844b6d5f64e36eb01464a1cfba39663 Mon Sep 17 00:00:00 2001
From: Jovi Zhang bookj...@gmail.com
Date: Wed, 11 Jul 2012 04:32:15 +0800
Subject: [PATCH] perf: fix perf-report display ip string incorrectly
This patch fix perf ui browser cannot display ip string correctly
when using perf-report.
char ipstr[BITS_PER_LONG / 4 + 1] cannot reserve enough space
for ip string, for example string 0x80049ede need 13 bytes,
so change it to BITS_PER_LONG / 4 + 3.
Signed-off-by: Jovi Zhang bookj...@gmail.com
---
tools/perf/ui/browsers/hists.c |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 53f6697..a17c9c7 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -386,6 +386,8 @@ static char *callchain_list__sym_name(struct
callchain_list *cl,
#define LEVEL_OFFSET_STEP 3
+#define IPSTR_LEN (BITS_PER_LONG / 4 + 3)
+
static int hist_browser__show_callchain_node_rb_tree(struct
hist_browser *browser,
struct callchain_node
*chain_node,
u64 total, int level,
@@ -416,7 +418,7 @@ static int
hist_browser__show_callchain_node_rb_tree(struct hist_browser *browse
remaining -= cumul;
list_for_each_entry(chain, child-val, list) {
- char ipstr[BITS_PER_LONG / 4 + 1], *alloc_str;
+ char ipstr[IPSTR_LEN], *alloc_str;
const char *str;
int color;
bool was_first = first;
@@ -492,7 +494,7 @@ static int
hist_browser__show_callchain_node(struct hist_browser *browser,
char folded_sign = ' ';
list_for_each_entry(chain, node-val, list) {
- char ipstr[BITS_PER_LONG / 4 + 1], *s;
+ char ipstr[IPSTR_LEN], *s;
int color;
folded_sign = callchain_list__folded(chain);
--
1.7.9.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf: fix perf-lock report coredump
From 4b363bf16c12b76788fbace1475123b7214ae58d Mon Sep 17 00:00:00 2001 From: Jovi Zhang bookj...@gmail.com Date: Wed, 11 Jul 2012 16:53:57 +0800 Subject: [PATCH] perf: fix perf-lock report coredump Check sample type event raw_data is existed in perf.data firstly, then invoke process_raw_event, otherwise it will coredump. Signed-off-by: Jovi Zhang bookj...@gmail.com --- tools/perf/builtin-lock.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index fd53319..4b0c230 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -860,7 +860,9 @@ static int process_sample_event(struct perf_tool *tool __used, return -1; } - process_raw_event(sample-raw_data, sample-cpu, sample-time, thread); + if (sample-raw_data) + process_raw_event(sample-raw_data, sample-cpu, sample-time, + thread); return 0; } -- 1.7.9.7 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf: fix perf-lock report coredump
On Wed, Jul 11, 2012 at 10:20 AM, David Ahern dsah...@gmail.com wrote: On 7/10/12 7:06 PM, Jovi Zhang wrote: From 4b363bf16c12b76788fbace1475123b7214ae58d Mon Sep 17 00:00:00 2001 From: Jovi Zhang bookj...@gmail.com Date: Wed, 11 Jul 2012 16:53:57 +0800 Subject: [PATCH] perf: fix perf-lock report coredump Check sample type event raw_data is existed in perf.data firstly, then invoke process_raw_event, otherwise it will coredump. Does this fix it for you: http://lkml.org/lkml/2012/7/6/405 Yeah, same problem. But the question is if there have some sample event with raw data in perf.data, are we still just exit(1)? or let perf-lock only report those sample events with raw data? Also does 'perf lock record' work for you? If so can you send me your kernel config file? Not working. I assume LOCKDEP is not configured in my test machine. Thanks, David -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] coredump: fix pipe coredump when core limit is 0
Hi Andrew, Is that attached patch ok to go through your -mm tree? this patch reviewed many months ago, but still not goto mainstream. :) that comments is quite mismatch with the code. Thanks. .jovi 0001-coredump-fix-wrong-comments-on-core-limits-of-pipe-c.patch Description: Binary data
Re: [PATCH] coredump: fix pipe coredump when core limit is 0
Hi Andrew, Is that attached patch ok to go through your -mm tree? this patch reviewed many months ago, but still not goto mainstream. :) that comments is quite mismatch with the code. Thanks. .jovi 0001-coredump-fix-wrong-comments-on-core-limits-of-pipe-c.patch Description: Binary data
