Re: [PATCH] PERF(kernel): Cleanup power events V2

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

* 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