Messages in this thread | | | Date | Mon, 14 Jul 2014 13:18:33 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH 2/5] perf: Destroy event's children on task exit |
| |
On Fri, Jul 11, 2014 at 01:56:19PM +0200, Jiri Olsa wrote: > From: Jiri Olsa <jolsa@redhat.com> > > When task exits we close: > 1) all events that are installed in task > 2) all events owned by task (via file descriptor) > > But we don't close children events of 2) events. Those children > events stay until the child task exits and are useless with the > parent being gone, because we have no way to get to values any > more. > > Plus if the event stays installed in task even with the owner task > gone, it runs the perf callback any time the task forks, for no > real reason. > > Closing all children events events when the owner task of the > parent event is closed.
Do we need this for the other patches, or is this an unrelated change?
> Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > kernel/events/core.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 71a56ae..37797dd 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -7535,6 +7535,32 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn) > put_ctx(child_ctx); > } > > +static void perf_event_exit_children(struct perf_event *parent) > +{ > + struct perf_event *child, *tmp; > + > + mutex_lock(&parent->child_mutex); > + list_for_each_entry_safe(child, tmp, &parent->child_list, > + child_list) { > + struct perf_event_context *child_ctx = child->ctx; > + > + /* > + * Child events got removed from child_list under > + * child_mutex and then freed. So it's safe to access > + * childs context in here, because the child holds > + * context ref. > + */ > + mutex_lock(&child_ctx->mutex); > + perf_remove_from_context(child, true); > + mutex_unlock(&child_ctx->mutex); > + > + list_del_init(&child->child_list); > + put_event(parent); > + free_event(child); > + } > + mutex_unlock(&parent->child_mutex); > +} > + > /* > * When a child task exits, feed back event values to parent events. > */ > @@ -7555,6 +7581,7 @@ void perf_event_exit_task(struct task_struct *child) > */ > smp_wmb(); > event->owner = NULL; > + perf_event_exit_children(event); > } > mutex_unlock(&child->perf_event_mutex);
I don't think this is correct, perf_event_init_context() can come in concurrently and the first place it runs into ->child_mutex is after its already allocated and created the (first) child event.
This also means that it not only makes exit() very slow for all tasks that own events -- surmountable I suppose, but also makes clone()/fork() very slow for all decedents -- not so good.
Now I suppose we can fix this by testing event->owner in perf_event_init_context() or somesuch, thereby bypassing event creation when the owner is tearing things down.
But it would be good if we can do all this in a separate series.
[unhandled content-type:application/pgp-signature] | |