Re: [PATCH 2/5] perf: Destroy event's children on task exit

[Peter Zijlstra]

On Fri, Jul 11, 2014 at 01:56:19PM +0200, Jiri Olsa wrote:
> From: Jiri Olsa <jolsa@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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.

Attachment: pgpSY0AmqYpBr.pgp
Description: PGP signature