Re: [PATCH v2] perf: Synchronously cleanup child events

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Jan 26, 2016 at 03:31:59PM -0800, Alexei Starovoitov wrote:

> This patch will conflict with kernel/bpf/arraymap.c and
> kernel/trace/bpf_trace.c that are planned for net-next,
> but the conflicts in kernel/events/core.c are probably harder
> to resolve, so yes please take it into tip/perf.

Thanks.

> I think your scm_right fixes depend on this patch and together
> it's an important bug fix, so probably makes sense to send
> them right now without waiting for the next merge window?

I'll leave that up to Ingo, but likely yes.

> As soon as you get the whole thing into tip, I'll test it
> to make sure bpf side is ok and I hope Wang will test it too.
> 
> I'm still a bit concerned about taking file reference for this,
> since bpf prorgams that use perf_events won't be able to be
> 'detached'. 

I was not aware BPF could be detached like that.

> Meaning there gotta be always a user space process
> that will be holding perf_event FDs.

By using fget() the BPF array thing will hold the FDs, right? I mean
once you do a full fget() userspace can go and kill itself, the struct
file will persists.

> On networking side we
> don't have this limitation. Like we can attach bpf to TC,
> iproute2 will exit and reattach some time later. So it
> kinda sux, but sounds like you want to get rid of
> perf_event->refcnt completely, 

We cannot actually get rid of it, we need it for some existence stuff.
But yes, we need stricter cleanup.

> so I don't see any other way.
> We can fix it later if it really becomes an issue.

One possible avenue would be to allow BPF to create its own (kernel)
events and manage its lifetime along with the BPF object. But so far I
don't see a problem with the file thing.