From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 4C2992C16; Thu, 23 Nov 2017 15:42:12 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Nov 2017 06:42:10 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,441,1505804400"; d="scan'208";a="1247670782" Received: from dhunt5-mobl1.ger.corp.intel.com (HELO [10.237.220.36]) ([10.237.220.36]) by fmsmga002.fm.intel.com with ESMTP; 23 Nov 2017 06:42:08 -0800 To: Radoslaw Biernacki , dev@dpdk.org Cc: stable@dpdk.org, alan.carew@intel.com, pablo.de.lara.guarch@intel.com 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: "Hunt, David" Message-ID: Date: Thu, 23 Nov 2017 14:42:07 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1510426507-28245-3-git-send-email-radoslaw.biernacki@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US 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: Thu, 23 Nov 2017 14:42:14 -0000 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