From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id C99671B1E6 for ; Thu, 13 Dec 2018 14:46:19 +0100 (CET) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Dec 2018 05:46:18 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,349,1539673200"; d="scan'208";a="303535653" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by fmsmga005.fm.intel.com with ESMTP; 13 Dec 2018 05:46:17 -0800 Received: from sivswdev09.ir.intel.com (sivswdev09.ir.intel.com [10.237.217.48]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id wBDDkGoF032258; Thu, 13 Dec 2018 13:46:16 GMT Received: from sivswdev09.ir.intel.com (localhost [127.0.0.1]) by sivswdev09.ir.intel.com with ESMTP id wBDDkGxY020905; Thu, 13 Dec 2018 13:46:16 GMT Received: (from lma25@localhost) by sivswdev09.ir.intel.com with LOCAL id wBDDkG7c020901; Thu, 13 Dec 2018 13:46:16 GMT Date: Thu, 13 Dec 2018 13:46:16 +0000 From: "Liang, Ma" To: "Burakov, Anatoly" Cc: david.hunt@intel.com, dev@dpdk.org, lei.a.yao@intel.com, ktraynor@redhat.com Message-ID: <20181213134616.GB26517@sivswdev09.ir.intel.com> References: <1542972781-6149-1-git-send-email-liang.j.ma@intel.com> <20181213105848.GA26517@sivswdev09.ir.intel.com> <79d8a78c-0dad-e750-5c71-7eacbdce9528@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <79d8a78c-0dad-e750-5c71-7eacbdce9528@intel.com> User-Agent: Mutt/1.9.1 (2017-09-22) Subject: Re: [dpdk-dev] [PATCH] libs/power: add p-state driver compatibility X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Dec 2018 13:46:20 -0000 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: > > > > > 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 > > > > > > > > > > > > > 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 > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +#include > > > > +#include > > > > + > > > > +#include "power_pstate_cpufreq.h" > > > > +#include "power_common.h" > > > > + > > > > > > > > > > + > > > > > > > +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; > > > > +} > > > > + > > > > > > > > > > > > > + 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 > > > > +#include > > > > +#include > > > > +#include > > > > +#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. > > > > + */ > > > > > > > > > > > > > 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