From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755058AbcHVRQX (ORCPT ); Mon, 22 Aug 2016 13:16:23 -0400 Received: from mail-it0-f51.google.com ([209.85.214.51]:35196 "EHLO mail-it0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752585AbcHVRQV (ORCPT ); Mon, 22 Aug 2016 13:16:21 -0400 Reply-To: ahs3@redhat.com Subject: Re: [PATCH v5] Force cppc_cpufreq to report values in KHz to fix user space reporting References: <1469049004-19069-1-git-send-email-ahs3@redhat.com> <20160801203156.GV4605@ubuntu> <872a4dd4-a752-507b-e720-ec2e6003bd8c@redhat.com> To: Viresh Kumar , ashwinch@google.com Cc: rjw@rjwysocki.net, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, Prashanth Prakash , "Rafael J. Wysocki" From: Al Stone Organization: Red Hat, Inc. Message-ID: <406534f5-5e45-237e-cc68-2b57631e26d1@redhat.com> Date: Mon, 22 Aug 2016 11:16:18 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <872a4dd4-a752-507b-e720-ec2e6003bd8c@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Maybe a top-post will get attention.... Yet another ping; this was first submitted on 20 July, and has received no comments. It has now been a month and other architectures are starting to use CPPC so they will run into the same errors that this fixes. Can I get an ACK, NAK, or further instructions, please? Also adding Rafael on the ACPI side, just in case, since he's also reviewing the Intel patches on the linux-acpi mailing list that are adding CPPC usage. On 08/11/2016 12:15 PM, Al Stone wrote: > On 08/01/2016 02:31 PM, Viresh Kumar wrote: >> [+ Ashwin's new email id..] >> >> On 20-07-16, 15:10, Al Stone wrote: >>> When CPPC is being used by ACPI on arm64, user space tools such as >>> cpupower report CPU frequency values from sysfs that are incorrect. >>> >>> What the driver was doing was reporting the values given by ACPI tables >>> in whatever scale was used to provide them. However, the ACPI spec >>> defines the CPPC values as unitless abstract numbers. Internal kernel >>> structures such as struct perf_cap, in contrast, expect these values >>> to be in KHz. When these struct values get reported via sysfs, the >>> user space tools also assume they are in KHz, causing them to report >>> incorrect values (for example, reporting a CPU frequency of 1MHz when >>> it should be 1.8GHz). >>> >>> The downside is that this approach has some assumptions: >>> >>> (1) It relies on SMBIOS3 being used, *and* that the Max Frequency >>> value for a processor is set to a non-zero value. >>> >>> (2) It assumes that all processors run at the same speed, or that >>> the CPPC values have all been scaled to reflect relative speed. >>> This patch retrieves the largest CPU Max Frequency from a type 4 DMI >>> record that it can find. This may not be an issue, however, as a >>> sampling of DMI data on x86 and arm64 indicates there is often only >>> one such record regardless. Since CPPC is relatively new, it is >>> unclear if the ACPI ASL will always be written to reflect any sort >>> of relative performance of processors of differing speeds. >>> >>> (3) It assumes that performance and frequency both scale linearly. >>> >>> For arm64 servers, this may be sufficient, but it does rely on >>> firmware values being set correctly. Hence, other approaches will >>> be considered in the future. >>> >>> This has been tested on three arm64 servers, with and without DMI, with >>> and without CPPC support. >>> >>> Changes for v5: >>> -- Move code to cpufreq/cppc_cpufreq.c from acpi/cppc_acpi.c to keep >>> frequency-related code together, and keep the CPPC abstract scale >>> in ACPI (Prashanth Prakash) >>> -- Fix the scaling to remove the incorrect assumption that frequency >>> was always a range from zero to max; as a practical matter, it is >>> not (Prasanth Prakash); this also allowed us to remove an over- >>> engineered function to do this math. >>> >>> Changes for v4: >>> -- Replaced magic constants with #defines (Rafael Wysocki) >>> -- Renamed cppc_unitless_to_khz() to cppc_to_khz() (Rafael Wysocki) >>> -- Replaced hidden initialization with a clearer form (Rafael Wysocki) >>> -- Instead of picking up the first Max Speed value from DMI, we will >>> now get the largest Max Speed; still an approximation, but slightly >>> less subject to error (Rafael Wysocki) >>> -- Kconfig for cppc_cpufreq now depends on DMI, instead of selecting >>> it, in order to make sure DMI is set up properly (Rafael Wysocki) >>> >>> Changes for v3: >>> -- Added clarifying commentary re short-term vs long-term fix (Alexey >>> Klimov) >>> -- Added range checking code to ensure proper arithmetic occurs, >>> especially no division by zero (Alexey Klimov) >>> >>> Changes for v2: >>> -- Corrected thinko: needed to have DEPENDS on DMI in Kconfig.arm, >>> not SELECT DMI (found by build daemon) >>> >>> Signed-off-by: Al Stone >>> Signed-off-by: Prashanth Prakash >>> --- >>> drivers/cpufreq/cppc_cpufreq.c | 53 ++++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 49 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >>> index 8882b8e..6debc18 100644 >>> --- a/drivers/cpufreq/cppc_cpufreq.c >>> +++ b/drivers/cpufreq/cppc_cpufreq.c >>> @@ -19,10 +19,19 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> >>> +#include >>> + >>> #include >>> >>> +/* Minimum struct length needed for the DMI processor entry we want */ >>> +#define DMI_ENTRY_PROCESSOR_MIN_LENGTH 48 >>> + >>> +/* Offest in the DMI processor structure for the max frequency */ >>> +#define DMI_PROCESSOR_MAX_SPEED 0x14 >>> + >>> /* >>> * These structs contain information parsed from per CPU >>> * ACPI _CPC structures. >>> @@ -32,6 +41,39 @@ >>> */ >>> static struct cpudata **all_cpu_data; >>> >>> +/* Capture the max KHz from DMI */ >>> +static u64 cppc_dmi_max_khz; >>> + >>> +/* Callback function used to retrieve the max frequency from DMI */ >>> +static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private) >>> +{ >>> + const u8 *dmi_data = (const u8 *)dm; >>> + u16 *mhz = (u16 *)private; >>> + >>> + if (dm->type == DMI_ENTRY_PROCESSOR && >>> + dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) { >>> + u16 val = (u16)get_unaligned((const u16 *) >>> + (dmi_data + DMI_PROCESSOR_MAX_SPEED)); >>> + *mhz = val > *mhz ? val : *mhz; >>> + } >>> +} >>> + >>> +/* Look up the max frequency in DMI */ >>> +static u64 cppc_get_dmi_max_khz(void) >>> +{ >>> + u16 mhz = 0; >>> + >>> + dmi_walk(cppc_find_dmi_mhz, &mhz); >>> + >>> + /* >>> + * Real stupid fallback value, just in case there is no >>> + * actual value set. >>> + */ >>> + mhz = mhz ? mhz : 1; >>> + >>> + return (1000 * mhz); >>> +} >>> + >>> static int cppc_cpufreq_set_target(struct cpufreq_policy *policy, >>> unsigned int target_freq, >>> unsigned int relation) >>> @@ -42,7 +84,7 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy, >>> >>> cpu = all_cpu_data[policy->cpu]; >>> >>> - cpu->perf_ctrls.desired_perf = target_freq; >>> + cpu->perf_ctrls.desired_perf = target_freq * policy->max / cppc_dmi_max_khz; >>> freqs.old = policy->cur; >>> freqs.new = target_freq; >>> >>> @@ -94,8 +136,10 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) >>> return ret; >>> } >>> >>> - policy->min = cpu->perf_caps.lowest_perf; >>> - policy->max = cpu->perf_caps.highest_perf; >>> + cppc_dmi_max_khz = cppc_get_dmi_max_khz(); >>> + >>> + policy->min = cpu->perf_caps.lowest_perf * cppc_dmi_max_khz / cpu->perf_caps.highest_perf; >>> + policy->max = cppc_dmi_max_khz; >>> policy->cpuinfo.min_freq = policy->min; >>> policy->cpuinfo.max_freq = policy->max; >>> policy->shared_type = cpu->shared_type; >>> @@ -112,7 +156,8 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) >>> cpu->cur_policy = policy; >>> >>> /* Set policy->cur to max now. The governors will adjust later. */ >>> - policy->cur = cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf; >>> + policy->cur = cppc_dmi_max_khz; >>> + cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf; >>> >>> ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls); >>> if (ret) >>> -- >>> 2.7.4 >> > > Another gentle ping -- any comments? Can this get pulled in now? > > Thanks. > -- ciao, al ----------------------------------- Al Stone Software Engineer Red Hat, Inc. ahs3@redhat.com ----------------------------------- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Stone Subject: Re: [PATCH v5] Force cppc_cpufreq to report values in KHz to fix user space reporting Date: Mon, 22 Aug 2016 11:16:18 -0600 Message-ID: <406534f5-5e45-237e-cc68-2b57631e26d1@redhat.com> References: <1469049004-19069-1-git-send-email-ahs3@redhat.com> <20160801203156.GV4605@ubuntu> <872a4dd4-a752-507b-e720-ec2e6003bd8c@redhat.com> Reply-To: ahs3@redhat.com Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <872a4dd4-a752-507b-e720-ec2e6003bd8c@redhat.com> Sender: linux-kernel-owner@vger.kernel.org To: Viresh Kumar , ashwinch@google.com Cc: rjw@rjwysocki.net, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, Prashanth Prakash "Rafael J. Wysocki" List-Id: linux-pm@vger.kernel.org Maybe a top-post will get attention.... Yet another ping; this was first submitted on 20 July, and has received no comments. It has now been a month and other architectures are starting to use CPPC so they will run into the same errors that this fixes. Can I get an ACK, NAK, or further instructions, please? Also adding Rafael on the ACPI side, just in case, since he's also reviewing the Intel patches on the linux-acpi mailing list that are adding CPPC usage. On 08/11/2016 12:15 PM, Al Stone wrote: > On 08/01/2016 02:31 PM, Viresh Kumar wrote: >> [+ Ashwin's new email id..] >> >> On 20-07-16, 15:10, Al Stone wrote: >>> When CPPC is being used by ACPI on arm64, user space tools such as >>> cpupower report CPU frequency values from sysfs that are incorrect. >>> >>> What the driver was doing was reporting the values given by ACPI tables >>> in whatever scale was used to provide them. However, the ACPI spec >>> defines the CPPC values as unitless abstract numbers. Internal kernel >>> structures such as struct perf_cap, in contrast, expect these values >>> to be in KHz. When these struct values get reported via sysfs, the >>> user space tools also assume they are in KHz, causing them to report >>> incorrect values (for example, reporting a CPU frequency of 1MHz when >>> it should be 1.8GHz). >>> >>> The downside is that this approach has some assumptions: >>> >>> (1) It relies on SMBIOS3 being used, *and* that the Max Frequency >>> value for a processor is set to a non-zero value. >>> >>> (2) It assumes that all processors run at the same speed, or that >>> the CPPC values have all been scaled to reflect relative speed. >>> This patch retrieves the largest CPU Max Frequency from a type 4 DMI >>> record that it can find. This may not be an issue, however, as a >>> sampling of DMI data on x86 and arm64 indicates there is often only >>> one such record regardless. Since CPPC is relatively new, it is >>> unclear if the ACPI ASL will always be written to reflect any sort >>> of relative performance of processors of differing speeds. >>> >>> (3) It assumes that performance and frequency both scale linearly. >>> >>> For arm64 servers, this may be sufficient, but it does rely on >>> firmware values being set correctly. Hence, other approaches will >>> be considered in the future. >>> >>> This has been tested on three arm64 servers, with and without DMI, with >>> and without CPPC support. >>> >>> Changes for v5: >>> -- Move code to cpufreq/cppc_cpufreq.c from acpi/cppc_acpi.c to keep >>> frequency-related code together, and keep the CPPC abstract scale >>> in ACPI (Prashanth Prakash) >>> -- Fix the scaling to remove the incorrect assumption that frequency >>> was always a range from zero to max; as a practical matter, it is >>> not (Prasanth Prakash); this also allowed us to remove an over- >>> engineered function to do this math. >>> >>> Changes for v4: >>> -- Replaced magic constants with #defines (Rafael Wysocki) >>> -- Renamed cppc_unitless_to_khz() to cppc_to_khz() (Rafael Wysocki) >>> -- Replaced hidden initialization with a clearer form (Rafael Wysocki) >>> -- Instead of picking up the first Max Speed value from DMI, we will >>> now get the largest Max Speed; still an approximation, but slightly >>> less subject to error (Rafael Wysocki) >>> -- Kconfig for cppc_cpufreq now depends on DMI, instead of selecting >>> it, in order to make sure DMI is set up properly (Rafael Wysocki) >>> >>> Changes for v3: >>> -- Added clarifying commentary re short-term vs long-term fix (Alexey >>> Klimov) >>> -- Added range checking code to ensure proper arithmetic occurs, >>> especially no division by zero (Alexey Klimov) >>> >>> Changes for v2: >>> -- Corrected thinko: needed to have DEPENDS on DMI in Kconfig.arm, >>> not SELECT DMI (found by build daemon) >>> >>> Signed-off-by: Al Stone >>> Signed-off-by: Prashanth Prakash >>> --- >>> drivers/cpufreq/cppc_cpufreq.c | 53 ++++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 49 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >>> index 8882b8e..6debc18 100644 >>> --- a/drivers/cpufreq/cppc_cpufreq.c >>> +++ b/drivers/cpufreq/cppc_cpufreq.c >>> @@ -19,10 +19,19 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> >>> +#include >>> + >>> #include >>> >>> +/* Minimum struct length needed for the DMI processor entry we want */ >>> +#define DMI_ENTRY_PROCESSOR_MIN_LENGTH 48 >>> + >>> +/* Offest in the DMI processor structure for the max frequency */ >>> +#define DMI_PROCESSOR_MAX_SPEED 0x14 >>> + >>> /* >>> * These structs contain information parsed from per CPU >>> * ACPI _CPC structures. >>> @@ -32,6 +41,39 @@ >>> */ >>> static struct cpudata **all_cpu_data; >>> >>> +/* Capture the max KHz from DMI */ >>> +static u64 cppc_dmi_max_khz; >>> + >>> +/* Callback function used to retrieve the max frequency from DMI */ >>> +static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private) >>> +{ >>> + const u8 *dmi_data = (const u8 *)dm; >>> + u16 *mhz = (u16 *)private; >>> + >>> + if (dm->type == DMI_ENTRY_PROCESSOR && >>> + dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) { >>> + u16 val = (u16)get_unaligned((const u16 *) >>> + (dmi_data + DMI_PROCESSOR_MAX_SPEED)); >>> + *mhz = val > *mhz ? val : *mhz; >>> + } >>> +} >>> + >>> +/* Look up the max frequency in DMI */ >>> +static u64 cppc_get_dmi_max_khz(void) >>> +{ >>> + u16 mhz = 0; >>> + >>> + dmi_walk(cppc_find_dmi_mhz, &mhz); >>> + >>> + /* >>> + * Real stupid fallback value, just in case there is no >>> + * actual value set. >>> + */ >>> + mhz = mhz ? mhz : 1; >>> + >>> + return (1000 * mhz); >>> +} >>> + >>> static int cppc_cpufreq_set_target(struct cpufreq_policy *policy, >>> unsigned int target_freq, >>> unsigned int relation) >>> @@ -42,7 +84,7 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy, >>> >>> cpu = all_cpu_data[policy->cpu]; >>> >>> - cpu->perf_ctrls.desired_perf = target_freq; >>> + cpu->perf_ctrls.desired_perf = target_freq * policy->max / cppc_dmi_max_khz; >>> freqs.old = policy->cur; >>> freqs.new = target_freq; >>> >>> @@ -94,8 +136,10 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) >>> return ret; >>> } >>> >>> - policy->min = cpu->perf_caps.lowest_perf; >>> - policy->max = cpu->perf_caps.highest_perf; >>> + cppc_dmi_max_khz = cppc_get_dmi_max_khz(); >>> + >>> + policy->min = cpu->perf_caps.lowest_perf * cppc_dmi_max_khz / cpu->perf_caps.highest_perf; >>> + policy->max = cppc_dmi_max_khz; >>> policy->cpuinfo.min_freq = policy->min; >>> policy->cpuinfo.max_freq = policy->max; >>> policy->shared_type = cpu->shared_type; >>> @@ -112,7 +156,8 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) >>> cpu->cur_policy = policy; >>> >>> /* Set policy->cur to max now. The governors will adjust later. */ >>> - policy->cur = cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf; >>> + policy->cur = cppc_dmi_max_khz; >>> + cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf; >>> >>> ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls); >>> if (ret) >>> -- >>> 2.7.4 >> > > Another gentle ping -- any comments? Can this get pulled in now? > > Thanks. > -- ciao, al ----------------------------------- Al Stone Software Engineer Red Hat, Inc. ahs3@redhat.com -----------------------------------