Messages in this thread | | | Date | Fri, 21 Jun 2013 21:12:50 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH 05/11] tracing: add basic event trigger framework |
| |
(2013/06/21 3:31), Tom Zanussi wrote: > The event trigger functionality is built on top of SOFT_DISABLE > functionality. It adds a TRIGGER_MODE bit to the ftrace_event_file > flags which is checked when any trace event fires. Triggers set for a > particular event need to be checked regardless of whether that event > is actually enabled or not - getting an event to fire even if it's not > enabled is essentially what's already implemented by SOFT_DISABLE > mode, so trigger mode directly reuses that. It essentially inherits > the soft disable logic in __ftrace_event_enable_disable() while adding > a bit of logic and trigger reference counting via tm_ref on top of > that. Because the enable_disable code needs to now be invoked from > outside trace_events.c, a wrapper is also added for those usages.
Agreed, but I think the implementation looks not enough. You implemented it directly in __ftrace_event_enable_disable(), but I think it should be wrapped with other function, something like ftrace_event_trigger_enable_disable(), and don't touch __ftrce_event_enable_disable().
[..] > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > index f9738dc..1fc1602 100644 > --- a/kernel/trace/trace_events.c > +++ b/kernel/trace/trace_events.c > @@ -242,7 +242,8 @@ void trace_event_enable_cmd_record(bool enable) > } > > static int __ftrace_event_enable_disable(struct ftrace_event_file *file, > - int enable, int soft_disable) > + int enable, int soft_disable, > + int trigger_enable)
Here, you added trigger_enable, but the code implies this flag must be set exclusively with soft_disable.
> { > struct ftrace_event_call *call = file->event_call; > int ret = 0; > @@ -263,7 +264,13 @@ static int __ftrace_event_enable_disable(struct ftrace_event_file *file, > * we do nothing. Do not disable the tracepoint, otherwise > * "soft enable"s (clearing the SOFT_DISABLED bit) wont work. > */ > - if (soft_disable) { > + if (trigger_enable) { > + if (atomic_dec_return(&file->tm_ref) > 0) > + break; > + clear_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT, &file->flags); > + ret = __ftrace_event_enable_disable(file, enable, 1, 0); > + break;
Why break here? If all triggers are gone, and no other user is on this event, this event should be disabled hardly.
> + } else if (soft_disable) { > if (atomic_dec_return(&file->sm_ref) > 0) > break; > disable = file->flags & FTRACE_EVENT_FL_SOFT_DISABLED; > @@ -279,7 +286,7 @@ static int __ftrace_event_enable_disable(struct ftrace_event_file *file, > } > call->class->reg(call, TRACE_REG_UNREGISTER, file); > } > - /* If in SOFT_MODE, just set the SOFT_DISABLE_BIT */ > + /* If in SOFT_MODE, just set the SOFT_DISABLE_BIT, else clear it */
This part should be merged with previous patch (which I doubted...).
> if (file->flags & FTRACE_EVENT_FL_SOFT_MODE) > set_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &file->flags); > else > @@ -294,7 +301,13 @@ static int __ftrace_event_enable_disable(struct ftrace_event_file *file, > * set SOFT_DISABLED before enabling the event tracepoint, so > * it still seems to be disabled. > */ > - if (!soft_disable) > + if (trigger_enable) { > + if (atomic_inc_return(&file->tm_ref) > 1) > + break; > + set_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT, &file->flags); > + ret = __ftrace_event_enable_disable(file, enable, 1, 0);
Hmm, if you do this, you can simply do as below :)
int ftrace_event_trigger_enable_disable(file, enable) { if (enable) { atomic-count-up-and-return-if-its-not-first-one set_bit(TRIGGER_MODE) __ftrace_event_enable_disable(file, 1, 1); } else { atomic-count-down-and-return-if-its-not-last-one clear_bit(TRIGGER_MODE) __ftrace_event_enable_disable(file, 0, 1); } }
[OffTopic] BTW, I think current the 3rd argument name "soft_disable" is a bit confused, it actually means "control event in soft-mode". Even if the event is enabled, __ftrace_event_enable_disable(file, 0, 1) doesn't disable it, but just removes soft-mode bit.
Thank you,
-- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com
| |