From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754014Ab1AZTca (ORCPT ); Wed, 26 Jan 2011 14:32:30 -0500 Received: from bombadil.infradead.org ([18.85.46.34]:40874 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753661Ab1AZTc2 convert rfc822-to-8bit (ORCPT ); Wed, 26 Jan 2011 14:32:28 -0500 Subject: Re: Q: perf_install_in_context/perf_event_enable are racy? From: Peter Zijlstra To: Oleg Nesterov Cc: Frederic Weisbecker , Ingo Molnar , Alan Stern , Arnaldo Carvalho de Melo , Paul Mackerras , Prasad , Roland McGrath , linux-kernel@vger.kernel.org In-Reply-To: <1296068731.15234.6.camel@laptop> References: <20110119182141.GA12183@redhat.com> <20110120193033.GA13924@redhat.com> <1295611905.28776.269.camel@laptop> <20110121130323.GA12900@elte.hu> <1295617185.28776.273.camel@laptop> <20110121142616.GA31165@redhat.com> <1295622304.28776.293.camel@laptop> <20110121204014.GA2870@nowhere> <20110124114234.GA12166@redhat.com> <20110126175322.GA28617@redhat.com> <20110126184957.GA32578@redhat.com> <1296068731.15234.6.camel@laptop> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Date: Wed, 26 Jan 2011 20:33:03 +0100 Message-ID: <1296070383.15234.10.camel@laptop> Mime-Version: 1.0 X-Mailer: Evolution 2.30.3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2011-01-26 at 20:05 +0100, Peter Zijlstra wrote: > On Wed, 2011-01-26 at 19:49 +0100, Oleg Nesterov wrote: > > On 01/26, Oleg Nesterov wrote: > > > > > > Please see the untested patch below. It doesn't change perf_event_enable(), > > > only perf_install_in_context(). > > > > Forgot to mention... Also, it doesn't try to fix the race with do_exit(), > > this needs another change. > > > > And, damn, can't resist. This is mostly cosmetic issue, but I feel > > discomfort every time I look at task_oncpu_function_call(). It _looks_ > > obviously wrong, even if the problem doesn't exist in practice. I'll > > send the pedantic fix to keep the maintainers busy ;) > > I've been trying to sit down and work my way through it today, your last > suggestion very nearly seemed to make sense, but I kept getting > distracted. > > FWIW I think perf_event_enable() has the very same issue... Wouldn't something like the below cure things too? --- kernel/sched.c | 23 ++++++++++++++++++++++- 1 files changed, 22 insertions(+), 1 deletions(-) diff --git a/kernel/sched.c b/kernel/sched.c index 18d38e4..7eadbcf 100644 --- a/kernel/sched.c +++ b/kernel/sched.c @@ -2265,6 +2265,22 @@ void kick_process(struct task_struct *p) EXPORT_SYMBOL_GPL(kick_process); #endif /* CONFIG_SMP */ +struct task_function_call { + struct task_struct *p; + void (*func)(void *info); + void *info; +}; + +void task_function_trampoline(void *data) +{ + struct task_function_call *tfc = data; + + if (this_rq()->curr != tfc->p) + return; + + tfc->func(tfc->data); +} + /** * task_oncpu_function_call - call a function on the cpu on which a task runs * @p: the task to evaluate @@ -2278,11 +2294,16 @@ void task_oncpu_function_call(struct task_struct *p, void (*func) (void *info), void *info) { int cpu; + struct task_function_call data = { + .p = p, + .func = func, + .info = info, + }; preempt_disable(); cpu = task_cpu(p); if (task_curr(p)) - smp_call_function_single(cpu, func, info, 1); + smp_call_function_single(cpu, task_function_trampoline, &data, 1); preempt_enable(); }