[RFC,1/2] power: fix power library with --lcores
Checks
Commit Message
This commit fixes an issue in the power library
related to using lcores mapped to different
physical cores (--lcores option in EAL).
Previously, the power library incorrectly accessed
CPU sysfs attributes for power management, treating
lcore IDs as CPU IDs.
e.g. with --lcores '1@128', lcore_id '1' was interpreted
as CPU_id instead of '128'.
This patch corrects the cpu_id based on lcore and CPU
mappings. It also constraints power management support
for lcores mapped to multiple physical cores/threads.
When multiple lcores are mapped to the same physical core,
invoking frequency scaling APIs on any lcore will apply the
changes effectively.
Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
---
lib/power/power_acpi_cpufreq.c | 16 ++++++++++++++--
lib/power/power_amd_pstate_cpufreq.c | 16 ++++++++++++++--
lib/power/power_cppc_cpufreq.c | 16 ++++++++++++++--
lib/power/power_pstate_cpufreq.c | 16 ++++++++++++++--
4 files changed, 56 insertions(+), 8 deletions(-)
Comments
On Wed, 24 Jul 2024 13:03:35 +0000
Sivaprasad Tummala <sivaprasad.tummala@amd.com> wrote:
> + POWER_LOG(ERR, "Power library doesn't support lcore %u mapping "
> + "to %u cpus", lcore_id, CPU_COUNT(&lcore_cpus));
Please don't break single format error string across lines. It makes it harder
for users to find the error in the source. Instead wrap after Er,,
On Wed, 24 Jul 2024 13:03:35 +0000
Sivaprasad Tummala <sivaprasad.tummala@amd.com> wrote:
> + lcore_cpus = rte_lcore_cpuset(lcore_id);
> + if (CPU_COUNT(&lcore_cpus) != 1) {
> + POWER_LOG(ERR, "Power library doesn't support lcore %u mapping "
> + "to %u cpus", lcore_id, CPU_COUNT(&lcore_cpus));
> + return -1;
> + }
> + for (cpu = 0; cpu < CPU_SETSIZE; cpu++) {
> + if (CPU_ISSET(cpu, &lcore_cpus))
> + break;
> + }
You are copy and pasting the same code into multiple places which
indicates it should be an API function.
[AMD Official Use Only - AMD Internal Distribution Only]
Hi Stephen,
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, July 24, 2024 8:10 PM
> To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>
> Cc: david.hunt@intel.com; anatoly.burakov@intel.com;
> thomas@monjalon.net; Yigit, Ferruh <Ferruh.Yigit@amd.com>;
> david.marchand@redhat.com; dev@dpdk.org
> Subject: Re: [RFC PATCH 1/2] power: fix power library with --lcores
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Wed, 24 Jul 2024 13:03:35 +0000
> Sivaprasad Tummala <sivaprasad.tummala@amd.com<mailto:sivaprasad.tummala@amd.com>> wrote:
>
> > + lcore_cpus = rte_lcore_cpuset(lcore_id);
> > + if (CPU_COUNT(&lcore_cpus) != 1) {
> > + POWER_LOG(ERR, "Power library doesn't support lcore %u mapping
> "
> > + "to %u cpus", lcore_id, CPU_COUNT(&lcore_cpus));
> > + return -1;
> > + }
> > + for (cpu = 0; cpu < CPU_SETSIZE; cpu++) {
> > + if (CPU_ISSET(cpu, &lcore_cpus))
> > + break;
> > + }
>
> You are copy and pasting the same code into multiple places which indicates it
> should be an API function.
ACK! Will fix this in next version.
@@ -9,6 +9,7 @@
#include <rte_memcpy.h>
#include <rte_stdatomic.h>
#include <rte_string_fns.h>
+#include <rte_lcore.h>
#include "power_acpi_cpufreq.h"
#include "power_common.h"
@@ -234,7 +235,8 @@ int
power_acpi_cpufreq_init(unsigned int lcore_id)
{
struct acpi_power_info *pi;
- uint32_t exp_state;
+ uint32_t exp_state, cpu;
+ rte_cpuset_t lcore_cpus;
if (lcore_id >= RTE_MAX_LCORE) {
POWER_LOG(ERR, "Lcore id %u can not exceeds %u",
@@ -258,7 +260,17 @@ power_acpi_cpufreq_init(unsigned int lcore_id)
return -1;
}
- pi->lcore_id = lcore_id;
+ lcore_cpus = rte_lcore_cpuset(lcore_id);
+ if (CPU_COUNT(&lcore_cpus) != 1) {
+ POWER_LOG(ERR, "Power library doesn't support lcore %u mapping "
+ "to %u cpus", lcore_id, CPU_COUNT(&lcore_cpus));
+ return -1;
+ }
+ for (cpu = 0; cpu < CPU_SETSIZE; cpu++) {
+ if (CPU_ISSET(cpu, &lcore_cpus))
+ break;
+ }
+ pi->lcore_id = cpu;
/* Check and set the governor */
if (power_set_governor_userspace(pi) < 0) {
POWER_LOG(ERR, "Cannot set governor of lcore %u to "
@@ -8,6 +8,7 @@
#include <rte_memcpy.h>
#include <rte_stdatomic.h>
+#include <rte_lcore.h>
#include "power_amd_pstate_cpufreq.h"
#include "power_common.h"
@@ -352,7 +353,8 @@ int
power_amd_pstate_cpufreq_init(unsigned int lcore_id)
{
struct amd_pstate_power_info *pi;
- uint32_t exp_state;
+ uint32_t exp_state, cpu;
+ rte_cpuset_t lcore_cpus;
if (lcore_id >= RTE_MAX_LCORE) {
POWER_LOG(ERR, "Lcore id %u can not exceeds %u",
@@ -376,7 +378,17 @@ power_amd_pstate_cpufreq_init(unsigned int lcore_id)
return -1;
}
- pi->lcore_id = lcore_id;
+ lcore_cpus = rte_lcore_cpuset(lcore_id);
+ if (CPU_COUNT(&lcore_cpus) != 1) {
+ POWER_LOG(ERR, "Power library doesn't support lcore %u mapping "
+ "to %u cpus", lcore_id, CPU_COUNT(&lcore_cpus));
+ return -1;
+ }
+ for (cpu = 0; cpu < CPU_SETSIZE; cpu++) {
+ if (CPU_ISSET(cpu, &lcore_cpus))
+ break;
+ }
+ pi->lcore_id = cpu;
/* Check and set the governor */
if (power_set_governor_userspace(pi) < 0) {
POWER_LOG(ERR, "Cannot set governor of lcore %u to "
@@ -7,6 +7,7 @@
#include <rte_memcpy.h>
#include <rte_stdatomic.h>
+#include <rte_lcore.h>
#include "power_cppc_cpufreq.h"
#include "power_common.h"
@@ -338,7 +339,8 @@ int
power_cppc_cpufreq_init(unsigned int lcore_id)
{
struct cppc_power_info *pi;
- uint32_t exp_state;
+ uint32_t exp_state, cpu;
+ rte_cpuset_t lcore_cpus;
if (lcore_id >= RTE_MAX_LCORE) {
POWER_LOG(ERR, "Lcore id %u can not exceeds %u",
@@ -362,7 +364,17 @@ power_cppc_cpufreq_init(unsigned int lcore_id)
return -1;
}
- pi->lcore_id = lcore_id;
+ lcore_cpus = rte_lcore_cpuset(lcore_id);
+ if (CPU_COUNT(&lcore_cpus) != 1) {
+ POWER_LOG(ERR, "Power library doesn't support lcore %u mapping "
+ "to %u cpus", lcore_id, CPU_COUNT(&lcore_cpus));
+ return -1;
+ }
+ for (cpu = 0; cpu < CPU_SETSIZE; cpu++) {
+ if (CPU_ISSET(cpu, &lcore_cpus))
+ break;
+ }
+ pi->lcore_id = cpu;
/* Check and set the governor */
if (power_set_governor_userspace(pi) < 0) {
POWER_LOG(ERR, "Cannot set governor of lcore %u to "
@@ -13,6 +13,7 @@
#include <rte_memcpy.h>
#include <rte_stdatomic.h>
+#include <rte_lcore.h>
#include "rte_power_pmd_mgmt.h"
#include "power_pstate_cpufreq.h"
@@ -540,7 +541,8 @@ int
power_pstate_cpufreq_init(unsigned int lcore_id)
{
struct pstate_power_info *pi;
- uint32_t exp_state;
+ uint32_t exp_state, cpu;
+ rte_cpuset_t lcore_cpus;
if (lcore_id >= RTE_MAX_LCORE) {
POWER_LOG(ERR, "Lcore id %u can not exceed %u",
@@ -564,7 +566,17 @@ power_pstate_cpufreq_init(unsigned int lcore_id)
return -1;
}
- pi->lcore_id = lcore_id;
+ lcore_cpus = rte_lcore_cpuset(lcore_id);
+ if (CPU_COUNT(&lcore_cpus) != 1) {
+ POWER_LOG(ERR, "Power library doesn't support lcore %u mapping "
+ "to %u cpus", lcore_id, CPU_COUNT(&lcore_cpus));
+ return -1;
+ }
+ for (cpu = 0; cpu < CPU_SETSIZE; cpu++) {
+ if (CPU_ISSET(cpu, &lcore_cpus))
+ break;
+ }
+ pi->lcore_id = cpu;
/* Check and set the governor */
if (power_set_governor_performance(pi) < 0) {
POWER_LOG(ERR, "Cannot set governor of lcore %u to "