diff mbox series

libs/power: fix the resource leaking issue

Message ID 1545996779-4098-1-git-send-email-liang.j.ma@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series libs/power: fix the resource leaking issue |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Liang, Ma Dec. 28, 2018, 11:32 a.m. UTC 
  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(-)

Comments

Yao, Lei A Jan. 2, 2019, 2 a.m. UTC | #1 
> -----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
Thomas Monjalon Jan. 14, 2019, 10:35 p.m. UTC | #2 
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?
Hunt, David Jan. 15, 2019, 10:07 a.m. UTC | #3 
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.
Thomas Monjalon Jan. 15, 2019, 10:08 a.m. UTC | #4 
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
diff mbox series

Patch

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