Hi,
This patchset is a first attempt at adding filtering to the
event-tracing infrastructure.
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.
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.
Of course, any comments about the rest of the interface and code are
also welcome...
Thanks,
Tom
* Tom Zanussi <[email protected]> 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.
- 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
On Tue, Mar 17, 2009 at 09:57:13AM +0100, Ingo Molnar wrote:
>
> * Tom Zanussi <[email protected]> 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
Hi,
On Tue, 2009-03-17 at 09:57 +0100, Ingo Molnar wrote:
> * Tom Zanussi <[email protected]> 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