From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f65.google.com (mail-lf0-f65.google.com [209.85.215.65]) by dpdk.org (Postfix) with ESMTP id 2F3CE1B21D for ; Sat, 11 Nov 2017 19:56:05 +0100 (CET) Received: by mail-lf0-f65.google.com with SMTP id b190so14360115lfg.9 for ; Sat, 11 Nov 2017 10:56:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=tEYyt+uhESeeqgHaaGbE0FMroNwwgkQqyt5ZOMN0KP4=; b=Q1NqAf8apyoyTo3h+EtQArfe/ll8i0L/79D5rMFZupZWd258wpswzPxhzhMvT+mUPA jdbzQWM5uK8lvRekRvWq3YBt5nGW6G5QK3TWTVdIFzlDgSgJo59fLJo8UeOUqcr84k/D uC9J3SK2MRZwduTpMxFgI02TN5/C3t0d+fjGU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=tEYyt+uhESeeqgHaaGbE0FMroNwwgkQqyt5ZOMN0KP4=; b=i432zjfltsArN88Uw7+1oUqoqxzncXO6IQjUoJJLYaaIZG+xv0fpaGXJOrYc+AnpaD 2jEChFRCyhEOonRp2JRS/wxmNE0YtXlLqagO4czFfrkoMCa5f8qwpk9v6HriqjhczaN6 sLZISwkiiQk4ZTOVHChEHx1cHxPvWcxaM/nr+4W0FxsadfJg27Cmz6L1CTRrCYJzFW/l fcnCXzGv6HBUquLleDedrJj/s14mrKE54J84lQCNWpfvsITpQ25YbVMNs8WXUTgWTMOy dafwXHwv1L/XK6RVeBu2pfXUfwEUtOyrCGJDCpHWSft87Yqkledqom7SwlPcSuaAZBle 1fqQ== X-Gm-Message-State: AJaThX48AkWhCFTip48hK/PLxxN44wKtf4zKqbBZLYtJSduFYW/R3+LX 4sg2qldfv98j6DWZRFS5bvbsimPa748= X-Google-Smtp-Source: AGs4zMbsFHeoJCRgqH2vbDNq/BM8+Y+6DgRX5p0kPqj538ziTk5OltUSELepCR15ijU6ZDCAyF0ZIg== X-Received: by 10.46.22.20 with SMTP id w20mr1510811ljd.69.1510426564559; Sat, 11 Nov 2017 10:56:04 -0800 (PST) Received: from rad-H81M-S1.semihalf.local (31-172-191-173.noc.fibertech.net.pl. [31.172.191.173]) by smtp.gmail.com with ESMTPSA id x90sm2342107ljb.86.2017.11.11.10.56.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sat, 11 Nov 2017 10:56:03 -0800 (PST) From: Radoslaw Biernacki To: dev@dpdk.org, david.hunt@intel.com Cc: stable@dpdk.org, alan.carew@intel.com, pablo.de.lara.guarch@intel.com Date: Sat, 11 Nov 2017 19:55:06 +0100 Message-Id: <1510426507-28245-3-git-send-email-radoslaw.biernacki@linaro.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1510426507-28245-1-git-send-email-radoslaw.biernacki@linaro.org> References: <1508161628-4265-1-git-send-email-radoslaw.biernacki@linaro.org> <1510426507-28245-1-git-send-email-radoslaw.biernacki@linaro.org> Subject: [dpdk-dev] [PATCH 2/3] power: switching to unbuffered access for /sys files X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 11 Nov 2017 18:56:05 -0000 This patch fixes the bug caused by improper use of buffered stdio file access for switching the CPU frequency and governor. When using buffered stdio, each fwrite() must use fflush() and the return code must be verified. Also fseek() is needed. Therefore it is better to use unbuffered mode or use plain open()/write() functions. This fix use second approach. This not only remove need for ffush() but also remove need for fseek() operations. This patch also reuse some code around power_set_governor_userspace() and power_set_governor_userspace() functions. Fixes: 445c6528b55f ("power: common interface for guest and host") CC: stable@dpdk.org Signed-off-by: Radoslaw Biernacki --- lib/librte_power/rte_power_acpi_cpufreq.c | 211 +++++++++++++----------------- 1 file changed, 91 insertions(+), 120 deletions(-) diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c index 3d0872f..f811bd3 100644 --- a/lib/librte_power/rte_power_acpi_cpufreq.c +++ b/lib/librte_power/rte_power_acpi_cpufreq.c @@ -30,7 +30,7 @@ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ - +#include #include #include #include @@ -47,6 +47,12 @@ #include "rte_power_acpi_cpufreq.h" #include "rte_power_common.h" +#define min(_x, _y) ({ \ + typeof(_x) _min1 = (_x); \ + typeof(_y) _min2 = (_y); \ + (void) (&_min1 == &_min2); \ + _min1 < _min2 ? _min1 : _min2; }) + #ifdef RTE_LIBRTE_POWER_DEBUG #define POWER_DEBUG_TRACE(fmt, args...) do { \ RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \ @@ -88,7 +94,7 @@ struct rte_power_info { unsigned lcore_id; /**< Logical core id */ uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */ uint32_t nb_freqs; /**< number of available freqs */ - FILE *f; /**< FD of scaling_setspeed */ + int fd; /**< FD of scaling_setspeed */ char governor_ori[32]; /**< Original governor name */ uint32_t curr_idx; /**< Freq index in freqs array */ volatile uint32_t state; /**< Power in use state */ @@ -105,6 +111,9 @@ static struct rte_power_info lcore_power_info[RTE_MAX_LCORE]; static int set_freq_internal(struct rte_power_info *pi, uint32_t idx) { + char buf[BUFSIZ]; + int count, ret; + if (idx >= RTE_MAX_LCORE_FREQS || idx >= pi->nb_freqs) { RTE_LOG(ERR, POWER, "Invalid frequency index %u, which " "should be less than %u\n", idx, pi->nb_freqs); @@ -117,17 +126,14 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx) POWER_DEBUG_TRACE("Freqency[%u] %u to be set for lcore %u\n", idx, pi->freqs[idx], pi->lcore_id); - if (fseek(pi->f, 0, SEEK_SET) < 0) { - RTE_LOG(ERR, POWER, "Fail to set file position indicator to 0 " - "for setting frequency for lcore %u\n", pi->lcore_id); + count = snprintf(buf, sizeof(buf), "%u", pi->freqs[idx]); + assert((size_t)count < sizeof(buf)-1); + ret = write(pi->fd, buf, count); + if (ret != count) { + RTE_LOG(ERR, POWER, "Fail to write new frequency (%s) for " + "lcore %u\n", buf, pi->lcore_id); return -1; } - if (fprintf(pi->f, "%u", pi->freqs[idx]) < 0) { - RTE_LOG(ERR, POWER, "Fail to write new frequency for " - "lcore %u\n", pi->lcore_id); - return -1; - } - fflush(pi->f); pi->curr_idx = idx; return 1; @@ -139,90 +145,109 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx) * governor will be saved for rolling back. */ static int -power_set_governor_userspace(struct rte_power_info *pi) +power_set_governor(unsigned int lcore_id, const char *new_gov, char *old_gov, + size_t old_gov_len) { - FILE *f; + int fd; + int count, len; int ret = -1; char buf[BUFSIZ]; char fullpath[PATH_MAX]; - char *s; - int val; snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR, - pi->lcore_id); - f = fopen(fullpath, "rw+"); - if (!f) { + lcore_id); + fd = open(fullpath, O_RDWR); + if (fd < 0) { RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath); return ret; } - s = fgets(buf, sizeof(buf), f); - if (!s) { - RTE_LOG(ERR, POWER, "fgets returns nothing\n"); + count = read(fd, buf, sizeof(buf)); + if (count < 0) { + RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath); goto out; } + buf[min((size_t)count, sizeof(buf)-1)] = '\0'; - /* Check if current governor is userspace */ - if (strncmp(buf, POWER_GOVERNOR_USERSPACE, - sizeof(POWER_GOVERNOR_USERSPACE)) == 0) { + /* Check if current governor is as requested */ + if (!strcmp(buf, new_gov)) { ret = 0; POWER_DEBUG_TRACE("Power management governor of lcore %u is " - "already userspace\n", pi->lcore_id); - goto out; - } - /* Save the original governor */ - snprintf(pi->governor_ori, sizeof(pi->governor_ori), "%s", buf); - - /* Write 'userspace' to the governor */ - val = fseek(f, 0, SEEK_SET); - if (val < 0) { - RTE_LOG(ERR, POWER, "fseek failed\n"); + "already %s\n", lcore_id, new_gov); goto out; } + /* Save the old governor */ + if (old_gov) + snprintf(old_gov, old_gov_len, "%s", buf); - val = fputs(POWER_GOVERNOR_USERSPACE, f); - if (val < 0) { - RTE_LOG(ERR, POWER, "fputs failed\n"); + /* Set new governor */ + len = strlen(new_gov); + count = write(fd, new_gov, len); + if (count != len) { + RTE_LOG(ERR, POWER, "Failed to set %s governor\n", new_gov); goto out; } ret = 0; RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been " - "set to user space successfully\n", pi->lcore_id); + "set to user space successfully\n", lcore_id); out: - fclose(f); + if (close(fd)) + RTE_LOG(ERR, POWER, "Error while closing file %s\n", fullpath); return ret; } /** + * It is to check the current scaling governor by reading sys file, and then + * set it into 'userspace' if it is not by writing the sys file. The original + * governor will be saved for rolling back. + */ +static int +power_set_governor_userspace(struct rte_power_info *pi) +{ + return power_set_governor(pi->lcore_id, POWER_GOVERNOR_USERSPACE, + pi->governor_ori, sizeof(pi->governor_ori)); +} + +/** + * It is to check the governor and then set the original governor back if + * needed by writing the the sys file. + */ +static int +power_set_governor_original(struct rte_power_info *pi) +{ + return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0); +} + +/** * It is to get the available frequencies of the specific lcore by reading the * sys file. */ static int power_get_available_freqs(struct rte_power_info *pi) { - FILE *f; + int fd; int ret = -1, i, count; char *p; char buf[BUFSIZ]; char fullpath[PATH_MAX]; char *freqs[RTE_MAX_LCORE_FREQS]; - char *s; snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_FREQ, - pi->lcore_id); - f = fopen(fullpath, "r"); - if (!f) { + pi->lcore_id); + fd = open(fullpath, O_RDONLY); + if (fd < 0) { RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath); return ret; } - s = fgets(buf, sizeof(buf), f); - if (!s) { - RTE_LOG(ERR, POWER, "fgets returns nothing\n"); + count = read(fd, buf, sizeof(buf)); + if (count < 0) { + RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath); goto out; } + buf[min((size_t)count, sizeof(buf)-1)] = '\0'; /* Strip the line break if there is */ p = strchr(buf, '\n'); @@ -267,7 +292,8 @@ power_get_available_freqs(struct rte_power_info *pi) POWER_DEBUG_TRACE("%d frequencie(s) of lcore %u are available\n", count, pi->lcore_id); out: - fclose(f); + if (close(fd)) + RTE_LOG(ERR, POWER, "Error while closing file %s\n", fullpath); return ret; } @@ -278,37 +304,39 @@ power_get_available_freqs(struct rte_power_info *pi) static int power_init_for_setting_freq(struct rte_power_info *pi) { - FILE *f; + int fd; + int count; + uint32_t i, freq; char fullpath[PATH_MAX]; char buf[BUFSIZ]; - uint32_t i, freq; - char *s; snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED, pi->lcore_id); - f = fopen(fullpath, "rw+"); - if (!f) { + fd = open(fullpath, O_RDWR); + if (fd < 0) { RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath); return -1; } - s = fgets(buf, sizeof(buf), f); - if (!s) { - RTE_LOG(ERR, POWER, "fgets returns nothing\n"); + count = read(fd, buf, sizeof(buf)); + if (count < 0) { + RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath); goto out; } + buf[min((size_t)count, sizeof(buf)-1)] = '\0'; freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL); for (i = 0; i < pi->nb_freqs; i++) { if (freq == pi->freqs[i]) { pi->curr_idx = i; - pi->f = f; + pi->fd = fd; return 0; } } out: - fclose(f); + if (close(fd)) + RTE_LOG(ERR, POWER, "Error while closing file %s\n", fullpath); return -1; } @@ -373,66 +401,6 @@ rte_power_acpi_cpufreq_init(unsigned lcore_id) return -1; } -/** - * It is to check the governor and then set the original governor back if - * needed by writing the the sys file. - */ -static int -power_set_governor_original(struct rte_power_info *pi) -{ - FILE *f; - int ret = -1; - char buf[BUFSIZ]; - char fullpath[PATH_MAX]; - char *s; - int val; - - snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR, - pi->lcore_id); - f = fopen(fullpath, "rw+"); - if (!f) { - RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath); - return ret; - } - - s = fgets(buf, sizeof(buf), f); - if (!s) { - RTE_LOG(ERR, POWER, "fgets returns nothing\n"); - goto out; - } - - /* Check if the governor to be set is the same as current */ - if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) { - ret = 0; - POWER_DEBUG_TRACE("Power management governor of lcore %u " - "has already been set to %s\n", - pi->lcore_id, pi->governor_ori); - goto out; - } - - /* Write back the original governor */ - val = fseek(f, 0, SEEK_SET); - if (val < 0) { - RTE_LOG(ERR, POWER, "fseek failed\n"); - goto out; - } - - val = fputs(pi->governor_ori, f); - if (val < 0) { - RTE_LOG(ERR, POWER, "fputs failed\n"); - goto out; - } - - ret = 0; - RTE_LOG(INFO, POWER, "Power management governor of lcore %u " - "has been set back to %s successfully\n", - pi->lcore_id, pi->governor_ori); -out: - fclose(f); - - return ret; -} - int rte_power_acpi_cpufreq_exit(unsigned lcore_id) { @@ -452,8 +420,11 @@ rte_power_acpi_cpufreq_exit(unsigned lcore_id) } /* Close FD of setting freq */ - fclose(pi->f); - pi->f = NULL; + if (close(pi->fd)) { + RTE_LOG(ERR, POWER, "Error while closing governor file\n"); + return -1; + } + pi->fd = -1; /* Set the governor back to the original */ if (power_set_governor_original(pi) < 0) { -- 2.7.4