On Thu, Jan 21, 2016 at 04:10:40PM +0100, Peter Zijlstra wrote:
> On Thu, Jan 21, 2016 at 10:42:35PM +0800, Huang Rui wrote:
> > > > @@ -164,14 +171,14 @@ static int pmu_event_add(struct perf_event
> > > > *event, int mode)
> > > > struct hw_perf_event *hwc = &event->hw;
> > > > unsigned long flags;
> > > >
> > > > - spin_lock_irqsave(&pmu->lock, flags);
> > > > + raw_spin_lock_irqsave(&pmu->lock, flags);
> > > >
> > > > hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
> > > >
> > > > if (mode & PERF_EF_START)
> > > > __pmu_event_start(pmu, event);
> > > >
> > > > - spin_unlock_irqrestore(&pmu->lock, flags);
> > > > + raw_spin_unlock_irqrestore(&pmu->lock, flags);
> > > >
> > > > return 0;
> > > > }
> > >
> > > So for these 4 {start,stop,add,del} you can drop the irqsave/irqrestore
> > > thing as its guaranteed that IRQs will be disabled.
> > >
> >
> > OK, I will remove the lock.
>
> No, the lock seems needed, as the list is global. Just the
> irqsave/irqrestore part is superfluous.
>
But actually, the lock is only used at {start,stop,add,del}. If we
drop irqsave/irqrestore on these 4 things, there won't be any use
cases.
Thanks,
Rui
