[dpdk-dev] [PATCH 1/2] power: fix power management env detection

[dpdk-dev] [PATCH 1/2] power: fix power management env detection

[Anatoly Burakov]

Anything coming from sysfs has a newline at the end. Cut it off before
comparing the strings.

Fixes: 20ab67608a39 ("power: add environment capability probing")

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_power/power_common.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_power/power_common.c b/lib/librte_power/power_common.c
index 59023d986b..fd527719cd 100644
--- a/lib/librte_power/power_common.c
+++ b/lib/librte_power/power_common.c
@@ -15,6 +15,7 @@ int
 cpufreq_check_scaling_driver(const char *driver_name)
 {
 	unsigned int lcore_id = 0; /* always check core 0 */
+	size_t len;
 	char fullpath[PATH_MAX];
 	char readbuf[PATH_MAX];
 	char *s;
@@ -39,6 +40,10 @@ cpufreq_check_scaling_driver(const char *driver_name)
 	if (s == NULL)
 		return 0;
 
+	/* when read from sysfs, driver name has an extra newline at the end */
+	len = strnlen(readbuf, sizeof(readbuf));
+	readbuf[len - 1] = '\0';
+
 	/* does the driver name match? */
 	if (strncmp(readbuf, driver_name, sizeof(readbuf)) != 0)
 		return 0;
-- 
2.17.1

[dpdk-dev] [PATCH 2/2] l3fwd-power: fix updating performance lcore parameters

[Anatoly Burakov]

When perf-config option is specified, we are calling into the power
library even though it may not necessarily be enabled. It is
questionable whether perf-config option is even applicable to non-power
library modes, but for now, fix it just by avoiding calling into the
power library if it wasn't initialized, and assume that every lcore is
high performance core.

Fixes: e0194feb322c ("examples/l3fwd-power: add interrupt-only mode")

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/perf_core.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/examples/l3fwd-power/perf_core.c b/examples/l3fwd-power/perf_core.c
index 0a294dfcc8..4705967ea2 100644
--- a/examples/l3fwd-power/perf_core.c
+++ b/examples/l3fwd-power/perf_core.c
@@ -28,12 +28,27 @@ struct perf_lcore_params {
 static struct perf_lcore_params prf_lc_prms[MAX_LCORE_PARAMS];
 static uint16_t nb_prf_lc_prms;
 
+static int
+is_hp_core(unsigned int lcore)
+{
+	struct rte_power_core_capabilities caps;
+	int ret;
+
+	/* do we have power management enabled? */
+	if (rte_power_get_env() == PM_ENV_NOT_SET) {
+		/* there's no power management, so just mark it as high perf */
+		return 1;
+	}
+	ret = rte_power_get_capabilities(lcore, &caps);
+	return ret == 0 && caps.turbo;
+}
+
 int
 update_lcore_params(void)
 {
 	uint8_t non_perf_lcores[RTE_MAX_LCORE];
 	uint16_t nb_non_perf_lcores = 0;
-	int i, j, ret;
+	int i, j;
 
 	/* if perf-config option was not used do nothing */
 	if (nb_prf_lc_prms == 0)
@@ -42,13 +57,9 @@ update_lcore_params(void)
 	/* if high-perf-cores option was not used query every available core */
 	if (nb_hp_lcores == 0) {
 		for (i = 0; i < RTE_MAX_LCORE; i++) {
-			if (rte_lcore_is_enabled(i)) {
-				struct rte_power_core_capabilities caps;
-				ret = rte_power_get_capabilities(i, &caps);
-				if (ret == 0 && caps.turbo) {
-					hp_lcores[nb_hp_lcores] = i;
-					nb_hp_lcores++;
-				}
+			if (rte_lcore_is_enabled(i) && is_hp_core(i)) {
+				hp_lcores[nb_hp_lcores] = i;
+				nb_hp_lcores++;
 			}
 		}
 	}
-- 
2.17.1

Re: [dpdk-dev] [PATCH 2/2] l3fwd-power: fix updating performance lcore parameters

[Hunt, David]

On 13/7/2020 1:15 PM, Anatoly Burakov wrote:
> When perf-config option is specified, we are calling into the power
> library even though it may not necessarily be enabled. It is
> questionable whether perf-config option is even applicable to non-power
> library modes, but for now, fix it just by avoiding calling into the
> power library if it wasn't initialized, and assume that every lcore is
> high performance core.
>
> Fixes: e0194feb322c ("examples/l3fwd-power: add interrupt-only mode")
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   examples/l3fwd-power/perf_core.c | 27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)

We saw a segfault with the following use-case: ./l3fwd-power -l 0,1 -- 
-p 0x1 -P --perf-config="(0,0,1,0)"

I've tested it and now runs without segfault.

Acked-by: David Hunt <david.hunt@intel.com>

Re: [dpdk-dev] [PATCH 1/2] power: fix power management env detection

[Hunt, David]

On 13/7/2020 1:15 PM, Anatoly Burakov wrote:
> Anything coming from sysfs has a newline at the end. Cut it off before
> comparing the strings.
>
> Fixes: 20ab67608a39 ("power: add environment capability probing")
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   lib/librte_power/power_common.c | 5 +++++
>   1 file changed, 5 insertions(+)


Acked-by: David Hunt <david.hunt@intel.com)

[dpdk-dev] [PATCH v2 1/2] power: fix power management env detection

[Anatoly Burakov]

Anything coming from sysfs has a newline at the end. Cut it off before
comparing the strings.

Fixes: 20ab67608a39 ("power: add environment capability probing")

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 lib/librte_power/power_common.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_power/power_common.c b/lib/librte_power/power_common.c
index 59023d986b..22b016ca9d 100644
--- a/lib/librte_power/power_common.c
+++ b/lib/librte_power/power_common.c
@@ -15,6 +15,7 @@ int
 cpufreq_check_scaling_driver(const char *driver_name)
 {
 	unsigned int lcore_id = 0; /* always check core 0 */
+	size_t end_idx;
 	char fullpath[PATH_MAX];
 	char readbuf[PATH_MAX];
 	char *s;
@@ -39,6 +40,13 @@ cpufreq_check_scaling_driver(const char *driver_name)
 	if (s == NULL)
 		return 0;
 
+	/* when read from sysfs, driver name has an extra newline at the end */
+	end_idx = strnlen(readbuf, sizeof(readbuf));
+	/* prevent underflow if len is zero */
+	if (end_idx > 0)
+		end_idx--;
+	readbuf[end_idx] = '\0';
+
 	/* does the driver name match? */
 	if (strncmp(readbuf, driver_name, sizeof(readbuf)) != 0)
 		return 0;
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2 1/2] power: fix power management env detection

[Bruce Richardson]

On Mon, Jul 13, 2020 at 03:54:27PM +0100, Anatoly Burakov wrote:
> Anything coming from sysfs has a newline at the end. Cut it off before
> comparing the strings.
> 
> Fixes: 20ab67608a39 ("power: add environment capability probing")
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: David Hunt <david.hunt@intel.com>
> ---
>  lib/librte_power/power_common.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/lib/librte_power/power_common.c b/lib/librte_power/power_common.c
> index 59023d986b..22b016ca9d 100644
> --- a/lib/librte_power/power_common.c
> +++ b/lib/librte_power/power_common.c
> @@ -15,6 +15,7 @@ int
>  cpufreq_check_scaling_driver(const char *driver_name)
>  {
>  	unsigned int lcore_id = 0; /* always check core 0 */
> +	size_t end_idx;
>  	char fullpath[PATH_MAX];
>  	char readbuf[PATH_MAX];
>  	char *s;
> @@ -39,6 +40,13 @@ cpufreq_check_scaling_driver(const char *driver_name)
>  	if (s == NULL)
>  		return 0;
>  
> +	/* when read from sysfs, driver name has an extra newline at the end */
> +	end_idx = strnlen(readbuf, sizeof(readbuf));
> +	/* prevent underflow if len is zero */
> +	if (end_idx > 0)
> +		end_idx--;
> +	readbuf[end_idx] = '\0';
> +
Would it not be safer to add " && readbuf[end_idx - 1] == '\n'" to the
condition, to check that it's terminated as expected? Theoretically if we
had a long string returned which was truncated, or only just fit, there
would not be a '\n' at the end.

/Bruce

Re: [dpdk-dev] [PATCH v2 1/2] power: fix power management env detection

[Burakov, Anatoly]

On 13-Jul-20 4:33 PM, Bruce Richardson wrote:
> On Mon, Jul 13, 2020 at 03:54:27PM +0100, Anatoly Burakov wrote:
>> Anything coming from sysfs has a newline at the end. Cut it off before
>> comparing the strings.
>>
>> Fixes: 20ab67608a39 ("power: add environment capability probing")
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> Acked-by: David Hunt <david.hunt@intel.com>
>> ---
>>   lib/librte_power/power_common.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/lib/librte_power/power_common.c b/lib/librte_power/power_common.c
>> index 59023d986b..22b016ca9d 100644
>> --- a/lib/librte_power/power_common.c
>> +++ b/lib/librte_power/power_common.c
>> @@ -15,6 +15,7 @@ int
>>   cpufreq_check_scaling_driver(const char *driver_name)
>>   {
>>   	unsigned int lcore_id = 0; /* always check core 0 */
>> +	size_t end_idx;
>>   	char fullpath[PATH_MAX];
>>   	char readbuf[PATH_MAX];
>>   	char *s;
>> @@ -39,6 +40,13 @@ cpufreq_check_scaling_driver(const char *driver_name)
>>   	if (s == NULL)
>>   		return 0;
>>   
>> +	/* when read from sysfs, driver name has an extra newline at the end */
>> +	end_idx = strnlen(readbuf, sizeof(readbuf));
>> +	/* prevent underflow if len is zero */
>> +	if (end_idx > 0)
>> +		end_idx--;
>> +	readbuf[end_idx] = '\0';
>> +
> Would it not be safer to add " && readbuf[end_idx - 1] == '\n'" to the
> condition, to check that it's terminated as expected? Theoretically if we
> had a long string returned which was truncated, or only just fit, there
> would not be a '\n' at the end.
> 
> /Bruce
> 

Yep, true, however unlikely :) I'll submit a v3.

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v3 1/2] power: fix power management env detection

[Anatoly Burakov]

Anything coming from sysfs has a newline at the end. Cut it off before
comparing the strings.

Fixes: 20ab67608a39 ("power: add environment capability probing")

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: David Hunt <david.hunt@intel.com>
---

Notes:
    v3:
    - Check if last character is really a newline before removing it
    
    v2:
    - Fix potential integer underflow

 lib/librte_power/power_common.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_power/power_common.c b/lib/librte_power/power_common.c
index 59023d986b..67e3318ec7 100644
--- a/lib/librte_power/power_common.c
+++ b/lib/librte_power/power_common.c
@@ -17,6 +17,7 @@ cpufreq_check_scaling_driver(const char *driver_name)
 	unsigned int lcore_id = 0; /* always check core 0 */
 	char fullpath[PATH_MAX];
 	char readbuf[PATH_MAX];
+	size_t end_idx;
 	char *s;
 	FILE *f;
 
@@ -39,6 +40,13 @@ cpufreq_check_scaling_driver(const char *driver_name)
 	if (s == NULL)
 		return 0;
 
+	/* when read from sysfs, driver name has an extra newline at the end */
+	end_idx = strnlen(readbuf, sizeof(readbuf));
+	if (end_idx > 0 && readbuf[end_idx - 1] == '\n') {
+		end_idx--;
+		readbuf[end_idx] = '\0';
+	}
+
 	/* does the driver name match? */
 	if (strncmp(readbuf, driver_name, sizeof(readbuf)) != 0)
 		return 0;
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 1/2] power: fix power management env detection

[Ma, LihongX]

Tested-by:ma,Lihong<lihongx.ma@intel.com>

Regards,
Ma,lihong

-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
Sent: Tuesday, July 14, 2020 6:30 PM
To: dev@dpdk.org
Cc: Hunt, David <david.hunt@intel.com>; Pattan, Reshma <reshma.pattan@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
Subject: [dpdk-dev] [PATCH v3 1/2] power: fix power management env detection

Anything coming from sysfs has a newline at the end. Cut it off before comparing the strings.

Fixes: 20ab67608a39 ("power: add environment capability probing")

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: David Hunt <david.hunt@intel.com>
---

Notes:
    v3:
    - Check if last character is really a newline before removing it
    
    v2:
    - Fix potential integer underflow

 lib/librte_power/power_common.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Re: [dpdk-dev] [PATCH v3 1/2] power: fix power management env detection

[Bruce Richardson]

On Tue, Jul 14, 2020 at 11:30:01AM +0100, Anatoly Burakov wrote:
> Anything coming from sysfs has a newline at the end. Cut it off before
> comparing the strings.
> 
> Fixes: 20ab67608a39 ("power: add environment capability probing")
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: David Hunt <david.hunt@intel.com>
> ---
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v3 1/2] power: fix power management env detection

[Thomas Monjalon]

17/07/2020 14:50, Bruce Richardson:
> On Tue, Jul 14, 2020 at 11:30:01AM +0100, Anatoly Burakov wrote:
> > Anything coming from sysfs has a newline at the end. Cut it off before
> > comparing the strings.
> > 
> > Fixes: 20ab67608a39 ("power: add environment capability probing")
> > 
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > Acked-by: David Hunt <david.hunt@intel.com>
> > ---
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks

[dpdk-dev] [PATCH v3 2/2] l3fwd-power: fix updating performance lcore parameters

[Anatoly Burakov]

When perf-config option is specified, we are calling into the power
library even though it may not necessarily be enabled. It is
questionable whether perf-config option is even applicable to non-power
library modes, but for now, fix it just by avoiding calling into the
power library if it wasn't initialized, and assume that every lcore is
high performance core.

Fixes: e0194feb322c ("examples/l3fwd-power: add interrupt-only mode")

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 examples/l3fwd-power/perf_core.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/examples/l3fwd-power/perf_core.c b/examples/l3fwd-power/perf_core.c
index 0a294dfcc8..4705967ea2 100644
--- a/examples/l3fwd-power/perf_core.c
+++ b/examples/l3fwd-power/perf_core.c
@@ -28,12 +28,27 @@ struct perf_lcore_params {
 static struct perf_lcore_params prf_lc_prms[MAX_LCORE_PARAMS];
 static uint16_t nb_prf_lc_prms;
 
+static int
+is_hp_core(unsigned int lcore)
+{
+	struct rte_power_core_capabilities caps;
+	int ret;
+
+	/* do we have power management enabled? */
+	if (rte_power_get_env() == PM_ENV_NOT_SET) {
+		/* there's no power management, so just mark it as high perf */
+		return 1;
+	}
+	ret = rte_power_get_capabilities(lcore, &caps);
+	return ret == 0 && caps.turbo;
+}
+
 int
 update_lcore_params(void)
 {
 	uint8_t non_perf_lcores[RTE_MAX_LCORE];
 	uint16_t nb_non_perf_lcores = 0;
-	int i, j, ret;
+	int i, j;
 
 	/* if perf-config option was not used do nothing */
 	if (nb_prf_lc_prms == 0)
@@ -42,13 +57,9 @@ update_lcore_params(void)
 	/* if high-perf-cores option was not used query every available core */
 	if (nb_hp_lcores == 0) {
 		for (i = 0; i < RTE_MAX_LCORE; i++) {
-			if (rte_lcore_is_enabled(i)) {
-				struct rte_power_core_capabilities caps;
-				ret = rte_power_get_capabilities(i, &caps);
-				if (ret == 0 && caps.turbo) {
-					hp_lcores[nb_hp_lcores] = i;
-					nb_hp_lcores++;
-				}
+			if (rte_lcore_is_enabled(i) && is_hp_core(i)) {
+				hp_lcores[nb_hp_lcores] = i;
+				nb_hp_lcores++;
 			}
 		}
 	}
-- 
2.17.1

[dpdk-dev] [PATCH v2 2/2] l3fwd-power: fix updating performance lcore parameters

[Anatoly Burakov]

When perf-config option is specified, we are calling into the power
library even though it may not necessarily be enabled. It is
questionable whether perf-config option is even applicable to non-power
library modes, but for now, fix it just by avoiding calling into the
power library if it wasn't initialized, and assume that every lcore is
high performance core.

Fixes: e0194feb322c ("examples/l3fwd-power: add interrupt-only mode")

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 examples/l3fwd-power/perf_core.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/examples/l3fwd-power/perf_core.c b/examples/l3fwd-power/perf_core.c
index 0a294dfcc8..4705967ea2 100644
--- a/examples/l3fwd-power/perf_core.c
+++ b/examples/l3fwd-power/perf_core.c
@@ -28,12 +28,27 @@ struct perf_lcore_params {
 static struct perf_lcore_params prf_lc_prms[MAX_LCORE_PARAMS];
 static uint16_t nb_prf_lc_prms;
 
+static int
+is_hp_core(unsigned int lcore)
+{
+	struct rte_power_core_capabilities caps;
+	int ret;
+
+	/* do we have power management enabled? */
+	if (rte_power_get_env() == PM_ENV_NOT_SET) {
+		/* there's no power management, so just mark it as high perf */
+		return 1;
+	}
+	ret = rte_power_get_capabilities(lcore, &caps);
+	return ret == 0 && caps.turbo;
+}
+
 int
 update_lcore_params(void)
 {
 	uint8_t non_perf_lcores[RTE_MAX_LCORE];
 	uint16_t nb_non_perf_lcores = 0;
-	int i, j, ret;
+	int i, j;
 
 	/* if perf-config option was not used do nothing */
 	if (nb_prf_lc_prms == 0)
@@ -42,13 +57,9 @@ update_lcore_params(void)
 	/* if high-perf-cores option was not used query every available core */
 	if (nb_hp_lcores == 0) {
 		for (i = 0; i < RTE_MAX_LCORE; i++) {
-			if (rte_lcore_is_enabled(i)) {
-				struct rte_power_core_capabilities caps;
-				ret = rte_power_get_capabilities(i, &caps);
-				if (ret == 0 && caps.turbo) {
-					hp_lcores[nb_hp_lcores] = i;
-					nb_hp_lcores++;
-				}
+			if (rte_lcore_is_enabled(i) && is_hp_core(i)) {
+				hp_lcores[nb_hp_lcores] = i;
+				nb_hp_lcores++;
 			}
 		}
 	}
-- 
2.17.1