Re: [PATCH v0 3/5] perf: Introduce instruction trace filtering

[Alexander Shishkin <alexander.shishkin@linux.intel.com>]

Peter Zijlstra <peterz@infradead.org> writes:

> On Fri, Dec 11, 2015 at 02:23:36PM +0000, Mark Rutland wrote:
>> On Fri, Dec 11, 2015 at 03:02:01PM +0100, Peter Zijlstra wrote:
>> > On Fri, Dec 11, 2015 at 03:36:36PM +0200, Alexander Shishkin wrote:
>> > 
>> > > The pmu driver interface basically adds an extra callback to the
>> > > pmu driver structure, which validates the filter configuration proposed
>> > > by the user against what the hardware is actually capable of doing
>> > > and translates it into something that pmu::start can program into
>> > > hardware.
>> > 
>> > > @@ -388,12 +393,38 @@ struct pmu {
>> > >  	void (*free_aux)		(void *aux); /* optional */
>> > >  
>> > >  	/*
>> > > +	 * Validate instruction tracing filters: make sure hw supports the
>> > > +	 * requested configuration and number of filters.
>> > > +	 *
>> > > +	 * Configure instruction tracing filters: translate hw-agnostic filter
>> > > +	 * into hardware configuration in event::hw::itrace_filters
>> > > +	 */
>> > > +	int (*itrace_filter_setup)	(struct perf_event *event); /* optional */
>> > > +
>> > > +	/*
>> > >  	 * Filter events for PMU-specific reasons.
>> > >  	 */
>> > >  	int (*filter_match)		(struct perf_event *event); /* optional */
>> > >  };
>> > 
>> > Any reason you cannot use pmu::filter_match ?
>> 
>> Maybe I've misunderstood your point, but the two seem quite different.
>> 
>> We introduced pmu::filter_match to apply a SW filter each time we
>> installed events from a context. We use that on ARM to avoid programming
>> big events into little cores and vice-versa.
>> 
>> As far as I can see, itrace_filter_setup is closer in operation to
>> event_init. It can fail at configuration time (long before scheduling
>> events to cores), and leaves the actual filtering to the HW.
>
> Ah indeed. I got confused by the similarities in the signature and both
> having filter in the name.
>
> Alexander, maybe then add a filter parameter to validate to the
> signature, otherwise you have to change the event state in order to
> validate it which leaves you with an invalid configured event in case it
> doesn't validate.

The validation part also checks that we have not more filters than
hardware supports, so this callback walks through the list and counts
them up, iow, it looks at all the filters in one go.

In case validation fails (and it can only fail in the ioctl() path), the
event itself is still in a valid state though, we just decline the
filters supplied by the user.

I could split this callback in two: one for validation and one for the
stop/update_filters/start cadence, but I don't see if that helps much
either.

Thanks,
--
Alex