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

[Jiri Olsa <jolsa@redhat.com>]

On Mon, Jul 14, 2014 at 01:18:33PM +0200, Peter Zijlstra wrote:
> 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.

just noticed this.. I'm working on the other version we decide, but FWIW
there's also mutex_lock(&child_ctx->mutex); before removing the context,
that should protect it against perf_event_init_context call

jirka