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 608ED1B3D4; Wed, 18 Oct 2017 12:28:16 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Oct 2017 03:28:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,396,1503385200"; d="scan'208";a="1207107161" Received: from dhunt5-mobl1.ger.corp.intel.com (HELO [10.237.220.44]) ([10.237.220.44]) by fmsmga001.fm.intel.com with ESMTP; 18 Oct 2017 03:28:13 -0700 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> From: "Hunt, David" Message-ID: <5ed2fa68-31b6-41aa-8589-b6f803dd9898@intel.com> Date: Wed, 18 Oct 2017 11:28:12 +0100 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: <1508161628-4265-1-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 1/2] power: switching to unbuffered stdio for /sys file access 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: Wed, 18 Oct 2017 10:28:16 -0000 Hi Radoslaw, Thanks for the patch. Some comments below. On 16/10/2017 2:47 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. Each write operation when using buffered stdio > must be flushed out and the return code from fflush() must > be verified. In buffered mode, write() syscall return value > is is not returned by fwrite()/fputs()/fprintf(). > Since with buffered approatch, fflush() need to be done > every time it is better to use unbuffered mode or not use > stdio at all (instead use plain open/write functions). To > minimize amount of changes this fix use first approach. > > Signed-off-by: Radoslaw Biernacki > --- > lib/librte_power/rte_power_acpi_cpufreq.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c > index 01ac5ac..8bf5685 100644 > --- a/lib/librte_power/rte_power_acpi_cpufreq.c > +++ b/lib/librte_power/rte_power_acpi_cpufreq.c > @@ -143,12 +143,13 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx) > "for setting frequency for lcore %u\n", pi->lcore_id); > return -1; > } > + /* we use unbuffered mode so following will fail if kernel will refuse > + * freq setting */ > 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; > One suggestion to to avoid switching to unbuffered mode would be to move the check from the fprintf to the fflush. That would eliminate the need for the changes below, keeping the changes to a minimum. But I also agree that open/write would be a better option, should you prefer to go that route. > return 1; > @@ -174,6 +175,11 @@ power_set_governor_userspace(struct rte_power_info *pi) > f = fopen(fullpath, "rw+"); > FOPEN_OR_ERR_RET(f, ret); > > + if (setvbuf(f, NULL, _IONBF, 0)) { > + RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n"); > + goto out; > + } > + > s = fgets(buf, sizeof(buf), f); > FOPS_OR_NULL_GOTO(s, out); > > @@ -292,6 +298,11 @@ power_init_for_setting_freq(struct rte_power_info *pi) > f = fopen(fullpath, "rw+"); > FOPEN_OR_ERR_RET(f, -1); > > + if (setvbuf(f, NULL, _IONBF, 0)) { > + RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n"); > + goto out; > + } > + > s = fgets(buf, sizeof(buf), f); > FOPS_OR_NULL_GOTO(s, out); > > @@ -389,6 +400,11 @@ power_set_governor_original(struct rte_power_info *pi) > f = fopen(fullpath, "rw+"); > FOPEN_OR_ERR_RET(f, ret); > > + if (setvbuf(f, NULL, _IONBF, 0)) { > + RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n"); > + goto out; > + } > + > s = fgets(buf, sizeof(buf), f); > FOPS_OR_NULL_GOTO(s, out); > Regards, Dave