* Re: [RFC][PATCH 0/4] tracing: event filtering
2009-03-17 8:57 ` Ingo Molnar
@ 2009-03-18 1:18 ` Frederic Weisbecker
2009-03-18 6:29 ` Tom Zanussi
1 sibling, 0 replies; 4+ messages in thread
From: Frederic Weisbecker @ 2009-03-18 1:18 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Tom Zanussi, linux-kernel, Steven Rostedt
On Tue, Mar 17, 2009 at 09:57:13AM +0100, Ingo Molnar wrote:
>
> * Tom Zanussi <tzanussi@gmail.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?
>
> 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.
>
> - parantheses could be supported too perhaps instead of the
> current 'echo separately to build up complex expressions', up
> to the expression-length limit.
Indeed, it would be much more intuitive.
Only one level of parenthesis with two operands and one operator inside
each groups. So that the expression parsing can stay about simple and
anyway people will not need more.
Frederic.
> - 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.
>
> Ingo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][PATCH 0/4] tracing: event filtering
2009-03-17 8:57 ` Ingo Molnar
2009-03-18 1:18 ` Frederic Weisbecker
@ 2009-03-18 6:29 ` Tom Zanussi
1 sibling, 0 replies; 4+ messages in thread
From: Tom Zanussi @ 2009-03-18 6:29 UTC (permalink / raw)
To: Ingo Molnar; +Cc: linux-kernel, Steven Rostedt, Frédéric Weisbecker
Hi,
On Tue, 2009-03-17 at 09:57 +0100, Ingo Molnar wrote:
> * Tom Zanussi <tzanussi@gmail.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
^ permalink raw reply [flat|nested] 4+ messages in thread