From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753932AbcANKo7 (ORCPT ); Thu, 14 Jan 2016 05:44:59 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:35765 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753584AbcANKo5 (ORCPT ); Thu, 14 Jan 2016 05:44:57 -0500 Date: Thu, 14 Jan 2016 11:44:49 +0100 From: Peter Zijlstra To: Alexander Shishkin Cc: mingo@kernel.org, eranian@google.com, linux-kernel@vger.kernel.org, vince@deater.net, dvyukov@google.com, andi@firstfloor.org, jolsa@redhat.com Subject: Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users Message-ID: <20160114104449.GW6344@twins.programming.kicks-ass.net> References: <20160111162458.427203780@infradead.org> <20160111163229.411314288@infradead.org> <87r3hl8qxp.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87r3hl8qxp.fsf@ashishki-desk.ger.corp.intel.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 13, 2016 at 05:00:50PM +0200, Alexander Shishkin wrote: > I think I caught one, below. > > Peter Zijlstra writes: > > > +static int event_function(void *info) > > +{ > > + struct event_function_struct *efs = info; > > + struct perf_event *event = efs->event; > > + struct perf_event_context *ctx = event->ctx; > > + struct perf_cpu_context *cpuctx = __get_cpu_context(ctx); > > + struct perf_event_context *task_ctx = cpuctx->task_ctx; > > + > > + WARN_ON_ONCE(!irqs_disabled()); > > + > > + /* > > + * Since we do the IPI call without holding ctx->lock things can have > > + * changed, double check we hit the task we set out to hit. > > + * > > + * If ctx->task == current, we know things must remain valid because > > + * we have IRQs disabled so we cannot schedule. > > + */ > > + if (ctx->task) { > > + if (ctx->task != current) > > + return -EAGAIN; > > + > > + WARN_ON_ONCE(task_ctx != ctx); > > Looks like between dropping ctx::lock in event_function_call() and here, > cpuctx::task_ctx may still become NULL. You were indeed correct, and I've found out how and why. Its a race with perf_event_exit_task(), which will schedule the context out. I think there's a bunch of races around this area, but in general I was planning on making event_function_call() a no-op if !(event->attach & PERF_ATTACH_CONTEXT). You cannot rely on ->is_active here, because while the context will be inactive, you still do not want it to call ->func(). Let me prod at this a bit more.