On 13-Dec-18 10:58 AM, Liang, Ma wrote:
On 10 Dec 16:08, Burakov, Anatoly wrote:
<snip>
Hi Liang,
General comment: please do not break up log strings onto multiple lines. It
makes it harder to grep the codebase. Checkpatch will not warn about log
strings going over 80 characters.
agree, I will update in next version
---
lib/librte_power/Makefile | 2 +
lib/librte_power/meson.build | 4 +-
lib/librte_power/power_pstate_cpufreq.c | 778
++++++++++++++++++++++++++++++++
lib/librte_power/power_pstate_cpufreq.h | 218 +++++++++
lib/librte_power/rte_power.c | 43 +-
lib/librte_power/rte_power.h | 3 +-
6 files changed, 1039 insertions(+), 9 deletions(-)
create mode 100644 lib/librte_power/power_pstate_cpufreq.c
create mode 100644 lib/librte_power/power_pstate_cpufreq.h
<snip>
sources = files('rte_power.c', 'power_acpi_cpufreq.c',
'power_kvm_vm.c', 'guest_channel.c',
- 'rte_power_empty_poll.c')
+ 'rte_power_empty_poll.c',
+ 'power_pstate_cpufreq.c')
headers = files('rte_power.h','rte_power_empty_poll.h')
-deps += ['timer']
diff --git a/lib/librte_power/power_pstate_cpufreq.c
b/lib/librte_power/power_pstate_cpufreq.c
new file mode 100644
index 0000000..f1dada8
--- /dev/null
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -0,0 +1,778 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2018 Intel Corporation
I believe it should be 2018.
I didn't see anything wrong here.
The copyright on the file should start when it was first written. You're
created this file in 2018, not 2010 :)
+ */
+
+#include <stdio.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <signal.h>
+#include <limits.h>
+#include <errno.h>
+
+#include <rte_memcpy.h>
+#include <rte_atomic.h>
+
+#include "power_pstate_cpufreq.h"
+#include "power_common.h"
+
<snip>
+
<snip>
+uint32_t
+power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t
num)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+
+ pi = &lcore_power_info[lcore_id];
+ if (num < pi->nb_freqs) {
+ RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
+ return 0;
+ }
+ rte_memcpy(freqs, pi->freqs, pi->nb_freqs * sizeof(uint32_t));
Why rte_memcpy?
the table can be large
So? :) This isn't a hotpath, so regular memcpy should be fine. I mean,
it's OK to use rte_memcpy as well, just seems weird.
+
+ return pi->nb_freqs;
+}
+
+uint32_t
+power_pstate_cpufreq_get_freq(unsigned int lcore_id)
+{
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return RTE_POWER_INVALID_FREQ_INDEX;
+ }
+
+ return lcore_power_info[lcore_id].curr_idx;
+}
+
<snip>
+ caps->turbo = !!(pi->turbo_available);
+
+ return 0;
+}
diff --git a/lib/librte_power/power_pstate_cpufreq.h
b/lib/librte_power/power_pstate_cpufreq.h
new file mode 100644
index 0000000..0fc917a
--- /dev/null
+++ b/lib/librte_power/power_pstate_cpufreq.h
@@ -0,0 +1,218 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2018 Intel Corporation
I believe it should be just 2018.
+ */
+
+#ifndef _POWER_PSTATE_CPUFREQ_H
+#define _POWER_PSTATE_CPUFREQ_H
+
+/**
+ * @file
+ * RTE Power Management via Intel Pstate driver
+ */
+
+#include <rte_common.h>
+#include <rte_byteorder.h>
+#include <rte_log.h>
+#include <rte_string_fns.h>
+#include "rte_power.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
I don't think this is necessary. These extern declarations are only for
public API headers, so that C++ compilers could parse them. Since this is
not a public header, there's no need for extern C declaration.
just keep as same as other headers
Well, other headers have it wrong then, if they too specify this extern
declaration. This is only needed for public-facing headers. Don't copy
bad/unnecessary code from other files - consistency is important, but
not *that* important :)
+
+/**
+ * Initialize power management for a specific lcore. It will check and set the
+ * governor to performance for the lcore, get the available frequencies, and
+ * prepare to set new lcore frequency.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * - 0 on success.
+ * - Negative on error.
+ */
<snip>
RTE_LOG(ERR, POWER, "Environment has not been set, unable to exit "
"gracefully\n");
diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
index d70bc0b..c5e8f6b 100644
--- a/lib/librte_power/rte_power.h
+++ b/lib/librte_power/rte_power.h
@@ -20,7 +20,8 @@ extern "C" {
#endif
/* Power Management Environment State */
-enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM};
+enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
+ PM_ENV_PSTATE_CPUFREQ};
I don't like this approach. While it can be argued that application needs to
be explicitly aware of whether it's using native (ACPI or pstate) vs. KVM
power management, there's no real reason for an application to differentiate
between ACPI and pstate modes.
Changing this would of course require a deprecation notice, so for now, can
we hide all of this behind ACPI mode, and auto-detect whether we want ACPI
or pstate mode? IMO it would be better for the user application to use a
somewhat misnamed ACPI option for both ACPI and pstate modes, than to
needlessly care about whether one or the other is in use.
What do you think?
acpi has significant difference with hwp. sysfs/xxxx/setspeed
sysfs/xxxx/scaling_driver is different
I would like to make application aware the driver type.
Yes, however please correct me if i'm wrong - aren't all of these
changes abstracted away by the power library API? However different they
are internally, does the *application* really need to differentiate
between the two? Are there differences *to the user* when using power
API with pstate vs. ACPI? If not, then why do we need another env type
when we can handle the differences internally?
/**
* Set the default power management implementation. If this is not called
prior
--
Thanks,
Anatoly
--
Thanks,
Anatoly
