[dpdk-dev] [PATCH] libs/power: fix the resource leaking issue

[dpdk-dev] [PATCH] libs/power: fix the resource leaking issue

[Liang Ma]

Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
Coverity issue: 328528

Also add the missing functionality of enable/disable turbo

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
---
 lib/librte_power/power_pstate_cpufreq.c | 34 ++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 411d0eb..cb226a5 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -160,6 +160,10 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 			pi->lcore_id);
 
 	f_max = fopen(fullpath_max, "rw+");
+
+	if (f_max == NULL)
+		fclose(f_min);
+
 	FOPEN_OR_ERR_RET(f_max, -1);
 
 	pi->f_cur_min = f_min;
@@ -214,7 +218,13 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
 	/* Turbo is available and enabled, first freq bucket is sys max freq */
 	if (pi->turbo_available && pi->turbo_enable && (idx == 0))
 		target_freq = pi->sys_max_freq;
-	else
+	else if (pi->turbo_available && (!pi->turbo_enable) && (idx == 0)) {
+
+		RTE_LOG(ERR, POWER, "Turbo is off, frequency can't be scaled up more %u\n",
+				pi->lcore_id);
+		return -1;
+
+	} else
 		target_freq = pi->freqs[idx];
 
 	/* Decrease freq, the min freq should be updated first */
@@ -394,6 +404,10 @@ power_get_available_freqs(struct pstate_power_info *pi)
 	FOPEN_OR_ERR_RET(f_min, ret);
 
 	f_max = fopen(fullpath_max, "r");
+
+	if (f_max == NULL)
+		fclose(f_min);
+
 	FOPEN_OR_ERR_RET(f_max, ret);
 
 	s_min = fgets(buf_min, sizeof(buf_min), f_min);
@@ -726,6 +740,14 @@ power_pstate_enable_turbo(unsigned int lcore_id)
 			return -1;
 	}
 
+	/* Max may have changed, so call to max function */
+	if (power_pstate_cpufreq_freq_max(lcore_id) < 0) {
+		RTE_LOG(ERR, POWER,
+			"Failed to set frequency of lcore %u to max\n",
+			lcore_id);
+			return -1;
+	}
+
 	return 0;
 }
 
@@ -744,6 +766,16 @@ power_pstate_disable_turbo(unsigned int lcore_id)
 
 	pi->turbo_enable = 0;
 
+	if ((pi->turbo_available) && (pi->curr_idx <= 1)) {
+		/* Try to set freq to max by default coming out of turbo */
+		if (power_pstate_cpufreq_freq_max(lcore_id) < 0) {
+			RTE_LOG(ERR, POWER,
+				"Failed to set frequency of lcore %u to max\n",
+				lcore_id);
+			return -1;
+		}
+	}
+
 
 	return 0;
 }
-- 
2.7.5

Re: [dpdk-dev] [PATCH] libs/power: fix the resource leaking issue

[Yao, Lei A]

> -----Original Message-----
> From: Ma, Liang J
> Sent: Friday, December 28, 2018 7:33 PM
> To: Hunt, David <david.hunt@intel.com>
> Cc: dev@dpdk.org; Burakov, Anatoly <anatoly.burakov@intel.com>; Yao, Lei
> A <lei.a.yao@intel.com>; Ma, Liang J <liang.j.ma@intel.com>
> Subject: [PATCH] libs/power: fix the resource leaking issue
> 
> Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
> Coverity issue: 328528
> 
> Also add the missing functionality of enable/disable turbo
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Reviewed-by: Lei Yao <lei.a.yao@intel.com>
Tested-by: Lei Yao <lei.a.yao@intel.com>
This patch has been tested based on 19.02-rc1 code. 
> ---
>  lib/librte_power/power_pstate_cpufreq.c | 34
> ++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_power/power_pstate_cpufreq.c
> b/lib/librte_power/power_pstate_cpufreq.c
> index 411d0eb..cb226a5 100644
> --- a/lib/librte_power/power_pstate_cpufreq.c
> +++ b/lib/librte_power/power_pstate_cpufreq.c
> @@ -160,6 +160,10 @@ power_init_for_setting_freq(struct
> pstate_power_info *pi)
>  			pi->lcore_id);
> 
>  	f_max = fopen(fullpath_max, "rw+");
> +
> +	if (f_max == NULL)
> +		fclose(f_min);
> +
>  	FOPEN_OR_ERR_RET(f_max, -1);
> 
>  	pi->f_cur_min = f_min;
> @@ -214,7 +218,13 @@ set_freq_internal(struct pstate_power_info *pi,
> uint32_t idx)
>  	/* Turbo is available and enabled, first freq bucket is sys max freq */
>  	if (pi->turbo_available && pi->turbo_enable && (idx == 0))
>  		target_freq = pi->sys_max_freq;
> -	else
> +	else if (pi->turbo_available && (!pi->turbo_enable) && (idx == 0)) {
> +
> +		RTE_LOG(ERR, POWER, "Turbo is off, frequency can't be
> scaled up more %u\n",
> +				pi->lcore_id);
> +		return -1;
> +
> +	} else
>  		target_freq = pi->freqs[idx];
> 
>  	/* Decrease freq, the min freq should be updated first */
> @@ -394,6 +404,10 @@ power_get_available_freqs(struct
> pstate_power_info *pi)
>  	FOPEN_OR_ERR_RET(f_min, ret);
> 
>  	f_max = fopen(fullpath_max, "r");
> +
> +	if (f_max == NULL)
> +		fclose(f_min);
> +
>  	FOPEN_OR_ERR_RET(f_max, ret);
> 
>  	s_min = fgets(buf_min, sizeof(buf_min), f_min);
> @@ -726,6 +740,14 @@ power_pstate_enable_turbo(unsigned int lcore_id)
>  			return -1;
>  	}
> 
> +	/* Max may have changed, so call to max function */
> +	if (power_pstate_cpufreq_freq_max(lcore_id) < 0) {
> +		RTE_LOG(ERR, POWER,
> +			"Failed to set frequency of lcore %u to max\n",
> +			lcore_id);
> +			return -1;
> +	}
> +
>  	return 0;
>  }
> 
> @@ -744,6 +766,16 @@ power_pstate_disable_turbo(unsigned int lcore_id)
> 
>  	pi->turbo_enable = 0;
> 
> +	if ((pi->turbo_available) && (pi->curr_idx <= 1)) {
> +		/* Try to set freq to max by default coming out of turbo */
> +		if (power_pstate_cpufreq_freq_max(lcore_id) < 0) {
> +			RTE_LOG(ERR, POWER,
> +				"Failed to set frequency of lcore %u to
> max\n",
> +				lcore_id);
> +			return -1;
> +		}
> +	}
> +
> 
>  	return 0;
>  }
> --
> 2.7.5

Re: [dpdk-dev] [PATCH] libs/power: fix the resource leaking issue

[Thomas Monjalon]

28/12/2018 12:32, Liang Ma:
> Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
> Coverity issue: 328528
> 
> Also add the missing functionality of enable/disable turbo

If you are changing two things, you probably need to split in two patches.
In 19.02 timeframe, we will get only the fix.

Dave, please could you help?

Re: [dpdk-dev] [PATCH] libs/power: fix the resource leaking issue

[Hunt, David]

On 14/1/2019 10:35 PM, Thomas Monjalon wrote:
> 28/12/2018 12:32, Liang Ma:
>> Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
>> Coverity issue: 328528
>>
>> Also add the missing functionality of enable/disable turbo
> If you are changing two things, you probably need to split in two patches.
> In 19.02 timeframe, we will get only the fix.
>
> Dave, please could you help?
>

Sure, Thomas. I've just pushed a v2 which focuses on the resource leak 
fix. We can handle the Turbo Boost enhancements in the next release.

Regards,
Dave.

Re: [dpdk-dev] [PATCH] libs/power: fix the resource leaking issue

[Thomas Monjalon]

15/01/2019 11:07, Hunt, David:
> 
> On 14/1/2019 10:35 PM, Thomas Monjalon wrote:
> > 28/12/2018 12:32, Liang Ma:
> >> Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
> >> Coverity issue: 328528
> >>
> >> Also add the missing functionality of enable/disable turbo
> > If you are changing two things, you probably need to split in two patches.
> > In 19.02 timeframe, we will get only the fix.
> >
> > Dave, please could you help?
> >
> 
> Sure, Thomas. I've just pushed a v2 which focuses on the resource leak 
> fix. We can handle the Turbo Boost enhancements in the next release.

OK thanks

[dpdk-dev] [PATCH] libs/power: fix the resource leaking issue

[David Hunt]

From: Liang Ma <liang.j.ma@intel.com>

Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
Coverity issue: 328528

v2: focus just on the resource leak. Remove additional code around
    Turbo Boost frequency handling.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Reviewed-by: Lei Yao <lei.a.yao@intel.com>
Tested-by: Lei Yao <lei.a.yao@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
---
 lib/librte_power/power_pstate_cpufreq.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index c4d972fc0..079822bf7 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -160,6 +160,10 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 			pi->lcore_id);
 
 	f_max = fopen(fullpath_max, "rw+");
+
+	if (f_max == NULL)
+		fclose(f_min);
+
 	FOPEN_OR_ERR_RET(f_max, -1);
 
 	pi->f_cur_min = f_min;
@@ -398,6 +402,10 @@ power_get_available_freqs(struct pstate_power_info *pi)
 	FOPEN_OR_ERR_RET(f_min, ret);
 
 	f_max = fopen(fullpath_max, "r");
+
+	if (f_max == NULL)
+		fclose(f_min);
+
 	FOPEN_OR_ERR_RET(f_max, ret);
 
 	s_min = fgets(buf_min, sizeof(buf_min), f_min);
-- 
2.17.1

Re: [dpdk-dev] [PATCH] libs/power: fix the resource leaking issue

[Thomas Monjalon]

15/01/2019 11:01, David Hunt:
> From: Liang Ma <liang.j.ma@intel.com>
> 
> Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
> Coverity issue: 328528
> 
> v2: focus just on the resource leak. Remove additional code around
>     Turbo Boost frequency handling.
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Reviewed-by: Lei Yao <lei.a.yao@intel.com>
> Tested-by: Lei Yao <lei.a.yao@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---
> --- a/lib/librte_power/power_pstate_cpufreq.c
> +++ b/lib/librte_power/power_pstate_cpufreq.c
>  	f_max = fopen(fullpath_max, "rw+");
> +
> +	if (f_max == NULL)
> +		fclose(f_min);

No need to insert a blank line before "if". Removing on apply.

Applied, thanks