| Message ID | 20230703150859.6176-1-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pmu: Use local64_try_cmpxchg in i915_pmu_event_read | expand |
On Tue, Jul 4, 2023 at 9:28 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Mon, 03 Jul 2023, Uros Bizjak <ubizjak@gmail.com> wrote: > > Use local64_try_cmpxchg instead of local64_cmpxchg (*ptr, old, new) == old > > in i915_pmu_event_read. x86 CMPXCHG instruction returns success in ZF flag, > > so this change saves a compare after cmpxchg (and related move instruction > > in front of cmpxchg). > > > > Also, try_cmpxchg implicitly assigns old *ptr value to "old" when cmpxchg > > fails. There is no need to re-read the value in the loop. > > > > No functional change intended. > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Cc: David Airlie <airlied@gmail.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > > --- > > drivers/gpu/drm/i915/i915_pmu.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > > index d35973b41186..108b675088ba 100644 > > --- a/drivers/gpu/drm/i915/i915_pmu.c > > +++ b/drivers/gpu/drm/i915/i915_pmu.c > > @@ -696,12 +696,11 @@ static void i915_pmu_event_read(struct perf_event *event) > > event->hw.state = PERF_HES_STOPPED; > > return; > > } > > -again: > > - prev = local64_read(&hwc->prev_count); > > - new = __i915_pmu_event_read(event); > > > > - if (local64_cmpxchg(&hwc->prev_count, prev, new) != prev) > > - goto again; > > + prev = local64_read(&hwc->prev_count); > > + do { > > + new = __i915_pmu_event_read(event); > > + } while (!local64_try_cmpxchg(&hwc->prev_count, &prev, new)); > > You could save everyone a lot of time by actually documenting what these > functions do. Assume you don't know what local64_try_cmpxchg() does, and > see how many calls you have to go through to figure it out. These functions are documented in Documentation/atomic_t.txt (under "RMW ops:" section), and the difference is explained in a separate section "CMPXCHG vs TRY_CMPXCGS" in the same file. Uros. > Because the next time I encounter this code or a patch like this, I'm > probably going to have to do that again. > > To me, the old one was more readable. The optimization is meaningless to > me if it's not quantified but reduces readability. > > > BR, > Jani. > > > > > > local64_add(new - prev, &event->count); > > } > > -- > Jani Nikula, Intel Open Source Graphics Center
On Mon, 03 Jul 2023, Uros Bizjak <ubizjak@gmail.com> wrote: > Use local64_try_cmpxchg instead of local64_cmpxchg (*ptr, old, new) == old > in i915_pmu_event_read. x86 CMPXCHG instruction returns success in ZF flag, > so this change saves a compare after cmpxchg (and related move instruction > in front of cmpxchg). > > Also, try_cmpxchg implicitly assigns old *ptr value to "old" when cmpxchg > fails. There is no need to re-read the value in the loop. > > No functional change intended. > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: David Airlie <airlied@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > --- > drivers/gpu/drm/i915/i915_pmu.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > index d35973b41186..108b675088ba 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -696,12 +696,11 @@ static void i915_pmu_event_read(struct perf_event *event) > event->hw.state = PERF_HES_STOPPED; > return; > } > -again: > - prev = local64_read(&hwc->prev_count); > - new = __i915_pmu_event_read(event); > > - if (local64_cmpxchg(&hwc->prev_count, prev, new) != prev) > - goto again; > + prev = local64_read(&hwc->prev_count); > + do { > + new = __i915_pmu_event_read(event); > + } while (!local64_try_cmpxchg(&hwc->prev_count, &prev, new)); Chased through the documentation again, and pushed to drm-intel-next. Thanks for the patch. BR, Jani. > > local64_add(new - prev, &event->count); > }
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index d35973b41186..108b675088ba 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -696,12 +696,11 @@ static void i915_pmu_event_read(struct perf_event *event) event->hw.state = PERF_HES_STOPPED; return; } -again: - prev = local64_read(&hwc->prev_count); - new = __i915_pmu_event_read(event); - if (local64_cmpxchg(&hwc->prev_count, prev, new) != prev) - goto again; + prev = local64_read(&hwc->prev_count); + do { + new = __i915_pmu_event_read(event); + } while (!local64_try_cmpxchg(&hwc->prev_count, &prev, new)); local64_add(new - prev, &event->count); }
Use local64_try_cmpxchg instead of local64_cmpxchg (*ptr, old, new) == old in i915_pmu_event_read. x86 CMPXCHG instruction returns success in ZF flag, so this change saves a compare after cmpxchg (and related move instruction in front of cmpxchg). Also, try_cmpxchg implicitly assigns old *ptr value to "old" when cmpxchg fails. There is no need to re-read the value in the loop. No functional change intended. Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: David Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> Signed-off-by: Uros Bizjak <ubizjak@gmail.com> --- drivers/gpu/drm/i915/i915_pmu.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)