From patchwork Tue Mar 30 14:25:09 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anatoly Burakov X-Patchwork-Id: 90113 X-Patchwork-Delegate: thomas@monjalon.net Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org 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 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 From patchwork Thu Apr 1 15:05:00 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anatoly Burakov X-Patchwork-Id: 90430 Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 84611A0548; Thu, 1 Apr 2021 17:05:16 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4EA5414116E; Thu, 1 Apr 2021 17:05:16 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mails.dpdk.org (Postfix) with ESMTP id 62E53140EE8 for ; Thu, 1 Apr 2021 17:05:14 +0200 (CEST) IronPort-SDR: e3jlc5boyrHoRK2dynM1xGTbCx/gdJwT6Go83lm+Vbw6FYgMU0VWXAKu0oLLnRFxkdd3yr+8Tf xdPU259ynt/w== X-IronPort-AV: E=McAfee;i="6000,8403,9941"; a="191741123" X-IronPort-AV: E=Sophos;i="5.81,296,1610438400"; d="scan'208";a="191741123" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Apr 2021 08:05:03 -0700 IronPort-SDR: REIdlvhVBCrT1EuIqM2eR/jFLOEeVkQ8jw9TkYmsp9FZqs5uySr8AHRr8X4RS5xJfXg1KGRMZA NJsNADL7tf8Q== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.81,296,1610438400"; d="scan'208";a="419235806" Received: from silpixa00399498.ir.intel.com (HELO silpixa00399498.ger.corp.intel.com) ([10.237.222.97]) by orsmga008.jf.intel.com with ESMTP; 01 Apr 2021 08:05:02 -0700 From: Anatoly Burakov To: dev@dpdk.org Cc: anatoly.burakov@intel.com, david.hunt@intel.com Date: Thu, 1 Apr 2021 15:05:00 +0000 Message-Id: <20210401150500.234015-2-anatoly.burakov@intel.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: References: MIME-Version: 1.0 Subject: [dpdk-dev] [PATCH v2 2/2] power: do not skip saving original pstate governor 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, when we set the pstate governor to "performance", we check if it is already set to this value, and if it is, we skip setting it. However, we never save this value anywhere, so that next time we come back and request the governor to be set to its original value, the original value is empty. Fix it by saving the original pstate governor first. While we're at it, replace `strlcpy` with `rte_strscpy`. Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility") Cc: david.hunt@intel.com Signed-off-by: Anatoly Burakov --- lib/librte_power/power_pstate_cpufreq.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c index af5ad0b506..3a5face4f0 100644 --- a/lib/librte_power/power_pstate_cpufreq.c +++ b/lib/librte_power/power_pstate_cpufreq.c @@ -382,6 +382,9 @@ power_set_governor_performance(struct pstate_power_info *pi) /* Strip off terminating '\n' */ strtok(buf, "\n"); + /* Save the original governor */ + rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori)); + /* Check if current governor is performance */ if (strncmp(buf, POWER_GOVERNOR_PERF, sizeof(POWER_GOVERNOR_PERF)) == 0) { @@ -390,8 +393,6 @@ power_set_governor_performance(struct pstate_power_info *pi) "already performance\n", pi->lcore_id); goto out; } - /* Save the original governor */ - strlcpy(pi->governor_ori, buf, sizeof(pi->governor_ori)); /* Write 'performance' to the governor */ val = fseek(f, 0, SEEK_SET);