[dpdk-stable] [PATCH v2 1/4] power: fix non thread-safe power env modification

[dpdk-stable] [PATCH v2 1/4] power: fix non thread-safe power env modification

[Hajkowski]

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

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.
-- 
2.17.2

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 1/4] power: fix non thread-safe power env modification

[Stephen Hemminger]

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?

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 1/4] power: fix non thread-safe power env modification

[Burakov, Anatoly]

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>

-- 
Thanks,
Anatoly

Re: [dpdk-stable] [PATCH v2 1/4] power: fix non thread-safe power env modification

[Thomas Monjalon]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 1/4] power: fix non thread-safe power env modification

[Burakov, Anatoly]

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


-- 
Thanks,
Anatoly

[dpdk-stable] [PATCH] power: fix non thread-safe power env modification

[Hajkowski]

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

diff --git a/doc/guides/rel_notes/release_19_05.rst b/doc/guides/rel_notes/release_19_05.rst
index c9d443e83..79f8ba76d 100644
--- a/doc/guides/rel_notes/release_19_05.rst
+++ b/doc/guides/rel_notes/release_19_05.rst
@@ -176,6 +176,8 @@ API Changes
   ``rte_vfio_container_dma_unmap`` have been extended with an option to
   request mapping or un-mapping to the default vfio container fd.
 
+* 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 dee7af345..47db69f33 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.
-- 
2.17.2

Re: [dpdk-stable] [dpdk-dev] [PATCH] power: fix non thread-safe power env modification

[Thomas Monjalon]

05/04/2019 16:35, Hajkowski:
> 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>

Applied, thanks