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

On Wed, Jan 27, 2016 at 10:58:22AM +0100, Peter Zijlstra wrote: > > > 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,

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

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

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

On Wed, Jan 27, 2016 at 10:58:22AM +0100, Peter Zijlstra wrote: > > > 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,

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

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

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

On Tue, Jan 26, 2016 at 06:24:25PM +0100, Peter Zijlstra wrote: > On Tue, Jan 26, 2016 at 05:16:37PM +0100, Peter Zijlstra wrote: > > > +struct file *perf_event_get(unsigned int fd) > > > { > > > + struct file *file; > > > > > > + file = fget_raw(fd); > > > > fget_raw() to guarantee the return

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

On Tue, Jan 26, 2016 at 05:16:37PM +0100, Peter Zijlstra wrote: > > +struct file *perf_event_get(unsigned int fd) > > { > > + struct file *file; > > > > + file = fget_raw(fd); > > fget_raw() to guarantee the return value isn't NULL? afaict the O_PATH > stuff does not apply to perf events,

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

On Mon, Jan 25, 2016 at 08:59:49PM -0800, Alexei Starovoitov wrote: > I think I understand what you're trying to do and > the patch looks good to me. Great, thanks! > As far as BPF side I did the following... > does it match the model you outlined above? Yes, a few comments/questions below. >

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

On Mon, Jan 25, 2016 at 08:59:49PM -0800, Alexei Starovoitov wrote: > I think I understand what you're trying to do and > the patch looks good to me. Great, thanks! > As far as BPF side I did the following... > does it match the model you outlined above? Yes, a few comments/questions below. >

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

On Tue, Jan 26, 2016 at 05:16:37PM +0100, Peter Zijlstra wrote: > > +struct file *perf_event_get(unsigned int fd) > > { > > + struct file *file; > > > > + file = fget_raw(fd); > > fget_raw() to guarantee the return value isn't NULL? afaict the O_PATH > stuff does not apply to perf events,

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

On Tue, Jan 26, 2016 at 06:24:25PM +0100, Peter Zijlstra wrote: > On Tue, Jan 26, 2016 at 05:16:37PM +0100, Peter Zijlstra wrote: > > > +struct file *perf_event_get(unsigned int fd) > > > { > > > + struct file *file; > > > > > > + file = fget_raw(fd); > > > > fget_raw() to guarantee the return

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

On Mon, Jan 25, 2016 at 10:04:10PM +0100, Peter Zijlstra wrote: > On Mon, Jan 25, 2016 at 03:54:14PM +0100, Peter Zijlstra wrote: > > Alexander, Alexei, > > > > How about the below? That uses event->state == PERF_EVENT_STATE_EXIT to > > indicate the event has been given up by its 'owner' and

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

On Mon, Jan 25, 2016 at 03:54:14PM +0100, Peter Zijlstra wrote: > Alexander, Alexei, > > How about the below? That uses event->state == PERF_EVENT_STATE_EXIT to > indicate the event has been given up by its 'owner' and decouples us > from the actual event->owner logic. > > This retains the

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

On Mon, Jan 25, 2016 at 12:48:46PM +0100, Peter Zijlstra wrote: > On Fri, Jan 22, 2016 at 11:44:03AM -0800, Alexei Starovoitov wrote: > > On Fri, Jan 22, 2016 at 01:38:47PM +0100, Peter Zijlstra wrote: > > > On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote: > > > > Peter Zijlstra

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

On Fri, Jan 22, 2016 at 11:44:03AM -0800, Alexei Starovoitov wrote: > On Fri, Jan 22, 2016 at 01:38:47PM +0100, Peter Zijlstra wrote: > > On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote: > > > Peter Zijlstra writes: > > > > > > > So I think there's a number of problems still

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

On Mon, Jan 25, 2016 at 10:04:10PM +0100, Peter Zijlstra wrote: > On Mon, Jan 25, 2016 at 03:54:14PM +0100, Peter Zijlstra wrote: > > Alexander, Alexei, > > > > How about the below? That uses event->state == PERF_EVENT_STATE_EXIT to > > indicate the event has been given up by its 'owner' and

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

On Mon, Jan 25, 2016 at 12:48:46PM +0100, Peter Zijlstra wrote: > On Fri, Jan 22, 2016 at 11:44:03AM -0800, Alexei Starovoitov wrote: > > On Fri, Jan 22, 2016 at 01:38:47PM +0100, Peter Zijlstra wrote: > > > On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote: > > > > Peter Zijlstra

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

On Mon, Jan 25, 2016 at 03:54:14PM +0100, Peter Zijlstra wrote: > Alexander, Alexei, > > How about the below? That uses event->state == PERF_EVENT_STATE_EXIT to > indicate the event has been given up by its 'owner' and decouples us > from the actual event->owner logic. > > This retains the

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

On Fri, Jan 22, 2016 at 11:44:03AM -0800, Alexei Starovoitov wrote: > On Fri, Jan 22, 2016 at 01:38:47PM +0100, Peter Zijlstra wrote: > > On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote: > > > Peter Zijlstra writes: > > > > > > > So I think there's a

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

On Fri, Jan 22, 2016 at 01:38:47PM +0100, Peter Zijlstra wrote: > On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote: > > Peter Zijlstra writes: > > > > > So I think there's a number of problems still :-( I've been looking at how perf_event->owner is handled and couldn't figure

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

On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote: > Peter Zijlstra writes: > > > So I think there's a number of problems still :-( > > Also, it does indeed race with > __perf_event_exit_task()/sync_child_event(), but that one I'd fix by > simply wrapping the

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

On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote: > Peter Zijlstra writes: > > > So I think there's a number of problems still :-( > > Also, it does indeed race with > __perf_event_exit_task()/sync_child_event(), but that one I'd fix by > simply wrapping the

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

Peter Zijlstra writes: > So I think there's a number of problems still :-( Also, it does indeed race with __perf_event_exit_task()/sync_child_event(), but that one I'd fix by simply wrapping the sync_child_event()/free_event() in mutex_lock(_event->child_mutex); if

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

On Fri, Jan 22, 2016 at 01:38:47PM +0100, Peter Zijlstra wrote: > On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote: > > Peter Zijlstra writes: > > > > > So I think there's a number of problems still :-( I've been looking at how perf_event->owner is

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

Peter Zijlstra writes: > So I think there's a number of problems still :-( Also, it does indeed race with __perf_event_exit_task()/sync_child_event(), but that one I'd fix by simply wrapping the sync_child_event()/free_event() in mutex_lock(_event->child_mutex); if

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

On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote: > Peter Zijlstra writes: > > > So I think there's a number of problems still :-( > > Also, it does indeed race with > __perf_event_exit_task()/sync_child_event(), but that one I'd fix by > simply wrapping

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

On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote: > Peter Zijlstra writes: > > > So I think there's a number of problems still :-( > > Also, it does indeed race with > __perf_event_exit_task()/sync_child_event(), but that one I'd fix by > simply wrapping

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

On Wed, Jan 20, 2016 at 09:32:22AM +0100, Peter Zijlstra wrote: > On Tue, Jan 19, 2016 at 01:58:19PM -0800, Alexei Starovoitov wrote: > > On Tue, Jan 19, 2016 at 09:05:58PM +0100, Peter Zijlstra wrote: > > > > The most obvious place that generates such magical references would be > > > the bpf

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

On Wed, Jan 20, 2016 at 09:32:22AM +0100, Peter Zijlstra wrote: > On Tue, Jan 19, 2016 at 01:58:19PM -0800, Alexei Starovoitov wrote: > > On Tue, Jan 19, 2016 at 09:05:58PM +0100, Peter Zijlstra wrote: > > > > The most obvious place that generates such magical references would be > > > the bpf