intel_pstate: Some code cleanups, mostly related to concurrency
Subsystems (2)
cover intel_pstate: Some code cleanups, mostly related to concurrency
Hi Everyone,
This series consists of a few cleanups of the intep_pstate driver, mostly related to spinlock locking and synchronization.
The patches are not expected to alter functionality in a visible way.
Please see individual patch changelogs for details.
Thanks!
PATCH 1/5 cpufreq: intel_pstate: Drop redundant locking from intel_pstate_driver_cleanup()
From: Rafael J. Wysocki <[email protected]>
Remove the spinlock locking from intel_pstate_driver_cleanup() as it is not necessary because no other code accessing all_cpu_data[] can run in parallel with that function.
Had the locking been necessary, though, it would have been incorrect because the lock in question is acquired from a hardirq handler and it cannot be acquired from thread context without disabling interrupts.
--- drivers/cpufreq/intel_pstate.c | 2 -- 1 file changed, 2 deletions(-)
drivers/cpufreq/intel_pstate.c +0 -2
@@ -3135,10 +3135,8 @@ static void intel_pstate_driver_cleanup(if (intel_pstate_driver == &intel_pstate)intel_pstate_clear_update_util_hook (cpu); -spin_lock (&hwp_notify_lock);kfree (all_cpu_data[cpu]);WRITE_ONCE (all_cpu_data[cpu],NULL ); -spin_unlock (&hwp_notify_lock); } }cpus_read_unlock ();
PATCH 2/5 cpufreq: intel_pstate: Simplify spinlock locking
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Because intel_pstate_enable/disable_hwp_interrupt() are only called from thread context, they need not save the IRQ flags when using a spinlock as interrupts are guaranteed to be enabled when they run, so make them use spin_lock/unlock_irq().
--- drivers/cpufreq/intel_pstate.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
drivers/cpufreq/intel_pstate.c +4 -8
@@ -1682,30 +1682,26 @@ ack_intr:static void intel_pstate_disable_hwp_interrupt (struct cpudata *cpudata) { -unsigned long flags; -if (!boot_cpu_has (X86_FEATURE_HWP_NOTIFY ))return ;/* wrmsrl_on_cpu has to be outside spinlock as this can result in IPC */ wrmsrl_on_cpu (cpudata->cpu,MSR_HWP_INTERRUPT ,0x00 ); -spin_lock_irqsave (&hwp_notify_lock, flags); +spin_lock_irq (&hwp_notify_lock);if (cpumask_test_and_clear_cpu (cpudata->cpu, &hwp_intr_enable_mask))cancel_delayed_work (&cpudata->hwp_notify_work); -spin_unlock_irqrestore (&hwp_notify_lock, flags); +spin_unlock_irq (&hwp_notify_lock); }static void intel_pstate_enable_hwp_interrupt (struct cpudata *cpudata) {/* Enable HWP notification interrupt for guaranteed performance change */ if (boot_cpu_has (X86_FEATURE_HWP_NOTIFY )) { -unsigned long flags; - -spin_lock_irqsave (&hwp_notify_lock, flags); +spin_lock_irq (&hwp_notify_lock);INIT_DELAYED_WORK (&cpudata->hwp_notify_work, intel_pstate_notify_work);cpumask_set_cpu (cpudata->cpu, &hwp_intr_enable_mask); -spin_unlock_irqrestore (&hwp_notify_lock, flags); +spin_unlock_irq (&hwp_notify_lock);/* wrmsrl_on_cpu has to be outside spinlock as this can result in IPC */ wrmsrl_on_cpu (cpudata->cpu,MSR_HWP_INTERRUPT ,0x01 );
PATCH 3/5 cpufreq: intel_pstate: Wait for canceled delayed work to complete
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Make intel_pstate_disable_hwp_interrupt() wait for canceled delayed work to complete to avoid leftover work items running when it returns which may be during driver unregistration and may confuse things going forward.
--- drivers/cpufreq/intel_pstate.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/cpufreq/intel_pstate.c +6 -2
@@ -1682,6 +1682,8 @@ ack_intr:static void intel_pstate_disable_hwp_interrupt (struct cpudata *cpudata) { +bool cancel_work; +if (!boot_cpu_has (X86_FEATURE_HWP_NOTIFY ))return ;
@@ -1689,9 +1691,11 @@ static void intel_pstate_disable_hwp_intwrmsrl_on_cpu (cpudata->cpu,MSR_HWP_INTERRUPT ,0x00 );spin_lock_irq (&hwp_notify_lock); -if (cpumask_test_and_clear_cpu (cpudata->cpu, &hwp_intr_enable_mask)) -cancel_delayed_work (&cpudata->hwp_notify_work); + cancel_work =cpumask_test_and_clear_cpu (cpudata->cpu, &hwp_intr_enable_mask);spin_unlock_irq (&hwp_notify_lock); + +if (cancel_work) +cancel_delayed_work_sync (&cpudata->hwp_notify_work); }static void intel_pstate_enable_hwp_interrupt (struct cpudata *cpudata)
PATCH 4/5 cpufreq: intel_pstate: Get rid of unnecessary READ_ONCE() annotations
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Drop a redundant check involving READ_ONCE() from notify_hwp_interrupt() and make it check hwp_active without READ_ONCE() which is not necessary, because that variable is only set once during the early initialization of the driver.
In order to make that clear, annotate hwp_active with __ro_after_init.
--- drivers/cpufreq/intel_pstate.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/cpufreq/intel_pstate.c +3 -11
@@ -292,7 +292,7 @@ struct pstate_funcs {static struct pstate_funcs pstate_funcs__read_mostly ; -static int hwp_active__read_mostly ; +static bool hwp_active__ro_after_init ;static int hwp_mode_bdw__read_mostly ;static bool per_cpu_limits__read_mostly ;static bool hwp_boost__read_mostly ;
@@ -1640,7 +1640,7 @@ void notify_hwp_interrupt(void)unsigned long flags;u64 value; -if (!READ_ONCE (hwp_active) || !boot_cpu_has (X86_FEATURE_HWP_NOTIFY )) +if (!hwp_active || !boot_cpu_has (X86_FEATURE_HWP_NOTIFY ))return ;rdmsrl_safe (MSR_HWP_STATUS , &value);
@@ -1653,14 +1653,6 @@ void notify_hwp_interrupt(void) goto ack_intr;/* -* Currently we never free all_cpu_data. And we can't reach here -* without this allocated. But for safety for future changes, added -* check. -*/ -if (unlikely (!READ_ONCE (all_cpu_data))) - goto ack_intr; - -/* * The free is done during cleanup, when cpufreq registry is failed. * We wouldn't be here if it fails on init or switch status. But for * future changes, added check.
@@ -3464,7 +3456,7 @@ static int __init intel_pstate_init(void* deal with it. */ if ((!no_hwp &&boot_cpu_has (X86_FEATURE_HWP_EPP )) || hwp_forced) { -WRITE_ONCE (hwp_active,1 ); + hwp_active = true; hwp_mode_bdw = id->driver_data; intel_pstate.attr = hwp_cpufreq_attrs; intel_cpufreq.attr = hwp_cpufreq_attrs;
PATCH 4/5 cpufreq: intel_pstate: Get rid of unnecessary READ_ONCE() annotations
From: Rafael J. Wysocki <[email protected]>
Drop two redundant checks involving READ_ONCE() from notify_hwp_interrupt() and make it check hwp_active without READ_ONCE() which is not necessary, because that variable is only set once during the early initialization of the driver.
In order to make that clear, annotate hwp_active with __ro_after_init.
---
v1 -> v1.1:
* There are two redundant checks in notify_hwp_interrupt() that can be dropped, so drop them both.
--- drivers/cpufreq/intel_pstate.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)
drivers/cpufreq/intel_pstate.c +3 -20
@@ -292,7 +292,7 @@ struct pstate_funcs {static struct pstate_funcs pstate_funcs__read_mostly ; -static int hwp_active__read_mostly ; +static bool hwp_active__ro_after_init ;static int hwp_mode_bdw__read_mostly ;static bool per_cpu_limits__read_mostly ;static bool hwp_boost__read_mostly ;
@@ -1640,7 +1640,7 @@ void notify_hwp_interrupt(void)unsigned long flags;u64 value; -if (!READ_ONCE (hwp_active) || !boot_cpu_has (X86_FEATURE_HWP_NOTIFY )) +if (!hwp_active || !boot_cpu_has (X86_FEATURE_HWP_NOTIFY ))return ;rdmsrl_safe (MSR_HWP_STATUS , &value);
@@ -1652,23 +1652,6 @@ void notify_hwp_interrupt(void)if (!cpumask_test_cpu (this_cpu, &hwp_intr_enable_mask)) goto ack_intr; -/* -* Currently we never free all_cpu_data. And we can't reach here -* without this allocated. But for safety for future changes, added -* check. -*/ -if (unlikely (!READ_ONCE (all_cpu_data))) - goto ack_intr; - -/* -* The free is done during cleanup, when cpufreq registry is failed. -* We wouldn't be here if it fails on init or switch status. But for -* future changes, added check. -*/ - cpudata =READ_ONCE (all_cpu_data[this_cpu]); -if (unlikely (!cpudata)) - goto ack_intr; -schedule_delayed_work (&cpudata->hwp_notify_work,msecs_to_jiffies (10 ));spin_unlock_irqrestore (&hwp_notify_lock, flags);
@@ -3464,7 +3447,7 @@ static int __init intel_pstate_init(void* deal with it. */ if ((!no_hwp &&boot_cpu_has (X86_FEATURE_HWP_EPP )) || hwp_forced) { -WRITE_ONCE (hwp_active,1 ); + hwp_active = true; hwp_mode_bdw = id->driver_data; intel_pstate.attr = hwp_cpufreq_attrs; intel_cpufreq.attr = hwp_cpufreq_attrs;
PATCH 4/5 cpufreq: intel_pstate: Get rid of unnecessary READ_ONCE() annotations
From: Rafael J. Wysocki <[email protected]>
Drop two redundant checks involving READ_ONCE() from notify_hwp_interrupt() and make it check hwp_active without READ_ONCE() which is not necessary, because that variable is only set once during the early initialization of the driver.
In order to make that clear, annotate hwp_active with __ro_after_init.
---
v1.1 -> v1.2: Fix uninitialized memory access introduced in v1.1.
--- drivers/cpufreq/intel_pstate.c | 27 +++++---------------------- 1 file changed, 5 insertions(+), 22 deletions(-)
drivers/cpufreq/intel_pstate.c +5 -22
@@ -292,7 +292,7 @@ struct pstate_funcs {static struct pstate_funcs pstate_funcs__read_mostly ; -static int hwp_active__read_mostly ; +static bool hwp_active__ro_after_init ;static int hwp_mode_bdw__read_mostly ;static bool per_cpu_limits__read_mostly ;static bool hwp_boost__read_mostly ;
@@ -1636,11 +1636,10 @@ static cpumask_t hwp_intr_enable_mask;void notify_hwp_interrupt (void ) {unsigned int this_cpu =smp_processor_id (); -struct cpudata *cpudata;unsigned long flags;u64 value; -if (!READ_ONCE (hwp_active) || !boot_cpu_has (X86_FEATURE_HWP_NOTIFY )) +if (!hwp_active || !boot_cpu_has (X86_FEATURE_HWP_NOTIFY ))return ;rdmsrl_safe (MSR_HWP_STATUS , &value);
@@ -1652,24 +1651,8 @@ void notify_hwp_interrupt(void)if (!cpumask_test_cpu (this_cpu, &hwp_intr_enable_mask)) goto ack_intr; -/* -* Currently we never free all_cpu_data. And we can't reach here -* without this allocated. But for safety for future changes, added -* check. -*/ -if (unlikely (!READ_ONCE (all_cpu_data))) - goto ack_intr; - -/* -* The free is done during cleanup, when cpufreq registry is failed. -* We wouldn't be here if it fails on init or switch status. But for -* future changes, added check. -*/ - cpudata =READ_ONCE (all_cpu_data[this_cpu]); -if (unlikely (!cpudata)) - goto ack_intr; - -schedule_delayed_work (&cpudata->hwp_notify_work,msecs_to_jiffies (10 )); +schedule_delayed_work (&all_cpu_data[this_cpu]->hwp_notify_work, +msecs_to_jiffies (10 ));spin_unlock_irqrestore (&hwp_notify_lock, flags);
@@ -3464,7 +3447,7 @@ static int __init intel_pstate_init(void* deal with it. */ if ((!no_hwp &&boot_cpu_has (X86_FEATURE_HWP_EPP )) || hwp_forced) { -WRITE_ONCE (hwp_active,1 ); + hwp_active = true; hwp_mode_bdw = id->driver_data; intel_pstate.attr = hwp_cpufreq_attrs; intel_cpufreq.attr = hwp_cpufreq_attrs;
PATCH 5/5 cpufreq: intel_pstate: Use __ro_after_init for three variables
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
There are at least 3 variables in intel_pstate that do not get updated after they have been initialized, so annotate them with __ro_after_init.
--- drivers/cpufreq/intel_pstate.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/cpufreq/intel_pstate.c +3 -3
@@ -293,10 +293,10 @@ struct pstate_funcs {static struct pstate_funcs pstate_funcs__read_mostly ;static bool hwp_active__ro_after_init ; -static int hwp_mode_bdw__read_mostly ; -static bool per_cpu_limits__read_mostly ; +static int hwp_mode_bdw__ro_after_init ; +static bool per_cpu_limits__ro_after_init ; +static bool hwp_forced__ro_after_init ;static bool hwp_boost__read_mostly ; -static bool hwp_forced__read_mostly ;static struct cpufreq_driver *intel_pstate_driver__read_mostly ;