Received: by 2002:a05:7412:3210:b0:e2:908c:2ebd with SMTP id eu16csp340609rdb; Thu, 31 Aug 2023 10:49:27 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFA6J0UfojaItiKm5uH6noFo7naDLEyFJ3zCEdsBl56RjEu09RmfiFYlXf6b28H4zpqpZvi X-Received: by 2002:a17:902:8608:b0:1c1:de3a:d3d7 with SMTP id f8-20020a170902860800b001c1de3ad3d7mr258175plo.68.1693504167503; Thu, 31 Aug 2023 10:49:27 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1693504167; cv=none; d=google.com; s=arc-20160816; b=CzhQrfSB5x0mXGaq/WZZk0klTwrm7FxuchxNX4jAY6/GbLVuiHuY77o+6u97GXV8c+ AwpN+fmiXhhsG45lai3etThUezjYET/dlwN3ReiQT7BdmgMI04EAqbz0YHExq1TLOtJA 9vLoE9KISJtBdrVaONn8pkj8JIDGUruRRxY6fVBVfxSN4a9e+eAeTmLLzl1O2XuJGKsd bwcuS1FhK0iBvyZwuPdmsUEKofMubj+C4sEBqmVPzwWO5Bf76tqbewDkVv0m1Ex1fj8A FEjY/Vw9BvCvnDX7XSCD+oWxjGwszjSt6XsOBhmiY3bpEbnwFyVmiCjQiPgvur65iayO mT6A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:cc:to:subject :message-id:date:from:in-reply-to:references:mime-version; bh=/54SOxE98oqtMUMeV/bFrtzDcU23AThacExssr5EzWM=; fh=TM6clYkeSFDGr4fGluSeJB2oAOUUdvK+SjHm8Tahi24=; b=wvesXDKYZZgHEyQ8HeOAswQ7xsHDh1p1MpiMntiTAqoG7veo20wuhElYzMuH/LKiiN fjYWMNFuATU0CnAJ8QgndUCFEfOvZCtMgdkOSzd+1OlKXkqmKd7EIX8mpNXLln334qid GM92uvJE6ppe0Oq3DC5ZkmVgzbJbkajuLNu1WNMe9388fNk7dYs276IvBKX4FJjL0uns FM4vgdQCYELDF1ZV4o7MHJVoMABr7w/7cMUkm+CcH9kxx/7EVzduBAWsA0wXYhYko+vA 913x1Q8ySmlJuQZ6kotdp+DdJ0srH8IncA9PpglzNVprgSonzriBhPgDMRYfDZAJjEgQ OMHg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b15-20020a65668f000000b00553892614basi1600006pgw.620.2023.08.31.10.49.11; Thu, 31 Aug 2023 10:49:27 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245247AbjHaNDR convert rfc822-to-8bit (ORCPT + 99 others); Thu, 31 Aug 2023 09:03:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48454 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232438AbjHaNDQ (ORCPT ); Thu, 31 Aug 2023 09:03:16 -0400 Received: from mail-oo1-f48.google.com (mail-oo1-f48.google.com [209.85.161.48]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6DE20CFE; Thu, 31 Aug 2023 06:03:13 -0700 (PDT) Received: by mail-oo1-f48.google.com with SMTP id 006d021491bc7-5711d5dac14so182600eaf.0; Thu, 31 Aug 2023 06:03:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693486992; x=1694091792; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Y9jm6k3mZ7DtL6C9l6hYtOCCTfhPR3psQJlw1t5U+L8=; b=HqV91C8UZKr+2fcz8t3Ot1cXPuHmQoPeDMhP4uhkGUxqkYq+FAarJT/aLuQcug7A1h hx1Ekw1lfUGOiQPs/YPbvmka1092Ct99kioXrIrJiWb2vIMMw4jxTaBgL/ZUiHyc92Xo FXj4XWwRlmlyDk+YtR+TObUBijFYiXSHrQnW6U8q5670ftPGqiV1z2Xgre5VzMlrLkVv BbVNOYgw8/8CN8RwlzzxYQmmUIBUs7l/BwK3Oj86CS9DdM60BFbQvNSkxqvf1rqaFdHy yMXBnj/Z6hnlhsHDvv/7OLykdlKmt2Esy9lrbfd9SzMdciMFxACn98nCI8/3cjW0a9sp kBPw== X-Gm-Message-State: AOJu0YyeuTzhhDd6GReFhJl27jRnZchhhLacLNPJZlvN/FvkdfGrMq6+ /HmqTEw0I8/JsptZ7uLuRQiM4uEGhtvalKc0GIM= X-Received: by 2002:a4a:bd8f:0:b0:573:2a32:6567 with SMTP id k15-20020a4abd8f000000b005732a326567mr4905484oop.0.1693486992644; Thu, 31 Aug 2023 06:03:12 -0700 (PDT) MIME-Version: 1.0 References: <20230808111325.8600-1-TonyWWang-oc@zhaoxin.com> In-Reply-To: From: "Rafael J. Wysocki" Date: Thu, 31 Aug 2023 15:03:01 +0200 Message-ID: Subject: Re: [PATCH v2] cpufreq: ACPI: add ITMT support when CPPC enabled To: Tony W Wang-oc Cc: "Rafael J. Wysocki" , viresh.kumar@linaro.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, CobeChen@zhaoxin.com, TimGuo@zhaoxin.com, LeoLiu-oc@zhaoxin.com, LindaChai@zhaoxin.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT X-Spam-Status: No, score=-1.4 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 31, 2023 at 12:19 PM Tony W Wang-oc wrote: > > > On 8/23/23 04:01, Rafael J. Wysocki wrote: > > On Tue, Aug 8, 2023 at 1:13 PM Tony W Wang-oc wrote: > >> > >> The _CPC method can get per-core highest frequency. > > > > Well, not exactly. A more precise way to say this would be "The > > per-core highest frequency can be obtained via CPPC." > > > > Thanks for your reply, will rewrite the commit in next version. > > >> The highest frequency may varies between cores which mean cores can > > > > "may vary" and "which means" > > > >> running at different max frequency, so can use it as a core priority > > > > "can run", but it would be better to say "may run". > > > >> and give a hint to scheduler in order to put critical task to the > >> higher priority core. > > > > Well, roughly speaking ... > > > > You should really talk about ITMT and how it can be hooked up to this. > > > > Ok, Got it. > > >> Signed-off-by: Tony W Wang-oc > >> --- > >> v1->v2: Fix build errors reported by kernel test robot > >> > >> arch/x86/kernel/itmt.c | 2 ++ > >> drivers/cpufreq/acpi-cpufreq.c | 59 ++++++++++++++++++++++++++++++---- > >> 2 files changed, 54 insertions(+), 7 deletions(-) > >> > >> diff --git a/arch/x86/kernel/itmt.c b/arch/x86/kernel/itmt.c > >> index ee4fe8cdb857..b49ac8ecbbd6 100644 > >> --- a/arch/x86/kernel/itmt.c > >> +++ b/arch/x86/kernel/itmt.c > >> @@ -122,6 +122,7 @@ int sched_set_itmt_support(void) > >> > >> return 0; > >> } > >> +EXPORT_SYMBOL_GPL(sched_set_itmt_support); > > > > This requires an ACK from the x86 maintainers. > > > >> > >> /** > >> * sched_clear_itmt_support() - Revoke platform's support of ITMT > >> @@ -181,3 +182,4 @@ void sched_set_itmt_core_prio(int prio, int cpu) > >> { > >> per_cpu(sched_core_priority, cpu) = prio; > >> } > >> +EXPORT_SYMBOL_GPL(sched_set_itmt_core_prio); > > > > And same here. > > > >> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > >> index b2f05d27167e..5733323e04ac 100644 > >> --- a/drivers/cpufreq/acpi-cpufreq.c > >> +++ b/drivers/cpufreq/acpi-cpufreq.c > >> @@ -628,28 +628,35 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c) > >> #endif > >> > >> #ifdef CONFIG_ACPI_CPPC_LIB > >> -static u64 get_max_boost_ratio(unsigned int cpu) > >> +static void cpufreq_get_core_perf(int cpu, u64 *highest_perf, u64 *nominal_perf) > > > > This is not a cpufreq core function, so please use a different prefix > > in its name. > > > > Ok. Will remove the prefix of "cpufreq_". > > >> { > >> struct cppc_perf_caps perf_caps; > >> - u64 highest_perf, nominal_perf; > >> int ret; > >> > >> if (acpi_pstate_strict) > >> - return 0; > >> + return; > >> > >> ret = cppc_get_perf_caps(cpu, &perf_caps); > >> if (ret) { > >> pr_debug("CPU%d: Unable to get performance capabilities (%d)\n", > >> cpu, ret); > >> - return 0; > >> + return; > >> } > >> > >> if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) > >> - highest_perf = amd_get_highest_perf(); > >> + *highest_perf = amd_get_highest_perf(); > >> else > >> - highest_perf = perf_caps.highest_perf; > >> + *highest_perf = perf_caps.highest_perf; > >> + > >> + *nominal_perf = perf_caps.nominal_perf; > >> + return; > >> +} > >> > >> - nominal_perf = perf_caps.nominal_perf; > >> +static u64 get_max_boost_ratio(unsigned int cpu) > >> +{ > >> + u64 highest_perf, nominal_perf; > >> + > >> + cpufreq_get_core_perf(cpu, &highest_perf, &nominal_perf); > >> > >> if (!highest_perf || !nominal_perf) { > >> pr_debug("CPU%d: highest or nominal performance missing\n", cpu); > >> @@ -663,8 +670,44 @@ static u64 get_max_boost_ratio(unsigned int cpu) > >> > >> return div_u64(highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf); > >> } > >> + > >> +static void cpufreq_sched_itmt_work_fn(struct work_struct *work) > > > > A similar comment applies here. > > > >> +{ > >> + sched_set_itmt_support(); > >> +} > >> + > >> +static DECLARE_WORK(sched_itmt_work, cpufreq_sched_itmt_work_fn); > >> + > >> +static void cpufreq_set_itmt_prio(int cpu) > >> +{ > >> + u64 highest_perf, nominal_perf; > >> + static u32 max_highest_perf = 0, min_highest_perf = U32_MAX; > >> + > >> + cpufreq_get_core_perf(cpu, &highest_perf, &nominal_perf); > >> + > >> + sched_set_itmt_core_prio(highest_perf, cpu); > >> + > >> + if (max_highest_perf <= min_highest_perf) { > >> + if (highest_perf > max_highest_perf) > >> + max_highest_perf = highest_perf; > >> + > >> + if (highest_perf < min_highest_perf) > >> + min_highest_perf = highest_perf; > >> + > >> + if (max_highest_perf > min_highest_perf) { > >> + /* > >> + * This code can be run during CPU online under the > >> + * CPU hotplug locks, so sched_set_itmt_support() > >> + * cannot be called from here. Queue up a work item > >> + * to invoke it. > >> + */ > >> + schedule_work(&sched_itmt_work); > >> + } > > > > This potentially runs before ITMT priorities are set for all CPUs. > > Isn't it a problem? > > > > Yes, you are right. > Will use schedule_delayed_work(&sched_itmt_work, msecs_to_jiffies(500)) > to fix this. If the ordering matters, it is better to enforce it directly (through an explicit code dependency, for example) than to rely on the timing to do the right thing. If you do the above, then it will not be clear why it is done (a comment may help to address that, though) and why the delay is 500 us in particular.