From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id A9D151B1E6 for ; Thu, 13 Dec 2018 14:53:44 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Dec 2018 05:53:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,349,1539673200"; d="scan'208";a="259187961" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.93]) ([10.237.220.93]) by orsmga004.jf.intel.com with ESMTP; 13 Dec 2018 05:53:41 -0800 To: "Liang, Ma" Cc: david.hunt@intel.com, dev@dpdk.org, lei.a.yao@intel.com, ktraynor@redhat.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> <20181213134616.GB26517@sivswdev09.ir.intel.com> From: "Burakov, Anatoly" Message-ID: <54856729-9acd-916f-be20-824f65511915@intel.com> Date: Thu, 13 Dec 2018 13:53:39 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.3.3 MIME-Version: 1.0 In-Reply-To: <20181213134616.GB26517@sivswdev09.ir.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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:53:45 -0000 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: >>> >>>> 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.;-) OK, fair enough so. -- Thanks, Anatoly