cpufreq: intel_pstate: Avoid initializing variables prematurely

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 <[email protected]>

---
V1 -> V2: Rewrite changelog for better explanation.

---
 drivers/cpufreq/intel_pstate.c | 35 +++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

drivers/cpufreq/intel_pstate.c +0 -0

index 2548ec92faa2..b85e340520d9 100644

drivers/cpufreq/intel_pstate.c +2 -3

        if (!cpu->valid_pss_table)
                return;
 

@@ -539,9 +538,8 @@ static void intel_pstate_hybrid_hwp_adjust(struct cpudata *cpu)
 static inline void update_turbo_state(void)
 {
        u64 misc_en;
-       struct cpudata *cpu;
+       struct cpudata *cpu = all_cpu_data[0];
 
-       cpu = all_cpu_data[0];
        rdmsrl(MSR_IA32_MISC_ENABLE, misc_en);
        global.turbo_disabled =
                (misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE ||

@@ -769,7 +767,7 @@ static struct cpufreq_driver intel_pstate;
 static ssize_t store_energy_performance_preference(
                struct cpufreq_policy *policy, const char *buf, size_t count)
 {
-       struct cpudata *cpu = all_cpu_data[policy->cpu];
+       struct cpudata *cpu;
        char str_preference[21];
        bool raw = false;
        ssize_t ret;

drivers/cpufreq/intel_pstate.c +1 -1

        mutex_lock(&intel_pstate_limits_lock);
 
        if (intel_pstate_driver == &intel_pstate) {

@@ -1297,7 +1297,7 @@ static void update_qos_request(enum freq_qos_req_type type)
        int i;
 
        for_each_possible_cpu(i) {
-               struct cpudata *cpu = all_cpu_data[i];
+               struct cpudata *cpu;
                unsigned int freq, perf_pct;
 
                policy = cpufreq_cpu_get(i);

drivers/cpufreq/intel_pstate.c +1 -1

@@ -1579,7 +1581,7 @@ static cpumask_t hwp_intr_enable_mask;
 
 void notify_hwp_interrupt(void)
 {
-       unsigned int this_cpu = smp_processor_id();
+       unsigned int this_cpu;
        struct cpudata *cpudata;
        unsigned long flags;
        u64 value;

drivers/cpufreq/intel_pstate.c +1 -1

@@ -2024,8 +2028,8 @@ static int hwp_boost_hold_time_ns = 3 * NSEC_PER_MSEC;
 
 static inline void intel_pstate_hwp_boost_up(struct cpudata *cpu)
 {
+       u64 hwp_cap;
        u64 hwp_req = READ_ONCE(cpu->hwp_req_cached);
-       u64 hwp_cap = READ_ONCE(cpu->hwp_cap_cached);
        u32 max_limit = (hwp_req & 0xff00) >> 8;
        u32 min_limit = (hwp_req & 0xff);
        u32 boost_level1;

drivers/cpufreq/intel_pstate.c +1 -4

@@ -2389,9 +2394,7 @@ static const struct x86_cpu_id intel_pstate_cpu_ee_disable_ids[] = {
 
 static int intel_pstate_init_cpu(unsigned int cpunum)
 {
-       struct cpudata *cpu;
-
-       cpu = all_cpu_data[cpunum];
+       struct cpudata *cpu = all_cpu_data[cpunum];
 
        if (!cpu) {
                cpu = kzalloc(sizeof(*cpu), GFP_KERNEL);

@@ -2431,11 +2434,13 @@ static int intel_pstate_init_cpu(unsigned int cpunum)
 
 static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
 {
-       struct cpudata *cpu = all_cpu_data[cpu_num];

drivers/cpufreq/intel_pstate.c +1 -3

@@ -2638,9 +2643,7 @@ static int intel_cpufreq_cpu_offline(struct cpufreq_policy *policy)
 
 static int intel_pstate_cpu_online(struct cpufreq_policy *policy)
 {
-       struct cpudata *cpu = all_cpu_data[policy->cpu];
-
-       pr_debug("CPU %d going online\n", cpu->cpu);
+       pr_debug("CPU %d going online\n", policy->cpu);
 
        intel_pstate_init_acpi_perf_limits(policy);
 

Quoted 27 lines · Fieah Lim

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 <[email protected]>

---
V1 -> V2: Rewrite changelog for better explanation.

Quoted 11 lines

 void notify_hwp_interrupt(void)
 {
-       unsigned int this_cpu = smp_processor_id(); +       unsigned int this_cpu;
        struct cpudata *cpudata;
        unsigned long flags;
        u64 value;

drivers/cpufreq/intel_pstate.c +0 -0

@@ -1591,6 +1593,8 @@ void notify_hwp_interrupt(void)
         if (!(value & 0x01))
                 return;
 

drivers/cpufreq/intel_pstate.c void notify_hwp_interrupt(void) · line 1600

Quoted 5 lines

drivers/cpufreq/intel_pstate.c +2 -0

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

Quoted 21 lines

drivers/cpufreq/intel_pstate.c +1 -1

@@ -2024,8 +2028,8 @@ static int hwp_boost_hold_time_ns = 3 *
 NSEC_PER_MSEC;
 
  static inline void intel_pstate_hwp_boost_up(struct cpudata *cpu)
  {
+       u64 hwp_cap;
         u64 hwp_req = READ_ONCE(cpu->hwp_req_cached);
-       u64 hwp_cap = READ_ONCE(cpu->hwp_cap_cached);
         u32 max_limit = (hwp_req & 0xff00) >> 8;
         u32 min_limit = (hwp_req & 0xff);

        u32 boost_level1;

drivers/cpufreq/intel_pstate.c +1 -0

@@ -2052,6 +2056,7 @@ static inline void
 intel_pstate_hwp_boost_up(struct cpudata *cpu)
                 cpu->hwp_boost_min = min_limit;
 
         /* level at half way mark between min and guranteed */
+       hwp_cap = READ_ONCE(cpu->hwp_cap_cached);
         boost_level1 = (HWP_GUARANTEED_PERF(hwp_cap) + min_limit)
 >> 1;

Quoted 1 line

        if (cpu->hwp_boost_min < boost_level1)

For clarity, the original code is much better than the new one and the
only case where hwp_cap is not used is when that single read doesn't matter.  Moreover, the compiler is free to optimize it too.

Quoted 21 lines

drivers/cpufreq/intel_pstate.c +1 -3

@@ -2389,9 +2394,7 @@ static const struct x86_cpu_id
 intel_pstate_cpu_ee_disable_ids[] = {
 
  static int intel_pstate_init_cpu(unsigned int cpunum)
  {
-       struct cpudata *cpu;
-
-       cpu = all_cpu_data[cpunum];
+       struct cpudata *cpu = all_cpu_data[cpunum];
 
         if (!cpu) {

                cpu = kzalloc(sizeof(*cpu), GFP_KERNEL);

(quoted diff) +0 -0

@@ -2431,11 +2434,13 @@ static int intel_pstate_init_cpu(unsigned
 int cpunum)
 

 static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
 {
-       struct cpudata *cpu = all_cpu_data[cpu_num]; +       struct cpudata *cpu;

Quoted 8 lines

        if (hwp_active && !hwp_boost)
                return;

+       cpu = all_cpu_data[cpu_num];
+
        if (cpu->update_util_set)
                return;

This isn't a hot path.

Quoted 15 lines

drivers/cpufreq/intel_pstate.c +1 -3

@@ -2638,9 +2643,7 @@ static int intel_cpufreq_cpu_offline(struct
 cpufreq_policy *policy)
 
  static int intel_pstate_cpu_online(struct cpufreq_policy *policy)
  {
-       struct cpudata *cpu = all_cpu_data[policy->cpu];
-
-       pr_debug("CPU %d going online\n", cpu->cpu);
+       pr_debug("CPU %d going online\n", policy->cpu);
 
         intel_pstate_init_acpi_perf_limits(policy);

@@ -2649,6 +2652,8 @@ static int intel_pstate_cpu_online(struct
 cpufreq_policy *policy)
                  * Re-enable HWP and clear the "suspended" flag to

Quoted 7 lines

let "resume"
                 * know that it need not do that.
                 */
+               struct cpudata *cpu = all_cpu_data[policy->cpu]; +
                intel_pstate_hwp_reenable(cpu); cpu->suspended = false;

Same here and I don't see why the change matters.

Quoted 2 lines

        }
--

There is one piece in this patch that may be regarded as useful. Please send it separately.

Quoted 28 lines · Rafael J. Wysocki

Quoted 27 lines · Fieah Lim

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 <[email protected]>

---
V1 -> V2: Rewrite changelog for better explanation.

[...]

Quoted 15 lines

Quoted 15 lines

void notify_hwp_interrupt(void)
{

drivers/cpufreq/intel_pstate.c +3 -1

-       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);
 

Quoted 4 lines

drivers/cpufreq/intel_pstate.c void notify_hwp_interrupt(void) · line 1600

Quoted 1 line

drivers/cpufreq/intel_pstate.c +0 -0

        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)

Quoted 55 lines · srinivas pandruvada

Quoted 29 lines · Rafael J. Wysocki

Quoted 27 lines · Fieah Lim

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 <[email protected]>

---
V1 -> V2: Rewrite changelog for better explanation.

[...]

Quoted 20 lines

Quoted 8 lines

 void notify_hwp_interrupt(void)
 {
-       unsigned int this_cpu = smp_processor_id(); +       unsigned int this_cpu;
        struct cpudata *cpudata;
        unsigned long flags;
        u64 value;

drivers/cpufreq/intel_pstate.c void notify_hwp_interrupt(void) · line 1600

Quoted 8 lines

drivers/cpufreq/intel_pstate.c +2 -0

@@ -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?

Quoted 3 lines

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.

Quoted 61 lines · Rafael J. Wysocki

Quoted 58 lines · srinivas pandruvada

Quoted 29 lines · Rafael J. Wysocki

Quoted 27 lines · Fieah Lim

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 <[email protected]>

---
V1 -> V2: Rewrite changelog for better explanation.

[...]

Quoted 15 lines

Quoted 15 lines

void notify_hwp_interrupt(void)
{

drivers/cpufreq/intel_pstate.c +3 -1

-       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);
 

Quoted 5 lines

drivers/cpufreq/intel_pstate.c void notify_hwp_interrupt(void) · line 1600

Quoted 1 line

drivers/cpufreq/intel_pstate.c +0 -0

        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)

Quoted 4 lines

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.

It will have neglible effect.

I can measure it, but may be next week.