Re: [PATCH] tracing/filters: allow on-the-fly filter switching

[Ingo Molnar <mingo@elte.hu>]

* Tom Zanussi <tzanussi@gmail.com> wrote:

> This patch allows event filters to be safely removed or switched 
> on-the-fly while avoiding the use of rcu or the suspension of 
> tracing of previous versions.
> 
> It does it by adding a new filter_pred_none() predicate function 
> which does nothing and by never deallocating either the predicates 
> or any of the filter_pred members used in matching; the predicate 
> lists are allocated and initialized during ftrace_event_calls 
> initialization.
> 
> Whenever a filter is removed or replaced, the filter_pred_* 
> functions currently in use by the affected ftrace_event_call are 
> immediately switched over to to the filter_pred_none() function, 
> while the rest of the filter_pred members are left intact, 
> allowing any currently executing filter_pred_* functions to finish 
> up, using the values they're currently using.
> 
> In the case of filter replacement, the new predicate values are 
> copied into the old predicates after the above step, and the 
> filter_pred_none() functions are replaced by the filter_pred_* 
> functions for the new filter.  In this case, it is possible though 
> very unlikely that a previous filter_pred_* is still running even 
> after the filter_pred_none() switch and the switch to the new 
> filter_pred_*.  In that case, however, because nothing has been 
> deallocated in the filter_pred, the worst that can happen is that 
> the old filter_pred_* function sees the new values and as a result 
> produces either a false positive or a false negative, depending on 
> the values it finds.
> 
> So one downside to this method is that rarely, it can produce a 
> bad match during the filter switch, but it should be possible to 
> live with that, IMHO.

Yeah.

It is really a strong thing to avoid RCU here. Instrumentation 
should be self-sufficient to a large degree, and it does not get any 
more lowlevel than filter expression evaluation engine. Forcing the 
use of rcu_read_lock() there would limit its utility.

> The other downside is that at least in this patch the predicate 
> lists are always pre-allocated, taking up memory from the start.  
> They could probably be allocated on first-use, and de-allocated 
> when tracing is completely stopped - if this patch makes sense, I 
> could create another one to do that later on.

That's not a big issue IMO.

> Oh, and it also places a restriction on the size of __arrays in 
> events, currently set to 128, since they can't be larger than the 
> now embedded str_val arrays in the filter_pred struct.

that's OK too - we really want pre-calculated filter expressions and 
as atomic evaluations as possible. So having the maximum width 
specified is no big deal.

The only exception would be if we ever do PATH_MAX type of field 
value comparisons - and i dont see any reason why not, once tracing 
is extended to the VFS or once the syscall tracer . That would 
increase it to 4096 bytes, making the max kzalloc larger than page 
size - still not outrageous so not a big problem. Just lets keep it 
in mind that 128 is a bit on the low side.

also:

> +     if (!val_str || !strlen(val_str)
> +         || strlen(val_str) >= MAX_FILTER_STR_VAL) {
>               pred->field_name = NULL;
>               return -EINVAL;
>	}

it might be quite cryptic to the user why a complex expression was 
not installed. I think a single-line KERN_INFO syslog entry would be 
most helpful.

	Ingo