From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id 37B3E3257 for ; Fri, 1 Dec 2017 22:01:56 +0100 (CET) Received: by mail-wm0-f66.google.com with SMTP id v19so5478222wmh.5 for ; Fri, 01 Dec 2017 13:01:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=VAI1LsRO5+spsZ9ugSuIJdRgg9wf0P3AEX6IdcHhL2k=; b=ilkOfYBcI5iBAofzEpn9z5LdRGO3+7ksq0bHrNfZS6W5IFsbJwDFxrC9CYeM1VaFDK uuVZ37FFdJorkFj++1FqVpCzOBa1x/MLXhP42UyIqwsR7klyLe58+OmepQcEBV2tV37Z dRiqFQT/aoU3Xzi3nRtqgV8o10CtLat6GAtfo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=VAI1LsRO5+spsZ9ugSuIJdRgg9wf0P3AEX6IdcHhL2k=; b=AUBu8MVVbpl7j8Yofzt+C3detmk+/hdZf/xupKNryNmzRzi924311yJd98oQ5coaVr XZ+vB6lvS9gW4fsS1GYmYDqPmNbzbZ6OOPlaXZj72l3UMuyjrzMn1Ey45/7j7elsiJQP TVzo6gMMAHPBukNx5M4NTSvoOKHTQ//MrPCUSuoSOWxWIzmxdgdPrjau0biAjDsLHGRs 2PQr3jEnc+3YGpyIl7X0lx0WyRlIPXuh0keMhwbBSBijgtkuAmMRR+fOwG1g4uokmPVB rBoOo53ZrWIBcMfw8c7OzwgKMZFPReDfCHCAWybGzH/RJjvS1svtq03hjhLStrZE5h62 D99w== X-Gm-Message-State: AJaThX6NHmL7LpslVgZ3HIz/1JY+phbQMwLctEiduGSBNE5vpBSQXvEP kga8jMHlsDCqYbbYWN7wADGQG9ySVdZTC1p14sMQew== X-Google-Smtp-Source: AGs4zMbLAU1zI5DV8Ta52HIyZC1l6LF35zf3l2PTlQFn4hiarYAeVKhKV9wsCtr/3TpIVhQg0JVz2EWSre+Dwvdwc4w= X-Received: by 10.28.172.66 with SMTP id v63mr2102004wme.37.1512162115766; Fri, 01 Dec 2017 13:01:55 -0800 (PST) MIME-Version: 1.0 Received: by 10.223.163.25 with HTTP; Fri, 1 Dec 2017 13:01:35 -0800 (PST) In-Reply-To: References: <1508161628-4265-1-git-send-email-radoslaw.biernacki@linaro.org> <1510426507-28245-1-git-send-email-radoslaw.biernacki@linaro.org> <1510426507-28245-3-git-send-email-radoslaw.biernacki@linaro.org> From: Radoslaw Biernacki Date: Fri, 1 Dec 2017 22:01:35 +0100 Message-ID: To: "Hunt, David" Cc: dev@dpdk.org, stable@dpdk.org, alan.carew@intel.com, pablo.de.lara.guarch@intel.com Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [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: Fri, 01 Dec 2017 21:01:56 -0000 Hi David, Sorry for log delay. I will make V3 with your suggestions. Thanks. On 23 November 2017 at 15:42, Hunt, David wrote: > Hi Radoslaw, > > > > On 11/11/2017 6:55 PM, Radoslaw Biernacki wrote: > >> 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) { >> > > Could you re-base on top of 17.11? It fails to apply currently. > > I think there is a little much going on in this patch, there are a couple > of discreet changes that could/should be split into individual patches. > Your first patch in this series does this well, remove the macros. > I think this one could be split into two patches: > 1. First the switch to unbuffered file access, including the extra > error checking. > 2. Then rework the functions, i.e. add the new "power_set_governor" > function and have "_userspace" and "_original" call that. > Do you agree? > Regards, > Dave > >