Messages in this thread | | | From | "Zhang, Rui" <> | Subject | Re: [PATCH V3 1/2] powercap: intel_rapl: Introduce APIs for PMU support | Date | Sun, 28 Apr 2024 09:35:49 +0000 |
| |
> > +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
| |