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

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

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