Date:	Wed, 18 Mar 2009 01:29:49 -0500
From:	Tom Zanussi <tzanussi@...il.com>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Frédéric Weisbecker <fweisbec@...il.com>
Subject: Re: [RFC][PATCH 0/4] tracing: event filtering

Hi,

On Tue, 2009-03-17 at 09:57 +0100, Ingo Molnar wrote:
> * Tom Zanussi <tzanussi@...il.com> wrote:
> 
> > Hi,
> > 
> > This patchset is a first attempt at adding filtering to the 
> > event-tracing infrastructure.
> 
> Really cool!
> 
> > The filtering itself seems to work ok, as far as I've been 
> > able to test it, but I'm still battling with getting the 
> > ring-buffer to do what I want (discarding events, see patch 2) 
> > so am hoping someone more familiar with the ring buffer can 
> > point me in the right direction before I do any more work on 
> > it.
> 
> Seems to be a weakness in our current event abstraction itself - 
> by the time we get to filtering we already have the record in 
> the ring buffer - and have to work hard to pull it out of there. 
> It would be better to allow tracing filters to operate on a 
> private copy of the data, before it's inserted into the 
> ringbuffer.
> 
> As an intermediate solution (until the rb details get sorted 
> out), i think your hack could be used - it essentially marks the 
> entry as discarded, so that the output stage ignores it, right?
> 

Yeah, that's the idea, which Steve's patch now does correctly.

> If the patch is brought into a more palatable state (no crashes, 
> no C99 comments) i'd argue we apply this almost as-is, so that 
> the filtering details can advance independently of the 
> ring-buffer management details. Steve, do you agree?
> 
> > Another specific thing it would be good to get comments on 
> > would be how to allow the user to unambiguously specify a 
> > field name in a filter when there are duplicate field names 
> > for an event, as mentioned in patch 1.
> 
> A short-term fix would be to name the common fields common_pid 
> or so, to reduce the chance of collision. (and show that in the 
> format output too)
> 
> Plus we should add a debug check as well when an event is 
> registered: all fields in a format should be uniquely 
> accessible.
> 
> > Of course, any comments about the rest of the interface and 
> > code are also welcome...
> 
> You wanted to keep the filter expression parser simple, and i 
> agree with that in general.
> 
> I'd expect the filter to be popular with kernel developers who 
> do ad-hoc tracing - so making it as compatible with typical 
> syntax variations as possible would still be nice. The parser 
> will be larger but that's OK.
> 
>  - it would be nice to extend the range of operators to all the
>    typical C syntax comparison expressions: <= < >= > != ==. Some 
>    of these are supported but not all.
> 
>  - there should be '||' and '&&' aliases for the 'or' / 'and' 
>    tokens.

I was trying to avoid using shell meta-characters to avoid the need for
any escaping, thus the 'and' and 'or', but can easily change it to use
this syntax instead if it's more intuitive.

> 
>  - parantheses could be supported too perhaps instead of the 
>    current 'echo separately to build up complex expressions', up
>    to the expression-length limit.
> 
>  - bitwise operators might be useful too: 'mask & 0xff'.
> 
> We really want this to be a popular built-in facility that can 
> be used intuitively by anyone who knows C expressions, and 
> limitations in the expression parser are counter-productive to 
> that aim.

I agree - the current parser is pretty silly anyway, so replacing it
with a more capable parser makes sense.  I'll do that in the next
iteration...

Tom

> 
> 	Ingo

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

Powered by blists - more mailing lists