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

[dpdk-dev] [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

[dpdk-dev] [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

[dpdk-dev] [PATCH v2 2/4] power: return error in set env when power env already set

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

On attempt to set_env in already initialized state notify
user by returning error that operation cannot be performed.

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

diff --git a/doc/guides/rel_notes/release_19_05.rst b/doc/guides/rel_notes/release_19_05.rst
index 65371e4c8..0da26b937 100644
--- a/doc/guides/rel_notes/release_19_05.rst
+++ b/doc/guides/rel_notes/release_19_05.rst
@@ -120,9 +120,13 @@ 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.
 
+   * power: Function ``rte_power_set_env`` modified to return -1 if environment
+     has been already set.
+
 ABI Changes
 -----------
 
diff --git a/lib/librte_power/rte_power.c b/lib/librte_power/rte_power.c
index 540d69be7..ed701087d 100644
--- a/lib/librte_power/rte_power.c
+++ b/lib/librte_power/rte_power.c
@@ -33,8 +33,9 @@ rte_power_set_env(enum power_management_env env)
 	rte_spinlock_lock(&global_env_cfg_lock);
 
 	if (global_default_env != PM_ENV_NOT_SET) {
+		RTE_LOG(ERR, POWER, "Power Management Environment already set.\n");
 		rte_spinlock_unlock(&global_env_cfg_lock);
-		return 0;
+		return -1;
 	}
 
 	int ret = 0;
diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
index 54b76b4ee..a3130e709 100644
--- a/lib/librte_power/rte_power.h
+++ b/lib/librte_power/rte_power.h
@@ -26,7 +26,8 @@ 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 thread safe.
+ * It is thread safe. New env can be set only in unitialized state
+ * (thus rte_power_unset_env must be called if different env was already set).
  *
  * @param env
  *  env. The environment in which to initialise Power Management for.
-- 
2.17.2

[dpdk-dev] [PATCH v2 2/4] power: return error in set env when power env already set

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

On attempt to set_env in already initialized state notify
user by returning error that operation cannot be performed.

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

diff --git a/doc/guides/rel_notes/release_19_05.rst b/doc/guides/rel_notes/release_19_05.rst
index 65371e4c8..0da26b937 100644
--- a/doc/guides/rel_notes/release_19_05.rst
+++ b/doc/guides/rel_notes/release_19_05.rst
@@ -120,9 +120,13 @@ 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.
 
+   * power: Function ``rte_power_set_env`` modified to return -1 if environment
+     has been already set.
+
 ABI Changes
 -----------
 
diff --git a/lib/librte_power/rte_power.c b/lib/librte_power/rte_power.c
index 540d69be7..ed701087d 100644
--- a/lib/librte_power/rte_power.c
+++ b/lib/librte_power/rte_power.c
@@ -33,8 +33,9 @@ rte_power_set_env(enum power_management_env env)
 	rte_spinlock_lock(&global_env_cfg_lock);
 
 	if (global_default_env != PM_ENV_NOT_SET) {
+		RTE_LOG(ERR, POWER, "Power Management Environment already set.\n");
 		rte_spinlock_unlock(&global_env_cfg_lock);
-		return 0;
+		return -1;
 	}
 
 	int ret = 0;
diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
index 54b76b4ee..a3130e709 100644
--- a/lib/librte_power/rte_power.h
+++ b/lib/librte_power/rte_power.h
@@ -26,7 +26,8 @@ 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 thread safe.
+ * It is thread safe. New env can be set only in unitialized state
+ * (thus rte_power_unset_env must be called if different env was already set).
  *
  * @param env
  *  env. The environment in which to initialise Power Management for.
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 2/4] power: return error in set env when power env already set

[Burakov, Anatoly]

On 18-Mar-19 11:56 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> On attempt to set_env in already initialized state notify
> user by returning error that operation cannot be performed.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2 2/4] power: return error in set env when power env already set

[Burakov, Anatoly]

On 18-Mar-19 11:56 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> On attempt to set_env in already initialized state notify
> user by returning error that operation cannot be performed.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2 3/4] power: reset function pointers on unset env

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Set all power environment related function pointers to NULL
when unset is being made.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 lib/librte_power/rte_power.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/lib/librte_power/rte_power.c b/lib/librte_power/rte_power.c
index ed701087d..6b7722727 100644
--- a/lib/librte_power/rte_power.c
+++ b/lib/librte_power/rte_power.c
@@ -27,6 +27,22 @@ rte_power_freq_change_t rte_power_freq_enable_turbo;
 rte_power_freq_change_t rte_power_freq_disable_turbo;
 rte_power_get_capabilities_t rte_power_get_capabilities;
 
+static void
+reset_power_function_ptrs(void)
+{
+	rte_power_freqs  = NULL;
+	rte_power_get_freq = NULL;
+	rte_power_set_freq = NULL;
+	rte_power_freq_up = NULL;
+	rte_power_freq_down = NULL;
+	rte_power_freq_max = NULL;
+	rte_power_freq_min = NULL;
+	rte_power_turbo_status = NULL;
+	rte_power_freq_enable_turbo = NULL;
+	rte_power_freq_disable_turbo = NULL;
+	rte_power_get_capabilities = NULL;
+}
+
 int
 rte_power_set_env(enum power_management_env env)
 {
@@ -85,8 +101,10 @@ rte_power_set_env(enum power_management_env env)
 
 	if (ret == 0)
 		global_default_env = env;
-	else
+	else {
 		global_default_env = PM_ENV_NOT_SET;
+		reset_power_function_ptrs();
+	}
 
 	rte_spinlock_unlock(&global_env_cfg_lock);
 	return ret;
@@ -97,6 +115,7 @@ rte_power_unset_env(void)
 {
 	rte_spinlock_lock(&global_env_cfg_lock);
 	global_default_env = PM_ENV_NOT_SET;
+	reset_power_function_ptrs();
 	rte_spinlock_unlock(&global_env_cfg_lock);
 }
 
-- 
2.17.2

[dpdk-dev] [PATCH v2 3/4] power: reset function pointers on unset env

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Set all power environment related function pointers to NULL
when unset is being made.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 lib/librte_power/rte_power.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/lib/librte_power/rte_power.c b/lib/librte_power/rte_power.c
index ed701087d..6b7722727 100644
--- a/lib/librte_power/rte_power.c
+++ b/lib/librte_power/rte_power.c
@@ -27,6 +27,22 @@ rte_power_freq_change_t rte_power_freq_enable_turbo;
 rte_power_freq_change_t rte_power_freq_disable_turbo;
 rte_power_get_capabilities_t rte_power_get_capabilities;
 
+static void
+reset_power_function_ptrs(void)
+{
+	rte_power_freqs  = NULL;
+	rte_power_get_freq = NULL;
+	rte_power_set_freq = NULL;
+	rte_power_freq_up = NULL;
+	rte_power_freq_down = NULL;
+	rte_power_freq_max = NULL;
+	rte_power_freq_min = NULL;
+	rte_power_turbo_status = NULL;
+	rte_power_freq_enable_turbo = NULL;
+	rte_power_freq_disable_turbo = NULL;
+	rte_power_get_capabilities = NULL;
+}
+
 int
 rte_power_set_env(enum power_management_env env)
 {
@@ -85,8 +101,10 @@ rte_power_set_env(enum power_management_env env)
 
 	if (ret == 0)
 		global_default_env = env;
-	else
+	else {
 		global_default_env = PM_ENV_NOT_SET;
+		reset_power_function_ptrs();
+	}
 
 	rte_spinlock_unlock(&global_env_cfg_lock);
 	return ret;
@@ -97,6 +115,7 @@ rte_power_unset_env(void)
 {
 	rte_spinlock_lock(&global_env_cfg_lock);
 	global_default_env = PM_ENV_NOT_SET;
+	reset_power_function_ptrs();
 	rte_spinlock_unlock(&global_env_cfg_lock);
 }
 
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 3/4] power: reset function pointers on unset env

[Burakov, Anatoly]

On 18-Mar-19 11:56 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Set all power environment related function pointers to NULL
> when unset is being made.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2 3/4] power: reset function pointers on unset env

[Burakov, Anatoly]

On 18-Mar-19 11:56 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Set all power environment related function pointers to NULL
> when unset is being made.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2 4/4] power: add UTs for all power env types

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Test all existing power environment configuration to verify if related
data is properly initialized and clean in set/unset scenarios.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 app/test/test_power.c | 156 ++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 35 deletions(-)

diff --git a/app/test/test_power.c b/app/test/test_power.c
index a0ee21983..c236c88cf 100644
--- a/app/test/test_power.c
+++ b/app/test/test_power.c
@@ -7,6 +7,7 @@
 #include <unistd.h>
 #include <limits.h>
 #include <string.h>
+#include <stdbool.h>
 
 #include "test.h"
 
@@ -23,6 +24,86 @@ test_power(void)
 
 #include <rte_power.h>
 
+static int
+check_function_ptrs(void)
+{
+	enum power_management_env env = rte_power_get_env();
+
+	const bool not_null_expected = !(env == PM_ENV_NOT_SET);
+
+	const char *inject_not_string1 = not_null_expected ? " not" : "";
+	const char *inject_not_string2 = not_null_expected ? "" : " not";
+
+	if ((rte_power_freqs == NULL) == not_null_expected) {
+		printf("rte_power_freqs should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+					inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_get_freq == NULL) == not_null_expected) {
+		printf("rte_power_get_freq should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+					inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_set_freq == NULL) == not_null_expected) {
+		printf("rte_power_set_freq should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_up == NULL) == not_null_expected) {
+		printf("rte_power_freq_up should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_down == NULL) == not_null_expected) {
+		printf("rte_power_freq_down should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_max == NULL) == not_null_expected) {
+		printf("rte_power_freq_max should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_min == NULL) == not_null_expected) {
+		printf("rte_power_freq_min should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_turbo_status == NULL) == not_null_expected) {
+		printf("rte_power_turbo_status should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_enable_turbo == NULL) == not_null_expected) {
+		printf("rte_power_freq_enable_turbo should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_disable_turbo == NULL) == not_null_expected) {
+		printf("rte_power_freq_disable_turbo should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_get_capabilities == NULL) == not_null_expected) {
+		printf("rte_power_get_capabilities should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int
 test_power(void)
 {
@@ -43,43 +124,48 @@ test_power(void)
 		return -1;
 	}
 
-	/* verify that function pointers are NULL */
-	if (rte_power_freqs != NULL) {
-		printf("rte_power_freqs should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_get_freq != NULL) {
-		printf("rte_power_get_freq should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_set_freq != NULL) {
-		printf("rte_power_set_freq should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_freq_up != NULL) {
-		printf("rte_power_freq_up should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_freq_down != NULL) {
-		printf("rte_power_freq_down should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_freq_max != NULL) {
-		printf("rte_power_freq_max should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_freq_min != NULL) {
-		printf("rte_power_freq_min should be NULL, environment has not been "
-				"initialised\n");
+	/* Verify that function pointers are NULL */
+	if (check_function_ptrs() < 0)
 		goto fail_all;
-	}
+
 	rte_power_unset_env();
+
+	/* Perform tests for valid environments.*/
+	const enum power_management_env envs[] = {PM_ENV_ACPI_CPUFREQ,
+						PM_ENV_KVM_VM,
+						PM_ENV_PSTATE_CPUFREQ};
+
+	const int envs_size = sizeof(envs)/sizeof(enum power_management_env);
+
+	int i;
+	for (i = 0; i < envs_size; ++i) {
+
+		/* Test setting a valid environment */
+		ret = rte_power_set_env(envs[i]);
+		if (ret != 0) {
+			printf("Unexpectedly unsucceeded on setting a valid environment\n");
+			return -1;
+		}
+
+		/* Test that the environment has been set */
+		env = rte_power_get_env();
+		if (env != envs[i]) {
+			printf("Not expected environment configuration\n");
+			return -1;
+		}
+
+		/* Verify that function pointers are NOT NULL */
+		if (check_function_ptrs() < 0)
+			goto fail_all;
+
+		rte_power_unset_env();
+
+		/* Verify that function pointers are NULL */
+		if (check_function_ptrs() < 0)
+			goto fail_all;
+
+	}
+
 	return 0;
 fail_all:
 	rte_power_unset_env();
-- 
2.17.2

[dpdk-dev] [PATCH v2 4/4] power: add UTs for all power env types

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Test all existing power environment configuration to verify if related
data is properly initialized and clean in set/unset scenarios.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 app/test/test_power.c | 156 ++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 35 deletions(-)

diff --git a/app/test/test_power.c b/app/test/test_power.c
index a0ee21983..c236c88cf 100644
--- a/app/test/test_power.c
+++ b/app/test/test_power.c
@@ -7,6 +7,7 @@
 #include <unistd.h>
 #include <limits.h>
 #include <string.h>
+#include <stdbool.h>
 
 #include "test.h"
 
@@ -23,6 +24,86 @@ test_power(void)
 
 #include <rte_power.h>
 
+static int
+check_function_ptrs(void)
+{
+	enum power_management_env env = rte_power_get_env();
+
+	const bool not_null_expected = !(env == PM_ENV_NOT_SET);
+
+	const char *inject_not_string1 = not_null_expected ? " not" : "";
+	const char *inject_not_string2 = not_null_expected ? "" : " not";
+
+	if ((rte_power_freqs == NULL) == not_null_expected) {
+		printf("rte_power_freqs should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+					inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_get_freq == NULL) == not_null_expected) {
+		printf("rte_power_get_freq should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+					inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_set_freq == NULL) == not_null_expected) {
+		printf("rte_power_set_freq should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_up == NULL) == not_null_expected) {
+		printf("rte_power_freq_up should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_down == NULL) == not_null_expected) {
+		printf("rte_power_freq_down should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_max == NULL) == not_null_expected) {
+		printf("rte_power_freq_max should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_min == NULL) == not_null_expected) {
+		printf("rte_power_freq_min should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_turbo_status == NULL) == not_null_expected) {
+		printf("rte_power_turbo_status should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_enable_turbo == NULL) == not_null_expected) {
+		printf("rte_power_freq_enable_turbo should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_freq_disable_turbo == NULL) == not_null_expected) {
+		printf("rte_power_freq_disable_turbo should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+	if ((rte_power_get_capabilities == NULL) == not_null_expected) {
+		printf("rte_power_get_capabilities should%s be NULL, environment has%s been "
+				"initialised\n", inject_not_string1,
+				inject_not_string2);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int
 test_power(void)
 {
@@ -43,43 +124,48 @@ test_power(void)
 		return -1;
 	}
 
-	/* verify that function pointers are NULL */
-	if (rte_power_freqs != NULL) {
-		printf("rte_power_freqs should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_get_freq != NULL) {
-		printf("rte_power_get_freq should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_set_freq != NULL) {
-		printf("rte_power_set_freq should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_freq_up != NULL) {
-		printf("rte_power_freq_up should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_freq_down != NULL) {
-		printf("rte_power_freq_down should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_freq_max != NULL) {
-		printf("rte_power_freq_max should be NULL, environment has not been "
-				"initialised\n");
-		goto fail_all;
-	}
-	if (rte_power_freq_min != NULL) {
-		printf("rte_power_freq_min should be NULL, environment has not been "
-				"initialised\n");
+	/* Verify that function pointers are NULL */
+	if (check_function_ptrs() < 0)
 		goto fail_all;
-	}
+
 	rte_power_unset_env();
+
+	/* Perform tests for valid environments.*/
+	const enum power_management_env envs[] = {PM_ENV_ACPI_CPUFREQ,
+						PM_ENV_KVM_VM,
+						PM_ENV_PSTATE_CPUFREQ};
+
+	const int envs_size = sizeof(envs)/sizeof(enum power_management_env);
+
+	int i;
+	for (i = 0; i < envs_size; ++i) {
+
+		/* Test setting a valid environment */
+		ret = rte_power_set_env(envs[i]);
+		if (ret != 0) {
+			printf("Unexpectedly unsucceeded on setting a valid environment\n");
+			return -1;
+		}
+
+		/* Test that the environment has been set */
+		env = rte_power_get_env();
+		if (env != envs[i]) {
+			printf("Not expected environment configuration\n");
+			return -1;
+		}
+
+		/* Verify that function pointers are NOT NULL */
+		if (check_function_ptrs() < 0)
+			goto fail_all;
+
+		rte_power_unset_env();
+
+		/* Verify that function pointers are NULL */
+		if (check_function_ptrs() < 0)
+			goto fail_all;
+
+	}
+
 	return 0;
 fail_all:
 	rte_power_unset_env();
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 4/4] power: add UTs for all power env types

[Burakov, Anatoly]

On 18-Mar-19 11:56 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Test all existing power environment configuration to verify if related
> data is properly initialized and clean in set/unset scenarios.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---

<snip>

> +
>   	rte_power_unset_env();
> +
> +	/* Perform tests for valid environments.*/
> +	const enum power_management_env envs[] = {PM_ENV_ACPI_CPUFREQ,
> +						PM_ENV_KVM_VM,
> +						PM_ENV_PSTATE_CPUFREQ};

Extreme nitpicking, but i believe we use two indentation levels for 
continuation.

Otherwise

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2 4/4] power: add UTs for all power env types

[Burakov, Anatoly]

On 18-Mar-19 11:56 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Test all existing power environment configuration to verify if related
> data is properly initialized and clean in set/unset scenarios.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---

<snip>

> +
>   	rte_power_unset_env();
> +
> +	/* Perform tests for valid environments.*/
> +	const enum power_management_env envs[] = {PM_ENV_ACPI_CPUFREQ,
> +						PM_ENV_KVM_VM,
> +						PM_ENV_PSTATE_CPUFREQ};

Extreme nitpicking, but i believe we use two indentation levels for 
continuation.

Otherwise

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [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-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-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-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-dev] [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-dev] [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-dev] [dpdk-stable] [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

Re: [dpdk-dev] [dpdk-stable] [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

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

[Thomas Monjalon]

29/03/2019 16:09, 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

What is the follow-up here?
We still have an old deprecation notice:
	http://git.dpdk.org/dpdk/commit/?id=3477b7a2cc

I wonder how such things can be forgotten.
I feel some help is needed in prioritization,
so let's consider this deprecation as the priority #1
gating any other change in the power library.

Priority #2: cleaning up API which are secretly exported
for example convenience. It is an old design issue never fixed:
	http://inbox.dpdk.org/dev/6046120.mQ0ExDuKPD@thomas/

Priority #3: request feedbacks from other maintainers
to add a generic API in ethdev to get a hook for power management.

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

[David Hunt]

On 25/10/2020 6:22 PM, Thomas Monjalon wrote:
> 29/03/2019 16:09, 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
> What is the follow-up here?
> We still have an old deprecation notice:
> 	http://git.dpdk.org/dpdk/commit/?id=3477b7a2cc
>
> I wonder how such things can be forgotten.
> I feel some help is needed in prioritization,
> so let's consider this deprecation as the priority #1
> gating any other change in the power library.


Hi Thomas,

#1 is now done, I've pushed a patch removing the deprication notice to 
the mailing list, as the change it describes had previously been applied.
Patch here: http://patches.dpdk.org/patch/82327/


> Priority #2: cleaning up API which are secretly exported
> for example convenience. It is an old design issue never fixed:
> 	http://inbox.dpdk.org/dev/6046120.mQ0ExDuKPD@thomas/


Regarding the virtio channel API, Bruce and I had a look at this, and I 
think I need to do some more research into it. I'd prefer not to make 
that API public, as it was intended to be mainly for the 
vm_power_manager app and the companion guest_cli.

So I'll look into this, and look at the best way to proceed with 
cleaning this up so that these apps can be build using meson/ninja as 
part of DPDK, as well as using 'make' extrernal to DPDK.

I hope to push up an RFC next week so we can get agreement on the best 
path forward on this item.


>
> Priority #3: request feedbacks from other maintainers
> to add a generic API in ethdev to get a hook for power management.
>

Would it be possible to look at #2 and #3 in parallel? I'm not sure I'd 
have #2 done fully in time for this release, and, if not, I will make 
sure it's done for 21.02.


Rgds,
Dave.

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

[Thomas Monjalon]

28/10/2020 14:53, David Hunt:
> On 25/10/2020 6:22 PM, Thomas Monjalon wrote:
> > 29/03/2019 16:09, 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
> > What is the follow-up here?
> > We still have an old deprecation notice:
> > 	http://git.dpdk.org/dpdk/commit/?id=3477b7a2cc
> >
> > I wonder how such things can be forgotten.
> > I feel some help is needed in prioritization,
> > so let's consider this deprecation as the priority #1
> > gating any other change in the power library.
> 
> 
> Hi Thomas,
> 
> #1 is now done, I've pushed a patch removing the deprication notice to 
> the mailing list, as the change it describes had previously been applied.
> Patch here: http://patches.dpdk.org/patch/82327/
> 
> 
> > Priority #2: cleaning up API which are secretly exported
> > for example convenience. It is an old design issue never fixed:
> > 	http://inbox.dpdk.org/dev/6046120.mQ0ExDuKPD@thomas/
> 
> 
> Regarding the virtio channel API, Bruce and I had a look at this, and I 
> think I need to do some more research into it. I'd prefer not to make 
> that API public, as it was intended to be mainly for the 
> vm_power_manager app and the companion guest_cli.
> 
> So I'll look into this, and look at the best way to proceed with 
> cleaning this up so that these apps can be build using meson/ninja as 
> part of DPDK, as well as using 'make' extrernal to DPDK.
> 
> I hope to push up an RFC next week so we can get agreement on the best 
> path forward on this item.
> 
> 
> >
> > Priority #3: request feedbacks from other maintainers
> > to add a generic API in ethdev to get a hook for power management.
> >
> 
> Would it be possible to look at #2 and #3 in parallel? I'm not sure I'd 
> have #2 done fully in time for this release, and, if not, I will make 
> sure it's done for 21.02.

Yes I'm looking at #3 in parallel but the ethdev API is really
not clear enough.

[dpdk-dev] [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

[dpdk-dev] [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-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

Re: [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