| Message ID | 1510426507-28245-3-git-send-email-radoslaw.biernacki@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | power: fixes for power ACPI through sysfs | expand |
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 <radoslaw.biernacki@linaro.org> > --- > 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 <assert.h> > #include <stdio.h> > #include <sys/types.h> > #include <sys/stat.h> > @@ -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
Hi David, Sorry for log delay. I will make V3 with your suggestions. Thanks. On 23 November 2017 at 15:42, Hunt, David <david.hunt@intel.com> 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 <radoslaw.biernacki@linaro.org> >> --- >> 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 <assert.h> >> #include <stdio.h> >> #include <sys/types.h> >> #include <sys/stat.h> >> @@ -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 > >
01/12/2017 22:01, Radoslaw Biernacki: > Hi David, > > Sorry for log delay. > I will make V3 with your suggestions. > Thanks. Hi, Any news?
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 <assert.h> #include <stdio.h> #include <sys/types.h> #include <sys/stat.h> @@ -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) {
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 <radoslaw.biernacki@linaro.org> --- lib/librte_power/rte_power_acpi_cpufreq.c | 211 +++++++++++++----------------- 1 file changed, 91 insertions(+), 120 deletions(-) -- 2.7.4