From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754157AbcAOR6G (ORCPT ); Fri, 15 Jan 2016 12:58:06 -0500 Received: from casper.infradead.org ([85.118.1.10]:39826 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752679AbcAOR6D (ORCPT ); Fri, 15 Jan 2016 12:58:03 -0500 Date: Fri, 15 Jan 2016 18:57:59 +0100 From: Peter Zijlstra To: Alexander Shishkin Cc: Ingo Molnar , linux-kernel@vger.kernel.org, vince@deater.net, eranian@google.com, Arnaldo Carvalho de Melo , Jiri Olsa Subject: Re: [PATCH] perf: Synchronously cleanup child events Message-ID: <20160115175759.GL3421@worktop> References: <20160115130932.GL6357@twins.programming.kicks-ass.net> <1452866861-17680-1-git-send-email-alexander.shishkin@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1452866861-17680-1-git-send-email-alexander.shishkin@linux.intel.com> User-Agent: Mutt/1.5.22.1 (2013-10-16) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 15, 2016 at 04:07:41PM +0200, Alexander Shishkin wrote: > int perf_event_release_kernel(struct perf_event *event) > { > + struct perf_event *child, *tmp; > + LIST_HEAD(child_list); > > + if (!is_kernel_event(event)) > + perf_remove_from_owner(event); > > + event->owner = NULL; > > + /* > + * event::child_mutex nests inside ctx::lock, so move children > + * to a safe place first and avoid inversion > + */ > + mutex_lock(&event->child_mutex); > + list_splice_init(&event->child_list, &child_list); > + mutex_unlock(&event->child_mutex); I suspect this races against inherit_event(), like: inherit_event() perf_event_release_kernel() if (is_orphaned_event(parent_event) /* false */ event->owner = NULL mutex_lock(child_mutex); list_splice mutex_unlock(child_mutex); mutex_lock(child_mutex); list_add_tail mutex_unlock(child_mutex); Something like this would fix that I think, not sure its the best way though... --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -979,8 +979,8 @@ static void put_ctx(struct perf_event_co * Lock order: * task_struct::perf_event_mutex * perf_event_context::mutex - * perf_event_context::lock * perf_event::child_mutex; + * perf_event_context::lock * perf_event::mmap_mutex * mmap_sem */ @@ -8956,6 +8958,16 @@ inherit_event(struct perf_event *parent_ if (parent_event->parent) parent_event = parent_event->parent; + WARN_ON_ONCE(parent_event->ctx->parent_ctx); + /* + * Serialize against perf_event_kernel_release()'s orphanage.. + */ + mutex_lock(&parent_event->child_mutex); + if (is_orphaned_event(parent_event)) { + mutex_unlock(&parent_event->child_mutex); + return NULL; + } + child_event = perf_event_alloc(&parent_event->attr, parent_event->cpu, child, @@ -8964,8 +8976,8 @@ inherit_event(struct perf_event *parent_ if (IS_ERR(child_event)) return child_event; - if (is_orphaned_event(parent_event) || - !atomic_long_inc_not_zero(&parent_event->refcount)) { + if (!atomic_long_inc_not_zero(&parent_event->refcount)) { + mutex_unlock(&parent_event->child_mutex); free_event(child_event); return NULL; } @@ -9013,8 +9025,6 @@ inherit_event(struct perf_event *parent_ /* * Link this into the parent event's child list */ - WARN_ON_ONCE(parent_event->ctx->parent_ctx); - mutex_lock(&parent_event->child_mutex); list_add_tail(&child_event->child_list, &parent_event->child_list); mutex_unlock(&parent_event->child_mutex);