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

Li Zefan
2009-06-19T05:00:32Z

Tom Zanussi wrote:
> 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@gmail.com>
>>>> | 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.
>

Yeah, being able to do this should be very useful:

# echo 'irq == 1' > irq/filter
# echo 'vec == 5' > irq/filter

Otherwise I have to set each filter one by one.

After setting subsystem/filter, one can change a single event's filter,
and then the subsys/filter is not consistent with it's members' filter.

So subsystem/filter is much more useful in writing but not reading.

> 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

This should be rare. In a subsys, if 2 events have a field with the
same name, they normally means the same thing.

And even in this case, it's not that dangerous I think. ;)

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