Re: [PATCH 10/11] tracing: add and use generic set_trigger_filter()implementation

[Steven Rostedt]

On Fri, 2013-06-21 at 12:59 -0500, Tom Zanussi wrote:
> Hi Masami,
> 
> On Fri, 2013-06-21 at 13:18 +0900, Masami Hiramatsu wrote:
> > (2013/06/21 3:31), Tom Zanussi wrote:
> > > diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> > > index 88ac7da..7c5627f 100644
> > > --- a/include/trace/ftrace.h
> > > +++ b/include/trace/ftrace.h
> > > @@ -522,14 +522,6 @@ ftrace_raw_event_##call(void *__data, proto)				\
> > >  	int __data_size;						\
> > >  	int pc;								\
> > >  									\
> > > -	if (test_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT,			\
> > > -		     &ftrace_file->flags))				\
> > > -		event_triggers_call(ftrace_file);			\
> > > -									\
> > > -	if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT,			\
> > > -		     &ftrace_file->flags))				\
> > > -		return;							\

I'd like to still exit early on soft disable if needed. Perhaps have a:

	if (ftrace_file->flags & (FTRACE_EVENT_FL_TRIGGER_MODE_BIT | \
	     FTRACE_EVENT_FL_SOFT_DISABLED_BIT) == \
	     FTRACE_EVENT_FL_SOFT_DISABLED_BIT) \
		return; \

> > > -									\
> > >  	local_save_flags(irq_flags);					\
> > >  	pc = preempt_count();						\
> > >  									\
> > > @@ -547,8 +539,22 @@ ftrace_raw_event_##call(void *__data, proto)				\
> > >  									\
> > >  	{ assign; }							\
> > >  									\
> > > +	if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT,			\
> > > +		     &ftrace_file->flags)) {				\
> > > +		ring_buffer_discard_commit(buffer, event);		\
> > > +									\
> > > +		if (test_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT,		\
> > > +			     &ftrace_file->flags))			\
> > > +			event_triggers_call(ftrace_file, entry);	\
> > > +		return;							\
> > > +	}								\
> > > +									\
> > >  	if (!filter_current_check_discard(buffer, event_call, entry, event)) \
> > >  		trace_buffer_unlock_commit(buffer, event, irq_flags, pc); \
> > > +									\
> > > +	if (test_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT,			\
> > > +		     &ftrace_file->flags))				\
> > > +		event_triggers_call(ftrace_file, entry);		\
> > 
> > Actually, since "entry" is a part of "event" which may be already discarded,
> > I think we should not access it here. It may not cause real problem because
> > even if it is discarded, that does NOT mean it is freed. However, it depends
> > on the ring-buffer implementation.
> > 
> > I recommend you to call event triggers before commit the event. It will also
> > make the code simpler :)
> >
> 
> That's what I originally did, and is what I was referring to when I
> mentioned the bit of 'trickiness' here. ;-)
> 
> The problem is that the trace_recursive_lock() check in
> ring_buffer_lock_reserve() prevents a trigger that itself logs to the
> ring buffer from reserving a slot in the buffer, since it's being done
> from the same context as the triggering event.  For example, the
> stacktrace trigger, which calls trace_dump_stack().

Ah, yeah that would happen.

> 
> So the code is a bit more convoluted than I'd like because of that -
> since we can't really invoke the triggers before the current event is
> committed, it waits until the current event is either discarded (because
> we're soft disabled) or committed (logging a normally-enabled event).
> 
> But you do point out a real problem with this - it means having to look
> at a discarded event in the soft-disabled case, and if for example some
> interrupt actually reserves and logs an event between the time we do the
> discard and invoke the filter, we could end up with a false filtering
> outcome.  I'm not sure how to prevent that though at this point - will
> have think about it some more...
> 

Perhaps add a trigger mode here:

enum trigger_mode {
	TM_NONE,
	TM_STACK,
};


Then do:

	trigger_mode = event_triggers_call(ftrace_file, entry);

	(finish event)


	if (unlikely(trigger_mode))
		event_triggers_post_call(ftrace_file, trigger_mode);

Something like that.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/