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;
MIME-Version: 1.0
References: <20230523085045.29391-1-kweifat@gmail.com> <CAJZ5v0ifp1088wY7o=7pnBVBm=_3H0M4sfq6=gmyChZD6R9g1g@mail.gmail.com>
 <b5e35f904174905d8f90df3f49944b22389126c7.camel@linux.intel.com>
In-Reply-To: <b5e35f904174905d8f90df3f49944b22389126c7.camel@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 23 May 2023 15:37:25 +0200
Message-ID: <CAJZ5v0ioQm95ZQ5LCCoDtVNX1TVQN_=sgzB_RRe5SAOOucpWJg@mail.gmail.com>
Subject: Re: [PATCH v2] cpufreq: intel_pstate: Avoid initializing variables prematurely
To:     srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Fieah Lim <kweifat@gmail.com>, lenb@kernel.org,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue, May 23, 2023 at 2:20 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Tue, 2023-05-23 at 13:08 +0200, Rafael J. Wysocki wrote:
> > On Tue, May 23, 2023 at 10:51 AM Fieah Lim <kweifat@gmail.com> wrote:
> > >
> > > 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:
> > >
> > >  - cpudata *cpu = all_cpu_data; (in everywhere)
> > >  - this_cpu = smp_processor_id(); (in notify_hwp_interrupt)
> > >  - hwp_cap = READ_ONCE(cpu->hwp_cap_cached); (in
> > > intel_pstate_hwp_boost_up)
> > >
> > > 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.
> > >
> > > While at it, tidy up how and when we initialize
> > > all of the cpu_data pointers, for the sake of consistency.
> > >
> > > 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.
> > >
> > > Signed-off-by: Fieah Lim <kweifat@gmail.com>
> > > ---
> > > V1 -> V2: Rewrite changelog for better explanation.
> > >
>
> [...]
>
> > >  void notify_hwp_interrupt(void)
> > >  {
> > > -       unsigned int this_cpu = smp_processor_id();
> > > +       unsigned int this_cpu;
> > >         struct cpudata *cpudata;
> > >         unsigned long flags;
> > >         u64 value;
> > > @@ -1591,6 +1593,8 @@ void notify_hwp_interrupt(void)
> > >         if (!(value & 0x01))
> > >                 return;
> > >
> > > +       this_cpu = smp_processor_id();
> > > +
> > >         spin_lock_irqsave(&hwp_notify_lock, flags);
> > >
> > >         if (!cpumask_test_cpu(this_cpu, &hwp_intr_enable_mask))
> >
> > This is a place where it may really matter for performance, but how
> > much?  Can you actually estimate this?
>
> If DEBUG_PREEMPT is defined
> ~12 instructions (most of them with latency = 1 in dependency chain)

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.

But yes, skipping it if not needed at least makes some sense.