From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754880AbcARMKE (ORCPT ); Mon, 18 Jan 2016 07:10:04 -0500 Received: from mga11.intel.com ([192.55.52.93]:37587 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754660AbcARMKB (ORCPT ); Mon, 18 Jan 2016 07:10:01 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,312,1449561600"; d="scan'208";a="892955404" From: Alexander Shishkin To: Peter Zijlstra 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 In-Reply-To: <20160115175759.GL3421@worktop> References: <20160115130932.GL6357@twins.programming.kicks-ass.net> <1452866861-17680-1-git-send-email-alexander.shishkin@linux.intel.com> <20160115175759.GL3421@worktop> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Mon, 18 Jan 2016 14:07:46 +0200 Message-ID: <878u3n850t.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Peter Zijlstra writes: > 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); Indeed, this is possible. > > 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 > */ This is, actually, the order that we have already: perf_ioctl(): ctx::mutex -> perf_event_for_each(): event::child_mutex -> _perf_event_enable(): ctx::lock that is, ctx::lock already nests inside event::child_mutex. So what you're suggesting is an ok solution. Regards, -- Alex