Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <60b70751a1a1ad786f534ca0b7fd3cc423736f0a.camel@linux.intel.com>
Subject: Re: [PATCH] dmaengine:idxd: Use local64_try_cmpxchg in
 perfmon_pmu_event_update
From:   Tom Zanussi <tom.zanussi@linux.intel.com>
To:     Dave Jiang <dave.jiang@intel.com>, Uros Bizjak <ubizjak@gmail.com>,
        dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Fenghua Yu <fenghua.yu@intel.com>, Vinod Koul <vkoul@kernel.org>,
        "Zanussi, Tom" <tom.zanussi@intel.com>
Date:   Mon, 10 Jul 2023 16:28:10 -0500
In-Reply-To: <e816aa75-4588-dae4-2d01-6f5ba9d4a4f3@intel.com>
References: <20230703145346.5206-1-ubizjak@gmail.com>
         <e816aa75-4588-dae4-2d01-6f5ba9d4a4f3@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.1-0ubuntu1 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-07-05 at 07:53 -0700, Dave Jiang wrote:
>=20
>=20
> On 7/3/23 07:52, Uros Bizjak wrote:
> > Use local64_try_cmpxchg instead of local64_cmpxchg (*ptr, old, new) =3D=
=3D old
> > in perfmon_pmu_event_update.=C2=A0 x86 CMPXCHG instruction returns succ=
ess in
> > ZF flag, so this change saves a compare after cmpxchg (and related move
> > instruction in front of cmpxchg).
> >=20
> > Also, try_cmpxchg implicitly assigns old *ptr value to "old" when cmpxc=
hg
> > fails. There is no need to re-read the value in the loop.
> >=20
> > No functional change intended.
> >=20
> > Cc: Fenghua Yu <fenghua.yu@intel.com>
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
>=20
>=20
> Cc: Tom Zanussi
>=20
> Tom do you mind review this patch? Thanks!

Looks fine to me.

Thanks,

Reviewed-by: Tom Zanussi <tom.zanussi@linux.intel.com>

>=20
> > ---
> > =C2=A0 drivers/dma/idxd/perfmon.c | 7 +++----
> > =C2=A0 1 file changed, 3 insertions(+), 4 deletions(-)
> >=20
> > diff --git a/drivers/dma/idxd/perfmon.c b/drivers/dma/idxd/perfmon.c
> > index d73004f47cf4..fdda6d604262 100644
> > --- a/drivers/dma/idxd/perfmon.c
> > +++ b/drivers/dma/idxd/perfmon.c
> > @@ -245,12 +245,11 @@ static void perfmon_pmu_event_update(struct perf_=
event *event)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int shift =3D 64 - idxd=
->idxd_pmu->counter_width;
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct hw_perf_event *h=
wc =3D &event->hw;
> > =C2=A0=20
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0prev_raw_count =3D local64_r=
ead(&hwc->prev_count);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0do {
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0prev_raw_count =3D local64_read(&hwc->prev_count);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0new_raw_count =3D perfmon_pmu_read_counter(event=
);
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} while (local64_cmpxchg(&hw=
c->prev_count, prev_raw_count,
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0new_ra=
w_count) !=3D prev_raw_count);
> > -
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} while (!local64_try_cmpxch=
g(&hwc->prev_count,
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 &p=
rev_raw_count, new_raw_count));
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0n =3D (new_raw_count <<=
 shift);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0p =3D (prev_raw_count <=
< shift);
> > =C2=A0=20