Re: [dpdk-dev] [PATCH] libs/power: add p-state driver compatibility

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 13-Dec-18 1:46 PM, Liang, Ma wrote:
>   13 Dec 11:16, Burakov, Anatoly wrote:
>> 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?
>>
> Currently,  rte_power_init will try all three subsystem one by one to figure out the right driver.
> however, the rte_power_set_env as  a interface is keep here for flexibility I thought.
> also that's the original design for the library. if we find better way, we can implementa that
> in the future. but that's other patch then.;-)

OK, fair enough so.

-- 
Thanks,
Anatoly