[dpdk-dev] [PATCH 2/3] power: switching to unbuffered access for /sys files
Radoslaw Biernacki
radoslaw.biernacki at linaro.org
Sat Nov 11 19:55:06 CET 2017
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 at dpdk.org
Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki at 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) {
--
2.7.4
More information about the dev
mailing list