[dpdk-dev] [PATCH 2/3] power: switching to unbuffered access for /sys files

[Hunt, David]

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