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: <96d81ba4c501134c77869a08b843b39fc1bf7526.camel@linux.intel.com>
Subject: Re: [PATCH v2] cpufreq: intel_pstate: Avoid initializing variables
 prematurely
From:   srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     Fieah Lim <kweifat@gmail.com>, lenb@kernel.org,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Tue, 23 May 2023 07:02:26 -0700
In-Reply-To: <CAJZ5v0ioQm95ZQ5LCCoDtVNX1TVQN_=sgzB_RRe5SAOOucpWJg@mail.gmail.com>
References: <20230523085045.29391-1-kweifat@gmail.com>
         <CAJZ5v0ifp1088wY7o=7pnBVBm=_3H0M4sfq6=gmyChZD6R9g1g@mail.gmail.com>
         <b5e35f904174905d8f90df3f49944b22389126c7.camel@linux.intel.com>
         <CAJZ5v0ioQm95ZQ5LCCoDtVNX1TVQN_=sgzB_RRe5SAOOucpWJg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu1 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-05-23 at 15:37 +0200, Rafael J. Wysocki wrote:
> On Tue, May 23, 2023 at 2:20=E2=80=AFPM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> >=20
> > On Tue, 2023-05-23 at 13:08 +0200, Rafael J. Wysocki wrote:
> > > On Tue, May 23, 2023 at 10:51=E2=80=AFAM Fieah Lim <kweifat@gmail.com=
>
> > > wrote:
> > > >=20
> > > > We should avoid initializing some rather expensive data
> > > > when the function has a chance to bail out earlier
> > > > before actually using it.
> > > > This applies to the following initializations:
> > > >=20
> > > > =C2=A0- cpudata *cpu =3D all_cpu_data; (in everywhere)
> > > > =C2=A0- this_cpu =3D smp_processor_id(); (in notify_hwp_interrupt)
> > > > =C2=A0- hwp_cap =3D READ_ONCE(cpu->hwp_cap_cached); (in
> > > > intel_pstate_hwp_boost_up)
> > > >=20
> > > > These initializations are premature because there is a chance
> > > > that the function will bail out before actually using the data.
> > > > I think this qualifies as a micro-optimization,
> > > > especially in such a hot path.
> > > >=20
> > > > While at it, tidy up how and when we initialize
> > > > all of the cpu_data pointers, for the sake of consistency.
> > > >=20
> > > > A side note on the intel_pstate_cpu_online change:
> > > > we simply don't have to initialize cpudata just
> > > > for the pr_debug, while policy->cpu is being there.
> > > >=20
> > > > Signed-off-by: Fieah Lim <kweifat@gmail.com>
> > > > ---
> > > > V1 -> V2: Rewrite changelog for better explanation.
> > > >=20
> >=20
> > [...]
> >=20
> > > > =C2=A0void notify_hwp_interrupt(void)
> > > > =C2=A0{
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned int this_cpu =3D smp=
_processor_id();
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned int this_cpu;
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct cpudata *cpudata;
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long flags;
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u64 value;
> > > > @@ -1591,6 +1593,8 @@ void notify_hwp_interrupt(void)
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!(value & 0x01))
> > > > =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 return;
> > > >=20
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 this_cpu =3D smp_processor_id=
();
> > > > +
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 spin_lock_irqsave(&hwp_n=
otify_lock, flags);
> > > >=20
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!cpumask_test_cpu(th=
is_cpu, &hwp_intr_enable_mask))
> > >=20
> > > This is a place where it may really matter for performance, but
> > > how
> > > much?=C2=A0 Can you actually estimate this?
> >=20
> > If DEBUG_PREEMPT is defined
> > ~12 instructions (most of them with latency =3D 1 in dependency
> > chain)
>=20
> I really meant "estimate the effect of this change on performance",
> because I'm not sure if it is going to be visible in any test.
>=20
> But yes, skipping it if not needed at least makes some sense.
It will have neglible effect. I can measure it, but may be next week.

Thanks,
Srinivas