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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <299a61dffc891af6f5896ba77e500803322fdb04.camel@intel.com>
Date: Sun, 28 Apr 2024 09:35:49 +0000
From: "Zhang, Rui" <rui.zhang@...el.com>
To: "rafael@...nel.org" <rafael@...nel.org>
CC: "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>, "Wysocki, Rafael J"
	<rafael.j.wysocki@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Pandruvada, Srinivas"
	<srinivas.pandruvada@...el.com>
Subject: Re: [PATCH V3 1/2] powercap: intel_rapl: Introduce APIs for PMU
 support

> > +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