From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 527291FECBA for ; Fri, 17 Jan 2025 13:04:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=90.155.50.34 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737119073; cv=none; b=PDKRukGJ4d/adID/ZNweAgHlBVF2YBL8Z/mMrdlpC4B1DZTELYw6s0NVQRPMsmp6+WfTxFce2MKniGi9dQxudNsvvwcfbCYFKkbrR8qbQ7zKROddR89XMg8f0p6mCmOacjy2e8fABp3Kzh9sRm1hJFRNdixDGgfcgjB/dpgQxQ4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737119073; c=relaxed/simple; bh=5HGMqt9ROcl8a3vxZFg0PNUgvK8n1kvEF4hw2jS6YcM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=XaOkS2l0GOBFyZbTpxEDA+372zzhx9c5ow6rgcqFAsnGVOK5s231/s8Og/PBR4mn20w0SJjwuUP8nx1Uh/wYcHjMtFpSbtkHvmVkvdscHQ1p8LrRhuJR2XtAGnvs8UXUatIitnh6GGEDgTgh6cCGLsDGrxZ/ratHigSeCqM+2dI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org; spf=none smtp.mailfrom=infradead.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b=tDv/9igb; arc=none smtp.client-ip=90.155.50.34 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=infradead.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="tDv/9igb" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=3CZwM02ZRQoFF45kSeAVvMDkNyFiw/IUtgDO8NeDaw8=; b=tDv/9igblgWCPtbR/Q61QUoic0 jdF90fWblc/G7ZbeXCzbxIqSXe8FQ5hpQ1HSCMevtldIPMU2MmP9hc43LwcViutZbuvTBtCKxikWK QTH4BQJ3Dbne44v31UMQC5PtEpzmRtd1g3rpLyhNASDHkD0ynnJHO6i44P6oMSVsrIeQMjmEQ1Kh4 bl9lAx14nBg+/qQfY0d5vb3ZIjzF7q7OUSUSmwCE5kqSmU9NO2L/ZiAIzfSBhR7BPkOkzDYQOvt0q 1XqX8ZaY2gsjoSePJZCQ6D2klMypgdrQ7rCiJO9cAIC6fHqMyD9L2Fia5wx+XhXEvqh0k9LXggw47 xru3Pe0g==; Received: from 77-249-17-89.cable.dynamic.v4.ziggo.nl ([77.249.17.89] helo=noisy.programming.kicks-ass.net) by casper.infradead.org with esmtpsa (Exim 4.98 #2 (Red Hat Linux)) id 1tYm1R-0000000DAqV-0EJ1; Fri, 17 Jan 2025 13:04:25 +0000 Received: by noisy.programming.kicks-ass.net (Postfix, from userid 1000) id D4A0C30057A; Fri, 17 Jan 2025 14:04:23 +0100 (CET) Date: Fri, 17 Jan 2025 14:04:23 +0100 From: Peter Zijlstra To: Ravi Bangoria Cc: "mingo@kernel.org" , "lucas.demarchi@intel.com" , "linux-kernel@vger.kernel.org" , "willy@infradead.org" , "acme@kernel.org" , "namhyung@kernel.org" , "mark.rutland@arm.com" , "alexander.shishkin@linux.intel.com" , "jolsa@kernel.org" , "irogers@google.com" , "adrian.hunter@intel.com" , "kan.liang@linux.intel.com" Subject: Re: [PATCH 19/19] perf: Make perf_pmu_unregister() useable Message-ID: <20250117130423.GI8385@noisy.programming.kicks-ass.net> References: <20241104133909.669111662@infradead.org> <20241104135519.715883982@infradead.org> <20241217091216.GK35539@noisy.programming.kicks-ass.net> <20241217115219.GH12500@noisy.programming.kicks-ass.net> <8c31f7bd-871d-4a38-ad15-a16a116e1f39@amd.com> <20250117000316.GB33629@noisy.programming.kicks-ass.net> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250117000316.GB33629@noisy.programming.kicks-ass.net> On Fri, Jan 17, 2025 at 01:03:16AM +0100, Peter Zijlstra wrote: > > 2) A race with perf_event_release_kernel(). perf_event_release_kernel() > > prepares a separate "free_list" of all children events under ctx->mutex > > and event->child_mutex. However, the "free_list" uses the same > > "event->child_list" for entries. OTOH, perf_pmu_unregister() ultimately > > calls __perf_remove_from_context() with DETACH_CHILD, which checks if > > the event being removed is a child event, and if so, it will try to > > detach the child from parent using list_del_init(&event->child_list); > > i.e. two code path doing list_del on the same list entry. > > > > perf_event_release_kernel() perf_pmu_unregister() > > /* Move children events to free_list */ ... > > list_for_each_entry_safe(child, tmp, &free_list, child_list) { perf_remove_from_context() /* with DETACH_CHILD */ > > ... __perf_remove_from_context() > > list_del(&child->child_list); perf_child_detach() > > list_del_init(&event->child_list); > > Bah, I had figured it was taken care of, because perf_event_exit_event() > has a similar race. I'll try and figure out what to do there. So, the problem appears to be that perf_event_release_kernel() does not use DETACH_CHILD, doing so will clear PERF_ATTACH_CHILD, at which point the above is fully serialized by parent->child_mutex. Then the next problem is that since pmu_detach_events() can hold an extra ref on things, the free_event() from free_list will WARN, like before. Easily fixed by making that put_event(), except that messes up the whole wait_var_event() scheme -- since __free_event() does the final put_ctx(). This in turn can be fixed by pushing that wake_up_var() nonsense into put_ctx() itself. Which then gives me something like so. But also, I think we can get rid of that free_list entirely. Anyway, let me go break this up into individual patches and go test this -- after lunch! --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -1229,8 +1229,14 @@ static void put_ctx(struct perf_event_co if (refcount_dec_and_test(&ctx->refcount)) { if (ctx->parent_ctx) put_ctx(ctx->parent_ctx); - if (ctx->task && ctx->task != TASK_TOMBSTONE) - put_task_struct(ctx->task); + if (ctx->task) { + if (ctx->task == TASK_TOMBSTONE) { + smp_mb(); + wake_up_var(&ctx->refcount); + } else { + put_task_struct(ctx->task); + } + } call_rcu(&ctx->rcu_head, free_ctx); } } @@ -5550,8 +5556,6 @@ int perf_event_release_kernel(struct per again: mutex_lock(&event->child_mutex); list_for_each_entry(child, &event->child_list, child_list) { - void *var = NULL; - /* * Cannot change, child events are not migrated, see the * comment with perf_event_ctx_lock_nested(). @@ -5584,46 +5588,32 @@ int perf_event_release_kernel(struct per tmp = list_first_entry_or_null(&event->child_list, struct perf_event, child_list); if (tmp == child) { - perf_remove_from_context(child, DETACH_GROUP); - list_move(&child->child_list, &free_list); + perf_remove_from_context(child, DETACH_GROUP | DETACH_CHILD); + /* + * Can't risk calling into free_event() here, since + * event->destroy() might invert with the currently + * held locks, see 82d94856fa22 ("perf/core: Fix lock + * inversion between perf,trace,cpuhp") + */ + list_add(&child->child_list, &free_list); /* * This matches the refcount bump in inherit_event(); * this can't be the last reference. */ put_event(event); - } else { - var = &ctx->refcount; } mutex_unlock(&event->child_mutex); mutex_unlock(&ctx->mutex); put_ctx(ctx); - if (var) { - /* - * If perf_event_free_task() has deleted all events from the - * ctx while the child_mutex got released above, make sure to - * notify about the preceding put_ctx(). - */ - smp_mb(); /* pairs with wait_var_event() */ - wake_up_var(var); - } goto again; } mutex_unlock(&event->child_mutex); list_for_each_entry_safe(child, tmp, &free_list, child_list) { - void *var = &child->ctx->refcount; - list_del(&child->child_list); - free_event(child); - - /* - * Wake any perf_event_free_task() waiting for this event to be - * freed. - */ - smp_mb(); /* pairs with wait_var_event() */ - wake_up_var(var); + put_event(child); } no_ctx: