From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathieu Desnoyers Subject: Re: [PATCH] PERF(kernel): Cleanup power events V2 Date: Tue, 26 Oct 2010 14:14:21 -0400 Message-ID: <20101026181421.GA30090@Krystal> References: <1287488171-25303-3-git-send-email-trenn@suse.de> <1288049630-10947-1-git-send-email-trenn@suse.de> <20101026071020.GC13036@elte.hu> <4CC6FC03.708@linux.intel.com> <1288115894.3673.12.camel@laptop> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail.openrapids.net ([64.15.138.104]:43549 "EHLO blackscsi.openrapids.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933445Ab0JZSOY (ORCPT ); Tue, 26 Oct 2010 14:14:24 -0400 Content-Disposition: inline In-Reply-To: <1288115894.3673.12.camel@laptop> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Peter Zijlstra , Greg Kroah-Hartman Cc: Pierre Tardy , Arjan van de Ven , Ingo Molnar , Thomas Renninger , Linus Torvalds , Andrew Morton , Thomas Gleixner , Masami Hiramatsu , Frank Eigler , Steven Rostedt , Kevin Hilman , linux-omap@vger.kernel.org, rjw@sisk.pl, linux-pm@lists.linux-foundation.org, linux-trace-users@vger.kernel.org, Jean Pihet , Frederic Weisbecker , Tejun Heo * Peter Zijlstra (peterz@infradead.org) wrote: > On Tue, 2010-10-26 at 11:56 -0500, Pierre Tardy wrote: > > > > + trace_runtime_pm_usage(dev, atomic_read(&dev->power.usage_count)+1); > > atomic_inc(&dev->power.usage_count); > > That's terribly racy.. Looking at the original code, it looks racy even without considering the tracepoint: int __pm_runtime_get(struct device *dev, bool sync) { int retval; + trace_runtime_pm_usage(dev, atomic_read(&dev->power.usage_count)+1); atomic_inc(&dev->power.usage_count); retval = sync ? pm_runtime_resume(dev) : pm_request_resume(dev); There is no implied memory barrier after "atomic_inc". So either all these inc/dec are protected with mutexes or spinlocks, in which case one might wonder why atomic operations are used at all, or it's a racy mess. (I vote for the second option) kref should certainly be used there. About the instrumentation, well... the only way to have something that's not racy would be to instrument kref directly, and use atomic_add_return() in both the get/put paths. But I fear that the performance impact on many architectures might be significant (turning atomic_add + smp_mb() into a cmpxchg()). Maybe it could be acceptable as a kernel debug option. Thanks, Mathieu -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com