Re: [PATCH V2 2/3] powercap: intel_rapl: Introduce APIs for PMU support

[Zhang, Rui]

> 
> > > +static void __rapl_pmu_event_start(struct perf_event *event)
> > > +{
> > > +       struct rapl_package_pmu_data *data =
> > > event_to_pmu_data(event);
> > > +
> > > +       if (WARN_ON_ONCE(!(event->hw.state & PERF_HES_STOPPED)))
> > > +               return;
> > > +
> > > +       event->hw.state = 0;
> > > +
> > > +       list_add_tail(&event->active_entry, &data->active_list);
> > > +
> > > +       local64_set(&event->hw.prev_count,
> > > event_read_counter(event));
> > > +       if (++data->n_active == 1)
> > > +               hrtimer_start(&data->hrtimer, data-
> > > >timer_interval,
> > > +                             HRTIMER_MODE_REL_PINNED);
> > > +}
> > > +
> > > +static void rapl_pmu_event_start(struct perf_event *event, int
> > > mode)
> > > +{
> > > +       struct rapl_package_pmu_data *data =
> > > event_to_pmu_data(event);
> > > +       unsigned long flags;
> > > +
> > > +       raw_spin_lock_irqsave(&data->lock, flags);
> > > +       __rapl_pmu_event_start(event);
> > > +       raw_spin_unlock_irqrestore(&data->lock, flags);
> > 
> > Why does it need to be raw_spin_lock_?
> > 
> > What exactly is protected by data->lock?
> > 
> This is copied from MSR RAPL PMU, which exists from day 1 of the
> code.
> 
> Let me double check.
> 


RAPL Package supports multiple events, and rapl_pmu_event_start(),
rapl_pmu_event_add() and rapl_pmu_event_stop() can be invoked for each
event.

And this lock is used to protect the per package data->active_list and
data->n_active from concurrent access.

And we need to do this in the hrtimer handler (rapl_hrtimer_handle()).

Thanks for reviewing and patch V3 has been updated and submitted.

thanks,
rui