* Peter Zijlstra (peterz@xxxxxxxxxxxxx) 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 -- To unsubscribe from this list: send the line "unsubscribe linux-trace-users" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
![]() |