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

[Liang, Ma]

 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.;-)
> > > 
> > > >    /**
> > > >     * Set the default power management implementation. If this is not called prior
> > > > 
> > > 
> > > -- 
> > > Thanks,
> > > Anatoly
> > 
> 
> 
> -- 
> Thanks,
> Anatoly