[dpdk-dev] [PATCH 0/3] rte_power enhancement

[dpdk-dev] [PATCH 0/3] rte_power enhancement

[Hajkowski]

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

Enhance rte_power API user notification by returning more
suitable value in case of power environment setting, add related tests
and some internal data cleaning corrections.

Please note that changes depends on patch
(http://patchwork.dpdk.org/patch/52349/) which should be applied
at first place.


Marcin Hajkowski (3):
  power: return error in set env when power env already set
  power: reset function pointers on unset env
  power: add UTs for all power env types

 app/test/test_power.c        | 156 +++++++++++++++++++++++++++--------
 lib/librte_power/rte_power.c |  24 +++++-
 lib/librte_power/rte_power.h |   3 +-
 3 files changed, 145 insertions(+), 38 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH 0/3] rte_power enhancement

[Hajkowski]

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

Enhance rte_power API user notification by returning more
suitable value in case of power environment setting, add related tests
and some internal data cleaning corrections.

Please note that changes depends on patch
(http://patchwork.dpdk.org/patch/52349/) which should be applied
at first place.


Marcin Hajkowski (3):
  power: return error in set env when power env already set
  power: reset function pointers on unset env
  power: add UTs for all power env types

 app/test/test_power.c        | 156 +++++++++++++++++++++++++++--------
 lib/librte_power/rte_power.c |  24 +++++-
 lib/librte_power/rte_power.h |   3 +-
 3 files changed, 145 insertions(+), 38 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH 1/3] 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>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_power/rte_power.c | 3 ++-
 lib/librte_power/rte_power.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

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 47db69f33..0650112ad 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 1/3] 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>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_power/rte_power.c | 3 ++-
 lib/librte_power/rte_power.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

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 47db69f33..0650112ad 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 2/3] 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>
Acked-by: Anatoly Burakov <anatoly.burakov@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 2/3] 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>
Acked-by: Anatoly Burakov <anatoly.burakov@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 3/3] 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>
Acked-by: Anatoly Burakov <anatoly.burakov@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..0ff2aa0d1 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 3/3] 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>
Acked-by: Anatoly Burakov <anatoly.burakov@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..0ff2aa0d1 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 0/3] rte_power enhancement

[Thomas Monjalon]

> Marcin Hajkowski (3):
>   power: return error in set env when power env already set
>   power: reset function pointers on unset env
>   power: add UTs for all power env types

Applied, thanks

Re: [dpdk-dev] [PATCH 0/3] rte_power enhancement

[Thomas Monjalon]

> Marcin Hajkowski (3):
>   power: return error in set env when power env already set
>   power: reset function pointers on unset env
>   power: add UTs for all power env types

Applied, thanks