From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id F3B79A034F; Tue, 30 Mar 2021 16:25:16 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 88066140E18; Tue, 30 Mar 2021 16:25:16 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mails.dpdk.org (Postfix) with ESMTP id AD1DA40691 for ; Tue, 30 Mar 2021 16:25:14 +0200 (CEST) IronPort-SDR: HWuucyJVp6gMXYAjN2I2jBLdF77aWZk7iMPKnnAavPXpu/8r97GchmF2uX16tvWoQSzW78MmjK rnsIgTY49ktA== X-IronPort-AV: E=McAfee;i="6000,8403,9939"; a="253116339" X-IronPort-AV: E=Sophos;i="5.81,291,1610438400"; d="scan'208";a="253116339" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Mar 2021 07:25:13 -0700 IronPort-SDR: TUVoehh4DEPqZ8nMN+JtJvm9xlLcfhgK+RFlDvMpE/cSKT5swIwn0HFLV0cn4fD7R85uSsDghm AJ0eqVU8wuBg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.81,291,1610438400"; d="scan'208";a="595484259" Received: from silpixa00399498.ir.intel.com (HELO silpixa00399498.ger.corp.intel.com) ([10.237.222.97]) by orsmga005.jf.intel.com with ESMTP; 30 Mar 2021 07:25:12 -0700 From: Anatoly Burakov To: dev@dpdk.org Cc: David Hunt Date: Tue, 30 Mar 2021 14:25:09 +0000 Message-Id: X-Mailer: git-send-email 2.25.1 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [dpdk-dev] [PATCH v2 1/3] power: refactor base frequency detection X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Currently, base frequency detection code is a bit of an unmaintainable mess that has a couple of Coverity issues (dead code, and a resource leak). Rather than just fixing the issues and leaving the rest in place, refactor the code in a way that makes it more maintainable and less prone to such Coverity issues in the first place. Coverity issue: 369693 Coverity issue: 369694 Fixes: 4db9587bbf72 ("power: check sysfs base frequency") Cc: david.hunt@intel.com Signed-off-by: Anatoly Burakov --- lib/librte_power/meson.build | 7 + lib/librte_power/power_pstate_cpufreq.c | 178 ++++++++++++++---------- 2 files changed, 113 insertions(+), 72 deletions(-) diff --git a/lib/librte_power/meson.build b/lib/librte_power/meson.build index 9a2dcbfc7a..fd408ffd4c 100644 --- a/lib/librte_power/meson.build +++ b/lib/librte_power/meson.build @@ -5,6 +5,13 @@ if not is_linux build = false reason = 'only supported on Linux' endif + +# we do some snprintf magic so silence format-nonliteral +flag_nonliteral = '-Wno-format-nonliteral' +if cc.has_argument(flag_nonliteral) + cflags += flag_nonliteral +endif + sources = files('rte_power.c', 'power_acpi_cpufreq.c', 'power_kvm_vm.c', 'guest_channel.c', 'rte_power_empty_poll.c', diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c index 8a1fffaed5..add06720db 100644 --- a/lib/librte_power/power_pstate_cpufreq.c +++ b/lib/librte_power/power_pstate_cpufreq.c @@ -37,6 +37,13 @@ } \ } while (0) +#define FOPEN_OR_ERR_GOTO(f, label) do { \ + if ((f) == NULL) { \ + RTE_LOG(ERR, POWER, "File not opened\n"); \ + goto label; \ + } \ +} while (0) + #define FOPS_OR_NULL_GOTO(ret, label) do { \ if ((ret) == NULL) { \ RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \ @@ -148,93 +155,107 @@ out: close(fd); return ret; } +static int +open_core_sysfs_file(const char *template, unsigned int core, const char *mode, + FILE **f) +{ + char fullpath[PATH_MAX]; + FILE *tmpf; + + /* silenced -Wformat-nonliteral here */ + snprintf(fullpath, sizeof(fullpath), template, core); + tmpf = fopen(fullpath, mode); + if (tmpf == NULL) + return -1; + *f = tmpf; + + return 0; +} + +static int +read_core_sysfs_u32(FILE *f, uint32_t *val) +{ + char buf[BUFSIZ]; + uint32_t fval; + char *s; + + s = fgets(buf, sizeof(buf), f); + if (s == NULL) + return -1; + + /* fgets puts null terminator in, but do this just in case */ + buf[BUFSIZ - 1] = '\0'; + + /* strip off any terminating newlines */ + if (strlen(buf)) + strtok(buf, "\n"); + + fval = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL); + + /* write the value */ + *val = fval; + + return 0; +} + /** * It is to fopen the sys file for the future setting the lcore frequency. */ static int power_init_for_setting_freq(struct pstate_power_info *pi) { - FILE *f_min, *f_max, *f_base = NULL, *f_base_max; - char fullpath_min[PATH_MAX]; - char fullpath_max[PATH_MAX]; - char fullpath_base[PATH_MAX]; - char fullpath_base_max[PATH_MAX]; - char buf_base[BUFSIZ]; - char *s_base; - char *s_base_max; - uint32_t base_ratio = 0; - uint32_t base_max_ratio = 0; - uint64_t max_non_turbo = 0; - int ret_val = 0; - - snprintf(fullpath_base_max, - sizeof(fullpath_base_max), - POWER_SYSFILE_BASE_MAX_FREQ, - pi->lcore_id); - f_base_max = fopen(fullpath_base_max, "r"); - FOPEN_OR_ERR_RET(f_base_max, -1); - if (f_base_max != NULL) { - s_base_max = fgets(buf_base, sizeof(buf_base), f_base_max); - FOPS_OR_NULL_GOTO(s_base_max, out); - - buf_base[BUFSIZ-1] = '\0'; - if (strlen(buf_base)) - /* Strip off terminating '\n' */ - strtok(buf_base, "\n"); - - base_max_ratio = - strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL) - / BUS_FREQ; - } - - snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ, - pi->lcore_id); - f_min = fopen(fullpath_min, "rw+"); - FOPEN_OR_ERR_RET(f_min, -1); - - snprintf(fullpath_max, sizeof(fullpath_max), POWER_SYSFILE_MAX_FREQ, - 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; - pi->f_cur_max = f_max; - - snprintf(fullpath_base, sizeof(fullpath_base), POWER_SYSFILE_BASE_FREQ, - pi->lcore_id); - - f_base = fopen(fullpath_base, "r"); - FOPEN_OR_ERR_RET(f_base, -1); - if (f_base == NULL) { - /* No sysfs base_frequency, that's OK, continue without */ - base_ratio = 0; + FILE *f_base = NULL, *f_base_max = NULL, *f_min = NULL, *f_max = NULL; + uint32_t base_ratio, base_max_ratio; + uint64_t max_non_turbo; + int ret; + + /* open all files we expect to have open */ + open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r", + &f_base_max); + FOPEN_OR_ERR_GOTO(f_base_max, err); + + open_core_sysfs_file(POWER_SYSFILE_MIN_FREQ, pi->lcore_id, "rw+", + &f_min); + FOPEN_OR_ERR_GOTO(f_min, err); + + open_core_sysfs_file(POWER_SYSFILE_MAX_FREQ, pi->lcore_id, "rw+", + &f_max); + FOPEN_OR_ERR_GOTO(f_max, err); + + open_core_sysfs_file(POWER_SYSFILE_BASE_FREQ, pi->lcore_id, "r", + &f_base); + /* base ratio file may not exist in some kernels, so no error check */ + + /* read base max ratio */ + ret = read_core_sysfs_u32(f_base_max, &base_max_ratio); + FOPS_OR_ERR_GOTO(ret, err); + + /* base ratio may not exist */ + if (f_base != NULL) { + ret = read_core_sysfs_u32(f_base, &base_ratio); + FOPS_OR_ERR_GOTO(ret, err); } else { - s_base = fgets(buf_base, sizeof(buf_base), f_base); - FOPS_OR_NULL_GOTO(s_base, out); - - buf_base[BUFSIZ-1] = '\0'; - if (strlen(buf_base)) - /* Strip off terminating '\n' */ - strtok(buf_base, "\n"); - - base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL) - / BUS_FREQ; + base_ratio = 0; } /* Add MSR read to detect turbo status */ + if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0) + goto err; + /* no errors after this point */ - if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0) { - ret_val = -1; - goto out; - } + /* convert ratios to bins */ + base_max_ratio /= BUS_FREQ; + base_ratio /= BUS_FREQ; + + /* assign file handles */ + pi->f_cur_min = f_min; + pi->f_cur_max = f_max; max_non_turbo = (max_non_turbo&NON_TURBO_MASK)>>NON_TURBO_OFFSET; POWER_DEBUG_TRACE("no turbo perf %"PRIu64"\n", max_non_turbo); - pi->non_turbo_max_ratio = max_non_turbo; + pi->non_turbo_max_ratio = (uint32_t)max_non_turbo; /* * If base_frequency is reported as greater than the maximum @@ -260,7 +281,20 @@ power_init_for_setting_freq(struct pstate_power_info *pi) out: if (f_base != NULL) fclose(f_base); - return ret_val; + fclose(f_base_max); + /* f_min and f_max are stored, no need to close */ + return 0; + +err: + if (f_base != NULL) + fclose(f_base); + if (f_base_max != NULL) + fclose(f_base_max); + if (f_min != NULL) + fclose(f_min); + if (f_max != NULL) + fclose(f_max); + return -1; } static int -- 2.25.1