Re: [PATCH V3 1/2] powercap: intel_rapl: Introduce APIs for PMU support

[Zhang, Rui]

> > +static u64 rapl_event_update(struct perf_event *event)
> > +{
> > +       struct hw_perf_event *hwc = &event->hw;
> > +       struct rapl_package_pmu_data *data =
> > event_to_pmu_data(event);
> > +       u64 prev_raw_count, new_raw_count;
> > +       s64 delta, sdelta;
> > +       s64 tmp;
> > +
> > +       do {
> > +               prev_raw_count = local64_read(&hwc->prev_count);
> > +               new_raw_count = event_read_counter(event);
> > +               tmp = local64_cmpxchg(&hwc->prev_count,
> > prev_raw_count, new_raw_count);
> > +       } while (tmp != prev_raw_count);
> 
> I think that it is only safe to call this function for draining an
> event going away, because otherwise the above may turn into an
> endless
> loop, and the function is called under a spinlock.

IMO, the logic above is needed by the hardware events that can generate
interrupts and update hwc->prev_count in the interrupt handler.

For the other PMUs, I suspect this loop actually makes any difference.
However, currently, this is the common logic followed by all PMU
drivers.

> 
> I would add a comment (above the loop) explaining that this is about
> draining, so the counter is expected to stop incrementing shortly.
> 
> The rest of the patch LGTM.
> 

Thanks,
rui