Re: [PATCH 1/2] tracing/filters: restore orignal filter when newfilter isn't applicable

[Tom Zanussi]

On Thu, 2009-06-18 at 19:18 -0400, Steven Rostedt wrote:
> 
> 
> On Thu, 18 Jun 2009, Tom Zanussi wrote:
> 
> > Hi,
> > 
> > On Wed, 2009-06-17 at 16:46 +0800, Li Zefan wrote:
> > > | commit 7ce7e4249921d5073e764f7ff7ad83cfa9894bd7
> > > | Author: Tom Zanussi <tzanussi@xxxxxxxxx>
> > > | Date:   Sun Mar 22 03:31:04 2009 -0500
> > > |
> > > |     tracing: add per-event filtering
> > > |     ...
> > > |
> > > |     Filters can also be set or cleared for a complete subsystem by writing
> > > |     the same filter as would be written to an individual event to the
> > > |     filter file at the root of the subsytem.  Note however, that if any
> > > |     event in the subsystem lacks a field specified in the filter being
> > > |     set, the set will fail and all filters in the subsytem are
> > > |     automatically cleared.  This change from the previous version was made
> > > |     because using only the fields that happen to exist for a given event
> > > |     would most likely result in a meaningless filter.
> > > 
> > > I really don't like this change. It is inconvenient, and makes subsystem
> > > filter much less useful:
> > > 
> > >   # echo 'vec == 1' > irq/softirq_entry/filter
> > >   # echo 'irq == 5' > irq/filter
> > >   bash: echo: write error: Invalid argument
> > >   # cat irq/softirq_entry/filter
> > >   none
> > > 
> > > I'd expect this will set the filter for irq_handler_entry and
> > > irq_handler_exit, and won't touch softirq_entry and softirq_exit.
> > > 
> > > But it just failed, and what's worse, each event's filter was
> > > cleared.
> > > 
> > 
> > The idea behind the change was that after setting a subsystem filter,
> > you'd be guaranteed that all or none of the events in the subsystem
> > would have the same filter at that point, and not some mix of different
> > filters depending on which ones failed or not, which to me seemed
> > nonintuitive.
> > 
> > If I set a filter like "vec == 1 && irq == 5", which really has no
> > overall meaning, I wouldn't expect softirq_entry to get "vec == 1" and
> > irq_handler_entry to get "irq == 5" - I'd rather get an error, but
> > that's just me.
> > 
> > So if it makes more sense to users to have subsystem filters propagate
> > to whichever events will take them, this patch would be fine with me.
> > 
> 
> I disagree. If a subsystem filter is set, it should be valid for all 
> filters underneath, if it is not, it should error.
> 
> But Li has a point, if you get an error, it should not reset all filters 
> underneath. That is, if the irq/filter setting took an error, then 
> irq/softirq_entry/filter should still stay the same.
> 
> Perhaps you need to run through it twice. See if the setting of a filter 
> is valid for all filters underneath, if it is not, then fail. If it is, 
> then reset all of them, and assign the filter.
> 

Yeah, I agree that this is better than just clearing them all on an
error, but it still means that a subsystem filter will succeed only when
it names common_ (or commonly named) fields.

I think what Li is saying is that that restriction makes the subsystem
filters less useful, and you should for convenience' sake be allowed to
propagate a filter to a subset and ignore the ones that don't make
sense.

Note that there's a danger in this case that a filter might be applied
but not really make sense e.g. two events might have a 'vec' field that
mean completely different things but the filter would be applied to both
just because they have the same name.  The only way to ensure that they
would always make sense would be to restrict the subsystem filter to
just the common_ fields.

But that's less useful, and maybe it would be better to leave the choice
up the user...

Tom

> -- 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/