From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1D4E1C433DB for ; Wed, 3 Feb 2021 08:41:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C7D9464DD4 for ; Wed, 3 Feb 2021 08:41:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233014AbhBCIlg (ORCPT ); Wed, 3 Feb 2021 03:41:36 -0500 Received: from mx2.suse.de ([195.135.220.15]:48040 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232978AbhBCIkH (ORCPT ); Wed, 3 Feb 2021 03:40:07 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 6B919AC9B; Wed, 3 Feb 2021 08:39:25 +0000 (UTC) Message-ID: <1612341564.3640.14.camel@suse.cz> Subject: Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula From: Giovanni Gherdovich To: "Rafael J. Wysocki" , Viresh Kumar Cc: Borislav Petkov , Ingo Molnar , Peter Zijlstra , "Rafael J . Wysocki" , Jon Grimm , Nathan Fontenot , Yazen Ghannam , Thomas Lendacky , Suthikulpanit Suravee , Mel Gorman , Pu Wen , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Michael Larabel , the arch/x86 maintainers , Linux PM , Linux Kernel Mailing List , ACPI Devel Maling List Date: Wed, 03 Feb 2021 09:39:24 +0100 In-Reply-To: References: <20210122204038.3238-1-ggherdovich@suse.cz> <20210122204038.3238-2-ggherdovich@suse.cz> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.26.6 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, both Rafael and Viresh make a similar remark: why adding a new "max_boost" variable, since "max_freq" is already available and could be used instead. Replying here to both. On Tue, 2021-02-02 at 20:26 +0100, Rafael J. Wysocki wrote: > On Tue, Feb 2, 2021 at 7:59 PM Rafael J. Wysocki wrote: > > > > On Fri, Jan 22, 2021 at 9:47 PM Giovanni Gherdovich wrote: > > > > [cut] > > > @@ -779,15 +829,25 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) > > > freq_table[valid_states-1].frequency / 1000) > > > continue; > > > > > > + freq = perf->states[i].core_frequency * 1000; > > > freq_table[valid_states].driver_data = i; > > > - freq_table[valid_states].frequency = > > > - perf->states[i].core_frequency * 1000; > > > + freq_table[valid_states].frequency = freq; > > > + > > > + if (freq > max_freq) > > > + max_freq = freq; > > > + > > > valid_states++; > > > } > > > freq_table[valid_states].frequency = CPUFREQ_TABLE_END; > > > policy->freq_table = freq_table; > > > perf->state = 0; > > > > > > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD && > > > + amd_max_boost(max_freq, &max_boost)) { > > > + policy->cpuinfo.max_boost = max_boost; > > > > Why not to set max_freq to max_boost instead? > > I mean, would setting the frequency in the last table entry to max_boost work? > > Alternatively, one more (artificial) entry with the frequency equal to > max_boost could be added. On Wed, 2021-02-03 at 11:34 +0530, Viresh Kumar wrote: > [cut] > > On 22-01-21, 21:40, Giovanni Gherdovich wrote: > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > > index 6931f0cdeb80..541f3db3f576 100644 > > --- a/kernel/sched/cpufreq_schedutil.c > > +++ b/kernel/sched/cpufreq_schedutil.c > > @@ -159,8 +159,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy, > > unsigned long util, unsigned long max) > > { > > struct cpufreq_policy *policy = sg_policy->policy; > > - unsigned int freq = arch_scale_freq_invariant() ? > > - policy->cpuinfo.max_freq : policy->cur; > > + unsigned int freq, max_freq; > > + > > + max_freq = cpufreq_driver_has_max_boost() ? > > + policy->cpuinfo.max_boost : policy->cpuinfo.max_freq; > > Also, can't we update max_freq itself from the cpufreq driver? What > troubles will it cost ? I could add the max boost frequency to the frequency table (and policy->cpuinfo.max_freq would follow), yes, but that would trigger the following warning from acpi-cpufreq.c: static void acpi_cpufreq_cpu_ready(struct cpufreq_policy *policy) { struct acpi_processor_performance *perf = per_cpu_ptr(acpi_perf_data, policy->cpu); if (perf->states[0].core_frequency * 1000 != policy->cpuinfo.max_freq) pr_warn(FW_WARN "P-state 0 is not max freq\n"); } so I thought that to stay out of troubles I'd supply a different variable, max_boost, to be used only in the schedutil formula. After schedutil figures out a desired next_freq then the usual comparison with the firmware-supplied frequency table could take place. Altering the frequency table seemed more invasive because once a freq value is in there, it's going to be actually requested by the driver to the platform. I only want my max_boost to stretch the range of schedutil's next_freq. On Tue, 2021-02-02 at 19:59 +0100, Rafael J. Wysocki wrote: > > [cut] > Also notice that the static branch is global and the max_boost value > for different CPUs may be different, at least in theory. In theory yes, but I'm guarding the code with two conditions: * vendor is X86_VENDOR_AMD * cppc_get_perf_caps() returns success this identifies AMD EPYC cpus with model 7xx2 and later, where max_boost is the same on all cores. I may have added synchronization so that only one cpu sets the value, but I didn't in the interest of simplicity for an -rc patch (I'd have to consider hotplug, the maxcpus= command line param, ecc). Giovanni