Re: [PATCH v2 03/14] perf: Add persistent event facilities

[Robert Richter <rric@kernel.org>]

On 24.06.13 11:44:58, Peter Zijlstra wrote:
> > +static void del_persistent_event(int cpu, struct perf_event_attr *attr)
> > +{
> > +	struct pers_event_desc *desc, *tmp;
> > +	struct perf_event *event = NULL;
> > +
> > +	list_for_each_entry_safe(desc, tmp, &per_cpu(pers_events, cpu), plist) {
> > +		if (desc->attr->config == attr->config) {
> 
> attr->config might not be enough; you might also want to compare type,
> config1, config2, and possible bp_type as well; although I think we want
> to avoid having hwbp style persistent events, but then what do I know.

[ I would call this what it is: perf_add_persistent_tracepoint(), or
so :-) ]

We chosed a tracepoint-only implementation as a first try and since
this is our use case. The goal is to have support for any event type.

I also was thinking of comparing the whole attr structure. But since
attr are similar (or almost identical) only you can't compare each
member of attr and need exceptions (e.g. cpu number). These exceptions
are ugly and depending on the specific use case.

I rather tend to assign unique identifiers to each persistent event
and then use attr->config to select the event, e.g.:

		if (desc->unique_id == attr->config) ...

Tracepoints have this unique id in it already, thus tp only first.

The change can be made without breaking ABI, since the event setup is
described in sysfs.

-Robert

> 
> > +			event = desc->event;
> > +			break;
> > +		}
> > +	}