[dpdk-dev] [PATCH] libs/power: add p-state driver compatibility
Burakov, Anatoly
anatoly.burakov at intel.com
Thu Dec 13 12:16:52 CET 2018
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
More information about the dev
mailing list