Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752531AbdIAUc6 (ORCPT ); Fri, 1 Sep 2017 16:32:58 -0400 Received: from merlin.infradead.org ([205.233.59.134]:57986 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752363AbdIAUc5 (ORCPT ); Fri, 1 Sep 2017 16:32:57 -0400 Date: Fri, 1 Sep 2017 22:32:50 +0200 From: Peter Zijlstra To: Thomas Gleixner Cc: Borislav Petkov , Sebastian Andrzej Siewior , lkml Subject: Re: WARNING: possible circular locking dependency detected Message-ID: <20170901203250.GS6524@worktop.programming.kicks-ass.net> References: <20170829194948.GD32112@worktop.programming.kicks-ass.net> <20170830054730.GF32112@worktop.programming.kicks-ass.net> <20170831073739.ytexc7omldyb5lgy@hirez.programming.kicks-ass.net> <20170831080922.daaypuxatenmpkfo@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 Content-Length: 3391 Lines: 96 On Thu, Aug 31, 2017 at 11:24:13PM +0200, Thomas Gleixner wrote: > On Thu, 31 Aug 2017, Thomas Gleixner wrote: > > On Thu, 31 Aug 2017, Peter Zijlstra wrote: > > > > > On Thu, Aug 31, 2017 at 09:55:57AM +0200, Thomas Gleixner wrote: > > > > > Arghh!!! > > > > > > > > > > And allowing us to create events for offline CPUs (possible I think, but > > > > > maybe slightly tricky) won't solve that, because we're already holding > > > > > the hotplug_lock during PREPARE. > > > > > > > > There are two ways to cure that: > > > > > > > > 1) Have a pre cpus_write_lock() stage which is serialized via > > > > cpus_add_remove_lock, which is the outer lock for hotplug. > > > > > > > > There we can sanely create stuff and fail with all consequences. > > > > > > True, if you're willing to add more state to that hotplug thing I'll try > > > and make that perf patch that allows attaching to offline CPUs. > > > > Now that I think more about it. That's going to be an interesting exercise > > vs. the hotplug state registration which relies on cpus_read_lock() > > serialization..... > > We could have that for built-in stuff which is guaranteed to be never > unregistered. Pretty restricted, but for cases like that it could > work. Famous last work ... I think something like this (on top of the previous patch that preserves the event<->cpu relation over hotplug) should allow perf_event_create_kernel_counter() to create events on offline CPUs. It will create them as if perf_event_attr::disabled=1 and requires perf_event_enable() after the CPU comes online (mirroring how offline does an implicit perf_event_disable() as per the previous patch). --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -2427,6 +2427,24 @@ perf_install_in_context(struct perf_even smp_store_release(&event->ctx, ctx); if (!task) { + /* + * Check if the @cpu we're creating an event for is offline. + * + * We use the perf_cpu_context::ctx::mutex to serialize against + * the hotplug notifiers. See perf_event_{init,exit}_cpu(). + */ + struct perf_cpu_context *cpuctx = + container_of(ctx, struct perf_cpu_context, ctx); + + if (!cpuctx->online) { + raw_spin_lock_irq(&ctx->lock); + add_event_to_context(event, ctx); + event->state = PERF_EVENT_STATE_OFF + + PERF_EVENT_STATE_HOTPLUG_OFFSET; + raw_spin_unlock_irq(&ctx->lock); + return; + } + cpu_function_call(cpu, __perf_install_in_context, event); return; } @@ -10181,6 +10199,8 @@ SYSCALL_DEFINE5(perf_event_open, * * We use the perf_cpu_context::ctx::mutex to serialize against * the hotplug notifiers. See perf_event_{init,exit}_cpu(). + * + * XXX not strictly required, preserves existing behaviour. */ struct perf_cpu_context *cpuctx = container_of(ctx, struct perf_cpu_context, ctx); @@ -10368,21 +10388,6 @@ perf_event_create_kernel_counter(struct goto err_unlock; } - if (!task) { - /* - * Check if the @cpu we're creating an event for is online. - * - * We use the perf_cpu_context::ctx::mutex to serialize against - * the hotplug notifiers. See perf_event_{init,exit}_cpu(). - */ - struct perf_cpu_context *cpuctx = - container_of(ctx, struct perf_cpu_context, ctx); - if (!cpuctx->online) { - err = -ENODEV; - goto err_unlock; - } - } - if (!exclusive_event_installable(event, ctx)) { err = -EBUSY; goto err_unlock;