Re: [PATCH net-next 4/6] perf, bpf: add perf events core support for BPF_PROG_TYPE_PERF_EVENT programs

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Mon, Aug 29, 2016 at 02:17:18PM +0200, Peter Zijlstra wrote:
> On Fri, Aug 26, 2016 at 07:31:22PM -0700, Alexei Starovoitov wrote:
> > +static int perf_event_set_bpf_handler(struct perf_event *event, u32 prog_fd)
> > +{
> > +	struct bpf_prog *prog;
> > +
> > +	if (event->overflow_handler_context)
> > +		/* hw breakpoint or kernel counter */
> > +		return -EINVAL;
> > +
> > +	if (event->prog)
> > +		return -EEXIST;
> > +
> > +	prog = bpf_prog_get_type(prog_fd, BPF_PROG_TYPE_PERF_EVENT);
> > +	if (IS_ERR(prog))
> > +		return PTR_ERR(prog);
> > +
> > +	event->prog = prog;
> > +	event->orig_overflow_handler = READ_ONCE(event->overflow_handler);
> > +	WRITE_ONCE(event->overflow_handler, bpf_overflow_handler);
> > +	return 0;
> > +}
> > +
> > +static void perf_event_free_bpf_handler(struct perf_event *event)
> > +{
> > +	struct bpf_prog *prog = event->prog;
> > +
> > +	if (!prog)
> > +		return;
> 
> Does it make sense to do something like:
> 
> 	WARN_ON_ONCE(event->overflow_handler != bpf_overflow_handler);

Yes that's an implicit assumption here, but checking for that
would be overkill. event->overflow_handler and event->prog are set
back to back in two places and reset here once together.
Such warn_on will only make people reading this code in the future
think that this bit is too complex to analyze by hand.

> > +
> > +	WRITE_ONCE(event->overflow_handler, event->orig_overflow_handler);
> > +	event->prog = NULL;
> > +	bpf_prog_put(prog);
> > +}
> 
> 
> >  static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)
> >  {
> >  	bool is_kprobe, is_tracepoint;
> >  	struct bpf_prog *prog;
> >  
> > +	if (event->attr.type == PERF_TYPE_HARDWARE ||
> > +	    event->attr.type == PERF_TYPE_SOFTWARE)
> > +		return perf_event_set_bpf_handler(event, prog_fd);
> > +
> >  	if (event->attr.type != PERF_TYPE_TRACEPOINT)
> >  		return -EINVAL;
> >  
> > @@ -7647,6 +7711,8 @@ static void perf_event_free_bpf_prog(struct perf_event *event)
> >  {
> >  	struct bpf_prog *prog;
> >  
> > +	perf_event_free_bpf_handler(event);
> > +
> >  	if (!event->tp_event)
> >  		return;
> >  
> 
> Does it at all make sense to merge the tp_event->prog thing into this
> new event->prog?

'struct trace_event_call *tp_event' is global while tp_event->perf_events
are per cpu, so I don't see how we can do that without breaking user space
logic. Right now users do single perf_event_open of kprobe and attach prog
that is executed on all cpus where kprobe is firing. Additional per-cpu
filtering is done from within bpf prog.

> >  #ifdef CONFIG_HAVE_HW_BREAKPOINT
> > @@ -8957,6 +9029,14 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
> >  	if (!overflow_handler && parent_event) {
> >  		overflow_handler = parent_event->overflow_handler;
> >  		context = parent_event->overflow_handler_context;
> > +		if (overflow_handler == bpf_overflow_handler) {
> > +			event->prog = bpf_prog_inc(parent_event->prog);
> > +			event->orig_overflow_handler = parent_event->orig_overflow_handler;
> > +			if (IS_ERR(event->prog)) {
> > +				event->prog = NULL;
> > +				overflow_handler = NULL;
> > +			}
> > +		}
> >  	}
> 
> Should we not fail the entire perf_event_alloc() call in that IS_ERR()
> case?

Yes. Good point. Will do.