From: Tom Zanussi <tom.zanussi@linux.intel.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: masami.hiramatsu.pt@hitachi.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/2] tracing/triggers: A couple minor variable name changes
Date: Mon, 06 Jan 2014 18:47:53 -0600 [thread overview]
Message-ID: <1389055673.3040.83.camel@empanada> (raw)
In-Reply-To: <20140106161347.4cc82a4b@gandalf.local.home>
Hi Steve,
On Mon, 2014-01-06 at 16:13 -0500, Steven Rostedt wrote:
> I applied these locally, but I'm also going to apply this patch to get
> rid of all that duplicate code. Any objections? (or do you see any
> mistakes? I only compiled and boot tested it, nothing more strenuous) I
> will do that before pushing it though.
>
It's a nice cleanup, and from my reading of it, the logic is all
correct, but there are a couple things that I think are a bit misleading
in the naming, see below..
> -- Steve
>
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 03d2db2..ccda66f 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -370,6 +370,75 @@ extern enum event_trigger_type event_triggers_call(struct ftrace_event_file *fil
> extern void event_triggers_post_call(struct ftrace_event_file *file,
> enum event_trigger_type tt);
>
> +static inline int
> +ftrace_trigger_call_disabled(struct ftrace_event_file *file)
> +{
> + unsigned long eflags = file->flags;
> +
> + if (!(eflags & FTRACE_EVENT_FL_TRIGGER_COND)) {
> + if (eflags & FTRACE_EVENT_FL_TRIGGER_MODE)
> + event_triggers_call(file, NULL);
> + if (eflags & FTRACE_EVENT_FL_SOFT_DISABLED)
> + return 1;
> + }
> + return 0;
> +}
> +
ftrace_trigger_call_disabled() as a name is OK, but it makes it sound
like the trigger calls are disabled if it returns 1, when it actually
can invoke the triggers when it returns 1, in cases where there's no
reason to delay them. ftrace_call_triggers_check_soft_disabled() is
more accurate but too unwieldy, so I'm not coming up with anything
better. Maybe ftrace_trigger_soft_disabled() reads better, or not.
Punting...
> +static inline int
> +__event_trigger_test_discard(struct ftrace_event_file *file,
> + struct ring_buffer *buffer,
> + struct ring_buffer_event *event,
> + void *entry,
> + enum event_trigger_type *tt)
> +{
> + unsigned long eflags = file->flags;
> +
> + if (eflags & FTRACE_EVENT_FL_TRIGGER_COND)
> + *tt = event_triggers_call(file, entry);
> +
> + if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &file->flags))
> + ring_buffer_discard_commit(buffer, event);
> + else if (!filter_check_discard(file, entry, buffer, event))
> + return 1;
If the function is called ..._discard() then shouldn't it return 0 if it
didn't discard?
> +
> + return 0;
> +}
> +
> +static inline void
> +event_trigger_unlock_commit(struct ftrace_event_file *file,
> + struct ring_buffer *buffer,
> + struct ring_buffer_event *event,
> + void *entry, unsigned long irq_flags, int pc)
> +{
> + unsigned long eflags = file->flags;
eflags isn't used in this function...
> + enum event_trigger_type tt = ETT_NONE;
> +
> + if (__event_trigger_test_discard(file, buffer, event, entry, &tt))
> + trace_buffer_unlock_commit(buffer, event, irq_flags, pc);
The logic is correct overall, but the way it reads is the opposite of
what it used to i.e. it should read 'if you don't discard the event,
then do the trace_buffer_unlock_commit' - it works as written because it
returns 1 if it didn't discard, which is confusing.
> +
> + if (tt)
> + event_triggers_post_call(file, tt);
> +}
> +
> +
> +static inline void
> +event_trigger_unlock_commit_regs(struct ftrace_event_file *file,
> + struct ring_buffer *buffer,
> + struct ring_buffer_event *event,
> + void *entry, unsigned long irq_flags, int pc,
> + struct pt_regs *regs)
> +{
> + unsigned long eflags = file->flags;
Ditto here on the unused eflags.
Tom
> + enum event_trigger_type tt = ETT_NONE;
> +
> + if (__event_trigger_test_discard(file, buffer, event, entry, &tt))
> + trace_buffer_unlock_commit_regs(buffer, event,
> + irq_flags, pc, regs);
> +
> + if (tt)
> + event_triggers_post_call(file, tt);
> +}
> +
> enum {
> FILTER_OTHER = 0,
> FILTER_STATIC_STRING,
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index 0962968..7a43935 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -546,8 +546,6 @@ ftrace_raw_event_##call(void *__data, proto) \
> struct ftrace_event_file *ftrace_file = __data; \
> struct ftrace_event_call *event_call = ftrace_file->event_call; \
> struct ftrace_data_offsets_##call __maybe_unused __data_offsets;\
> - unsigned long eflags = ftrace_file->flags; \
> - enum event_trigger_type __tt = ETT_NONE; \
> struct ring_buffer_event *event; \
> struct ftrace_raw_##call *entry; \
> struct ring_buffer *buffer; \
> @@ -555,12 +553,8 @@ ftrace_raw_event_##call(void *__data, proto) \
> int __data_size; \
> int pc; \
> \
> - if (!(eflags & FTRACE_EVENT_FL_TRIGGER_COND)) { \
> - if (eflags & FTRACE_EVENT_FL_TRIGGER_MODE) \
> - event_triggers_call(ftrace_file, NULL); \
> - if (eflags & FTRACE_EVENT_FL_SOFT_DISABLED) \
> - return; \
> - } \
> + if (ftrace_trigger_call_disabled(ftrace_file)) \
> + return; \
> \
> local_save_flags(irq_flags); \
> pc = preempt_count(); \
> @@ -579,17 +573,8 @@ ftrace_raw_event_##call(void *__data, proto) \
> \
> { assign; } \
> \
> - if (eflags & FTRACE_EVENT_FL_TRIGGER_COND) \
> - __tt = event_triggers_call(ftrace_file, entry); \
> - \
> - if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, \
> - &ftrace_file->flags)) \
> - ring_buffer_discard_commit(buffer, event); \
> - else if (!filter_check_discard(ftrace_file, entry, buffer, event)) \
> - trace_buffer_unlock_commit(buffer, event, irq_flags, pc); \
> - \
> - if (__tt) \
> - event_triggers_post_call(ftrace_file, __tt); \
> + event_trigger_unlock_commit(ftrace_file, buffer, event, entry, \
> + irq_flags, pc); \
> }
> /*
> * The ftrace_test_probe is compiled out, it is only here as a build time check
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 3afa716..196b427 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -929,20 +929,12 @@ __kprobe_trace_func(struct trace_kprobe *tk, struct pt_regs *regs,
> struct ring_buffer *buffer;
> int size, dsize, pc;
> unsigned long irq_flags;
> - unsigned long eflags;
> - enum event_trigger_type tt = ETT_NONE;
> struct ftrace_event_call *call = &tk->tp.call;
>
> WARN_ON(call != ftrace_file->event_call);
>
> - eflags = ftrace_file->flags;
> -
> - if (!(eflags & FTRACE_EVENT_FL_TRIGGER_COND)) {
> - if (eflags & FTRACE_EVENT_FL_TRIGGER_MODE)
> - event_triggers_call(ftrace_file, NULL);
> - if (eflags & FTRACE_EVENT_FL_SOFT_DISABLED)
> - return;
> - }
> + if (ftrace_trigger_call_disabled(ftrace_file))
> + return;
>
> local_save_flags(irq_flags);
> pc = preempt_count();
> @@ -960,16 +952,8 @@ __kprobe_trace_func(struct trace_kprobe *tk, struct pt_regs *regs,
> entry->ip = (unsigned long)tk->rp.kp.addr;
> store_trace_args(sizeof(*entry), &tk->tp, regs, (u8 *)&entry[1], dsize);
>
> - if (eflags & FTRACE_EVENT_FL_TRIGGER_COND)
> - tt = event_triggers_call(ftrace_file, entry);
> -
> - if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags))
> - ring_buffer_discard_commit(buffer, event);
> - else if (!filter_check_discard(ftrace_file, entry, buffer, event))
> - trace_buffer_unlock_commit_regs(buffer, event,
> - irq_flags, pc, regs);
> - if (tt)
> - event_triggers_post_call(ftrace_file, tt);
> + event_trigger_unlock_commit_regs(ftrace_file, buffer, event,
> + entry, irq_flags, pc, regs);
> }
>
> static __kprobes void
> @@ -992,20 +976,12 @@ __kretprobe_trace_func(struct trace_kprobe *tk, struct kretprobe_instance *ri,
> struct ring_buffer *buffer;
> int size, pc, dsize;
> unsigned long irq_flags;
> - unsigned long eflags;
> - enum event_trigger_type tt = ETT_NONE;
> struct ftrace_event_call *call = &tk->tp.call;
>
> WARN_ON(call != ftrace_file->event_call);
>
> - eflags = ftrace_file->flags;
> -
> - if (!(eflags & FTRACE_EVENT_FL_TRIGGER_COND)) {
> - if (eflags & FTRACE_EVENT_FL_TRIGGER_MODE)
> - event_triggers_call(ftrace_file, NULL);
> - if (eflags & FTRACE_EVENT_FL_SOFT_DISABLED)
> - return;
> - }
> + if (ftrace_trigger_call_disabled(ftrace_file))
> + return;
>
> local_save_flags(irq_flags);
> pc = preempt_count();
> @@ -1024,16 +1000,8 @@ __kretprobe_trace_func(struct trace_kprobe *tk, struct kretprobe_instance *ri,
> entry->ret_ip = (unsigned long)ri->ret_addr;
> store_trace_args(sizeof(*entry), &tk->tp, regs, (u8 *)&entry[1], dsize);
>
> - if (eflags & FTRACE_EVENT_FL_TRIGGER_COND)
> - tt = event_triggers_call(ftrace_file, entry);
> -
> - if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags))
> - ring_buffer_discard_commit(buffer, event);
> - else if (!filter_check_discard(ftrace_file, entry, buffer, event))
> - trace_buffer_unlock_commit_regs(buffer, event,
> - irq_flags, pc, regs);
> - if (tt)
> - event_triggers_post_call(ftrace_file, tt);
> + event_trigger_unlock_commit_regs(ftrace_file, buffer, event,
> + entry, irq_flags, pc, regs);
> }
>
> static __kprobes void
> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index a4acf9b..6d407eb 100644
> --- a/kernel/trace/trace_syscalls.c
> +++ b/kernel/trace/trace_syscalls.c
> @@ -306,10 +306,8 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id)
> struct syscall_trace_enter *entry;
> struct syscall_metadata *sys_data;
> struct ring_buffer_event *event;
> - enum event_trigger_type tt = ETT_NONE;
> struct ring_buffer *buffer;
> unsigned long irq_flags;
> - unsigned long eflags;
> int pc;
> int syscall_nr;
> int size;
> @@ -323,14 +321,8 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id)
> if (!ftrace_file)
> return;
>
> - eflags = ftrace_file->flags;
> -
> - if (!(eflags & FTRACE_EVENT_FL_TRIGGER_COND)) {
> - if (eflags & FTRACE_EVENT_FL_TRIGGER_MODE)
> - event_triggers_call(ftrace_file, NULL);
> - if (eflags & FTRACE_EVENT_FL_SOFT_DISABLED)
> - return;
> - }
> + if (ftrace_trigger_call_disabled(ftrace_file))
> + return;
>
> sys_data = syscall_nr_to_meta(syscall_nr);
> if (!sys_data)
> @@ -351,16 +343,8 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id)
> entry->nr = syscall_nr;
> syscall_get_arguments(current, regs, 0, sys_data->nb_args, entry->args);
>
> - if (eflags & FTRACE_EVENT_FL_TRIGGER_COND)
> - tt = event_triggers_call(ftrace_file, entry);
> -
> - if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags))
> - ring_buffer_discard_commit(buffer, event);
> - else if (!filter_check_discard(ftrace_file, entry, buffer, event))
> - trace_current_buffer_unlock_commit(buffer, event,
> - irq_flags, pc);
> - if (tt)
> - event_triggers_post_call(ftrace_file, tt);
> + event_trigger_unlock_commit(ftrace_file, buffer, event, entry,
> + irq_flags, pc);
> }
>
> static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret)
> @@ -370,10 +354,8 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret)
> struct syscall_trace_exit *entry;
> struct syscall_metadata *sys_data;
> struct ring_buffer_event *event;
> - enum event_trigger_type tt = ETT_NONE;
> struct ring_buffer *buffer;
> unsigned long irq_flags;
> - unsigned long eflags;
> int pc;
> int syscall_nr;
>
> @@ -386,14 +368,8 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret)
> if (!ftrace_file)
> return;
>
> - eflags = ftrace_file->flags;
> -
> - if (!(eflags & FTRACE_EVENT_FL_TRIGGER_COND)) {
> - if (eflags & FTRACE_EVENT_FL_TRIGGER_MODE)
> - event_triggers_call(ftrace_file, NULL);
> - if (eflags & FTRACE_EVENT_FL_SOFT_DISABLED)
> - return;
> - }
> + if (ftrace_trigger_call_disabled(ftrace_file))
> + return;
>
> sys_data = syscall_nr_to_meta(syscall_nr);
> if (!sys_data)
> @@ -413,16 +389,8 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret)
> entry->nr = syscall_nr;
> entry->ret = syscall_get_return_value(current, regs);
>
> - if (eflags & FTRACE_EVENT_FL_TRIGGER_COND)
> - tt = event_triggers_call(ftrace_file, entry);
> -
> - if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags))
> - ring_buffer_discard_commit(buffer, event);
> - else if (!filter_check_discard(ftrace_file, entry, buffer, event))
> - trace_current_buffer_unlock_commit(buffer, event,
> - irq_flags, pc);
> - if (tt)
> - event_triggers_post_call(ftrace_file, tt);
> + event_trigger_unlock_commit(ftrace_file, buffer, event, entry,
> + irq_flags, pc);
> }
>
> static int reg_event_syscall_enter(struct ftrace_event_file *file,
next prev parent reply other threads:[~2014-01-07 0:47 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-06 19:44 [PATCH 0/2] tracing/triggers: A couple minor variable name changes Tom Zanussi
2014-01-06 19:44 ` [PATCH 1/2 v2] tracing/kprobes: Add trace event trigger invocations Tom Zanussi
2014-01-06 21:56 ` Masami Hiramatsu
2014-01-06 19:44 ` [PATCH 2/2] tracing: Remove double-underscore naming in syscall " Tom Zanussi
2014-01-06 21:13 ` [PATCH 0/2] tracing/triggers: A couple minor variable name changes Steven Rostedt
2014-01-07 0:47 ` Tom Zanussi [this message]
2014-01-07 1:50 ` Steven Rostedt
2014-01-07 2:51 ` Tom Zanussi
2014-01-07 3:27 ` Steven Rostedt
2014-01-07 17:29 ` Tom Zanussi
2014-01-07 17:56 ` Steven Rostedt
2014-01-07 3:31 ` Steven Rostedt
2014-01-07 17:30 ` Tom Zanussi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1389055673.3040.83.camel@empanada \
--to=tom.zanussi@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=masami.hiramatsu.pt@hitachi.com \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).