Re: [PATCH v2 net-next 1/3] perf, bpf: Add BPF support to all perf_event types

[Peter Zijlstra]

On Mon, May 29, 2017 at 11:12:53AM +0200, Peter Zijlstra wrote:
> On Thu, May 25, 2017 at 10:55:47PM -0700, Alexei Starovoitov wrote:
> 
> > +++ b/kernel/bpf/arraymap.c
> > @@ -462,26 +462,22 @@ static void *perf_event_fd_array_get_ptr(struct bpf_map *map,
> >  
> >  	event = perf_file->private_data;
> >  	ee = ERR_PTR(-EINVAL);
> > +	/* Per-task events are not supported */
> > +	if (event->attach_state & PERF_ATTACH_TASK)
> > +		goto err_out;
> >  
> >  	attr = perf_event_attrs(event);
> >  	if (IS_ERR(attr) || attr->inherit)
> >  		goto err_out;
> 
> > +	/* TRACEPOINT and BREAKPOINT not supported in perf_event_read_local */
> 
> I cannot find reason for this comment. That is, why would
> perf_event_read_local() not support those two types?
> 
> > +	if (attr->type == PERF_TYPE_TRACEPOINT ||
> > +	    attr->type == PERF_TYPE_BREAKPOINT)
> > +		goto err_out;
> >  
> > +	ee = bpf_event_entry_gen(perf_file, map_file);
> > +	if (ee)
> > +		return ee;
> > +	ee = ERR_PTR(-ENOMEM);
> >  
> >  err_out:
> >  	fput(perf_file);


Do we want something like the below to replace much of the above?

	if (!perf_event_valid_local(event, NULL, cpu))
		goto err_out;

Seems to be roughly what you're after, although I suppose @cpu might be
hard to determine a priory, so maybe we should allow a magic value to
short-circuit that test.

---
 kernel/events/core.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 8d6acaeeea17..a7dc34f19568 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3630,6 +3630,36 @@ static inline u64 perf_event_count(struct perf_event *event)
 }
 
 /*
+ * perf_event_valid_local() - validates if the event is usable by perf_event_read_local()
+ * event: the event to validate
+ * task:  the task the @event will be used in
+ * cpu:   the cpu the @event will be used on
+ *
+ * In case one wants to disallow all per-task events, use @task = NULL.
+ * In case one wants to disallow all per-cpu events, use @cpu = -1.
+ */
+bool perf_event_valid_local(struct perf_event *event, struct task_struct *task, int cpu)
+{
+	/* See perf_event_read_local() for the reasons for these tests */
+
+	if ((event->attach_state & PERF_ATTACH_TASK) &&
+	    event->hw.target != task)
+		return false;
+
+	if (!(event->attach_state & PERF_ATTACH_TASK) &&
+	    event->cpu != cpu)
+		return false;
+
+	if (event->attr.inherit)
+		return false;
+
+	if (event->pmu->count)
+		return false;
+
+	return true;
+}
+
+/*
  * NMI-safe method to read a local event, that is an event that
  * is:
  *   - either for the current task, or for this CPU