On Fri, Jul 11, 2014 at 01:56:19PM +0200, Jiri Olsa wrote: > From: Jiri Olsa <jo...@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 <jo...@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.
pgppbNPvW0WVG.pgp
Description: PGP signature
