[v2,1/4] power: fix non thread-safe power env modification

Message ID 20190318115647.14784-1-marcinx.hajkowski@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • [v2,1/4] power: fix non thread-safe power env modification
Related show

Checks

Context Check Description
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Hajkowski March 18, 2019, 11:56 a.m.
From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Due to lack of thread safety in exisiting solution
use spinlock mechanism for atomic
modification of power environment related data.

Fixes: 445c6528b5 ("power: common interface for guest and host")
Cc: stable@dpdk.org

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 doc/guides/rel_notes/release_19_05.rst |  2 ++
 lib/librte_power/rte_power.c           | 30 ++++++++++++++++++--------
 lib/librte_power/rte_power.h           |  2 +-
 3 files changed, 24 insertions(+), 10 deletions(-)

Comments

Stephen Hemminger March 18, 2019, 3:01 p.m. | #1
On Mon, 18 Mar 2019 12:56:44 +0100
Hajkowski <marcinx.hajkowski@intel.com> wrote:

> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Due to lack of thread safety in exisiting solution
> use spinlock mechanism for atomic
> modification of power environment related data.
> 
> Fixes: 445c6528b5 ("power: common interface for guest and host")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Thanks for making this library better.

When I did the dpdk-metrics investigation at the US DPDK summit, the power
library was one of the DPDK libraries that no upstream open source project
uses.

You might want to consider:
  * why is no project using it? what is wrong?
  * does it not fit any use case in any project should it continue?
  * if it is for a non-open source customer, then why is it part of DPDK?
Burakov, Anatoly March 19, 2019, 10:57 a.m. | #2
On 18-Mar-19 11:56 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Due to lack of thread safety in exisiting solution
> use spinlock mechanism for atomic
> modification of power environment related data.
> 
> Fixes: 445c6528b5 ("power: common interface for guest and host")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Thomas Monjalon March 29, 2019, 2:14 p.m. | #3
18/03/2019 12:56, Hajkowski:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> --- a/doc/guides/rel_notes/release_19_05.rst
> +++ b/doc/guides/rel_notes/release_19_05.rst
> @@ -120,6 +120,8 @@ API Changes
> +   * power: ``rte_power_set_env`` and ``rte_power_unset_env`` functions
> +     have been modified to be thread safe.

The deprecation notice was recently sent,
so I guess this patch is for DPDK 19.08.

Review from the maintainer (David) may help.
Thanks
Burakov, Anatoly March 29, 2019, 3:09 p.m. | #4
On 29-Mar-19 2:14 PM, Thomas Monjalon wrote:
> 18/03/2019 12:56, Hajkowski:
>> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
>> --- a/doc/guides/rel_notes/release_19_05.rst
>> +++ b/doc/guides/rel_notes/release_19_05.rst
>> @@ -120,6 +120,8 @@ API Changes
>> +   * power: ``rte_power_set_env`` and ``rte_power_unset_env`` functions
>> +     have been modified to be thread safe.
> 
> The deprecation notice was recently sent,
> so I guess this patch is for DPDK 19.08.

Yes, this is changing API so the target was 19.08. However, first patch 
is a fix and can be applied to 19.05 as well. The API documentation 
stated that the function was not thread safe, but the code itself was 
thread safe (it wasn't because it was buggy, but the intention of being 
thread safe was there), so this could be considered fixing docs to match 
the intended behavior of the code.

> 
> Review from the maintainer (David) may help.
> Thanks
> 
> 
>

Patch

diff --git a/doc/guides/rel_notes/release_19_05.rst b/doc/guides/rel_notes/release_19_05.rst
index 61a2c7383..65371e4c8 100644
--- a/doc/guides/rel_notes/release_19_05.rst
+++ b/doc/guides/rel_notes/release_19_05.rst
@@ -120,6 +120,8 @@  API Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+   * power: ``rte_power_set_env`` and ``rte_power_unset_env`` functions
+     have been modified to be thread safe.
 
 ABI Changes
 -----------
diff --git a/lib/librte_power/rte_power.c b/lib/librte_power/rte_power.c
index a05fbef94..540d69be7 100644
--- a/lib/librte_power/rte_power.c
+++ b/lib/librte_power/rte_power.c
@@ -2,7 +2,7 @@ 
  * Copyright(c) 2010-2014 Intel Corporation
  */
 
-#include <rte_atomic.h>
+#include <rte_spinlock.h>
 
 #include "rte_power.h"
 #include "power_acpi_cpufreq.h"
@@ -12,7 +12,7 @@ 
 
 enum power_management_env global_default_env = PM_ENV_NOT_SET;
 
-volatile uint32_t global_env_cfg_status = 0;
+static rte_spinlock_t global_env_cfg_lock = RTE_SPINLOCK_INITIALIZER;
 
 /* function pointers */
 rte_power_freqs_t rte_power_freqs  = NULL;
@@ -30,9 +30,15 @@  rte_power_get_capabilities_t rte_power_get_capabilities;
 int
 rte_power_set_env(enum power_management_env env)
 {
-	if (rte_atomic32_cmpset(&global_env_cfg_status, 0, 1) == 0) {
+	rte_spinlock_lock(&global_env_cfg_lock);
+
+	if (global_default_env != PM_ENV_NOT_SET) {
+		rte_spinlock_unlock(&global_env_cfg_lock);
 		return 0;
 	}
+
+	int ret = 0;
+
 	if (env == PM_ENV_ACPI_CPUFREQ) {
 		rte_power_freqs = power_acpi_cpufreq_freqs;
 		rte_power_get_freq = power_acpi_cpufreq_get_freq;
@@ -73,18 +79,24 @@  rte_power_set_env(enum power_management_env env)
 	} else {
 		RTE_LOG(ERR, POWER, "Invalid Power Management Environment(%d) set\n",
 				env);
-		rte_power_unset_env();
-		return -1;
+		ret = -1;
 	}
-	global_default_env = env;
-	return 0;
+
+	if (ret == 0)
+		global_default_env = env;
+	else
+		global_default_env = PM_ENV_NOT_SET;
+
+	rte_spinlock_unlock(&global_env_cfg_lock);
+	return ret;
 }
 
 void
 rte_power_unset_env(void)
 {
-	if (rte_atomic32_cmpset(&global_env_cfg_status, 1, 0) != 0)
-		global_default_env = PM_ENV_NOT_SET;
+	rte_spinlock_lock(&global_env_cfg_lock);
+	global_default_env = PM_ENV_NOT_SET;
+	rte_spinlock_unlock(&global_env_cfg_lock);
 }
 
 enum power_management_env
diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
index c5e8f6b5b..54b76b4ee 100644
--- a/lib/librte_power/rte_power.h
+++ b/lib/librte_power/rte_power.h
@@ -26,7 +26,7 @@  enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
 /**
  * Set the default power management implementation. If this is not called prior
  * to rte_power_init(), then auto-detect of the environment will take place.
- * It is not thread safe.
+ * It is thread safe.
  *
  * @param env
  *  env. The environment in which to initialise Power Management for.