[dpdk-dev] [PATCH 2/3] power: switching to unbuffered access for /sys files
Radoslaw Biernacki
radoslaw.biernacki at linaro.org
Fri Dec 1 22:01:35 CET 2017
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 at 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 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) {
>>
>
> 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
>
>
More information about the dev
mailing list