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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 49A2CC3600A for ; Tue, 17 Sep 2024 10:39:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=VfYuxMif3rsMwUpaJiAzeGRKM2MKLdo5kAQeOyPOkt4=; b=VzS5ius/uTotGZaItZBJ7R4Ucu hvouUGQN+BXZIRiYWTLIgv/V9ZXuEDr+v22Mv/2x3sBGwgPUiXrAPBGOm8Aum8UkMqs1sBAAuPRZ9 J8p+LFr6BY7tg9x5hHAlq5v1gXZBqd57K5A87XLwXaj37FcpVVZeQecdECcrTmQlwnJmhoCh2oPwm PLnnMoiYlX23io9QJNoklkFi4EuZNdu8gQpOrH9bPJwOclMgwfRD015HTFfG5jFUcCBwULdpaMGod yvs3TdQDtTxXPmcpdzfmq38V2HgTRGY02DvWaz2CyqF7fBG5pI/4aQWQZ5PNK3d2kibfXyRjFqRBS /yCGldcg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1sqVbq-00000005yE5-0mwe; Tue, 17 Sep 2024 10:39:02 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1sqVZb-00000005xhu-2YOj for linux-arm-kernel@lists.infradead.org; Tue, 17 Sep 2024 10:36:50 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D82841007; Tue, 17 Sep 2024 03:37:08 -0700 (PDT) Received: from localhost (ionvoi01-desktop.cambridge.arm.com [10.2.80.58]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1A96B3F64C; Tue, 17 Sep 2024 03:36:39 -0700 (PDT) Date: Tue, 17 Sep 2024 11:36:37 +0100 From: Ionela Voinescu To: Jie Zhan Cc: beata.michalska@arm.com, wangxiongfeng2@huawei.com, viresh.kumar@linaro.org, rafael@kernel.org, linux-pm@vger.kernel.org, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linuxarm@huawei.com, jonathan.cameron@huawei.com, wanghuiqiang@huawei.com, zhenglifeng1@huawei.com, lihuisong@huawei.com, yangyicong@huawei.com, liaochang1@huawei.com, zengheng4@huawei.com Subject: Re: [PATCH v2 1/3] cppc_cpufreq: Return desired perf in ->get() if feedback counters are 0 Message-ID: References: <20240912072231.439332-1-zhanjie9@hisilicon.com> <20240912072231.439332-2-zhanjie9@hisilicon.com> <79353a26-7304-9d6a-9237-cfa8e7794601@hisilicon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <79353a26-7304-9d6a-9237-cfa8e7794601@hisilicon.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240917_033643_794598_3C9E545D X-CRM114-Status: GOOD ( 33.03 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, On Friday 13 Sep 2024 at 20:05:50 (+0800), Jie Zhan wrote: > > Hi Ionela, > > On 12/09/2024 17:43, Ionela Voinescu wrote: > > ... > > > > > A possible (slimmer) alternative implementation for you to consider > > (this merges patches 1 & 2): > > > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > > index bafa32dd375d..c16be9651a6f 100644 > > --- a/drivers/cpufreq/cppc_cpufreq.c > > +++ b/drivers/cpufreq/cppc_cpufreq.c > > @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work) > > > > perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs, > > &fb_ctrs); > > + if (!perf) > > + perf = cpu_data->perf_ctrls.desired_perf; > > + > > I think it's better to just return here. > If feedback counters are successfully read but unchanged, the following > calculation and update in cppc_scale_freq_workfn() is meaningless because it > won't change anything. Agreed! > > > cppc_fi->prev_perf_fb_ctrs = fb_ctrs; > > > > perf <<= SCHED_CAPACITY_SHIFT; > > @@ -726,7 +729,7 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data, > > > > /* Check to avoid divide-by zero and invalid delivered_perf */ > > if (!delta_reference || !delta_delivered) > > - return cpu_data->perf_ctrls.desired_perf; > > + return 0; > > This makes sense to me. > Here is probably why Patch 2 looks bulky. > > > > > return (reference_perf * delta_delivered) / delta_reference; > > } > > @@ -736,7 +739,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > > struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0}; > > struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); > > struct cppc_cpudata *cpu_data; > > - u64 delivered_perf; > > + u64 delivered_perf = 0; > > int ret; > > > > if (!policy) > > @@ -747,19 +750,22 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > > cpufreq_cpu_put(policy); > > > > ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); > > - if (ret) > > - return 0; > > - > > - udelay(2); /* 2usec delay between sampling */ > > - > > - ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); > > - if (ret) > > - return 0; > > + if (!ret) { > > + udelay(2); /* 2usec delay between sampling */ > > + ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); > > + } > > + if (!ret) > > + delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, > > + &fb_ctrs_t1); > > TBH, 'if (!ret)' style looks very strange to me. > We haven't done so anywhere in cppc_cpufreq, so let's keep consistency and make > it easier for people to read and maintain? I agree it's a bit of a difficult read, that's why I only sent my code as a suggestion. I did like the benefit of not having to have two different calls to cppc_perf_to_khz() and making the code below common for the error and non-error paths. But it's up to you. > > > + if ((ret == -EFAULT) || !delivered_perf) { > > + if (cppc_get_desired_perf(cpu, &delivered_perf)) > > + delivered_perf = cpu_data->perf_ctrls.desired_perf; > > will take this. > > > + } > > > > - delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, > > - &fb_ctrs_t1); > > + if (delivered_perf) > > + return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); > > > > - return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); > > + return 0; > > } > > > > disclaimer: not fully checked so likely not "production ready" code :) > > > > Hope it helps, > > Ionela. > > > >> > >> static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) > >> -- > >> 2.33.0 > >> > > > > How about this? merged patch 1 & 2 as well. > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > index bafa32dd375d..411303f2e8cb 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work) > > perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs, > &fb_ctrs); > + if (!perf) > + return; > + > cppc_fi->prev_perf_fb_ctrs = fb_ctrs; > > perf <<= SCHED_CAPACITY_SHIFT; > @@ -726,7 +729,7 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data, > > /* Check to avoid divide-by zero and invalid delivered_perf */ > if (!delta_reference || !delta_delivered) > - return cpu_data->perf_ctrls.desired_perf; > + return 0; > > return (reference_perf * delta_delivered) / delta_reference; > } > @@ -748,18 +751,32 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > > ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); > if (ret) > - return 0; > + goto out_err; > > udelay(2); /* 2usec delay between sampling */ > > ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); > if (ret) > - return 0; > + goto out_err; > > delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, > &fb_ctrs_t1); You need a check here for !delivered_perf (when at least one of the deltas is 0) in which case it would be good to take the same error path below. Something like: if(delivered_perf) return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); else ret = -EFAULT; That's why I did the tricky if/else dance above as we need to take the error path below for multiple cases. Thanks, Ionela. > > return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); > + > +out_err: > + /* > + * Feedback counters could be 0 when cores are powered down. > + * Take desired perf for reflecting frequency in this case. > + */ > + if (ret == -EFAULT) { > + if (cppc_get_desired_perf(cpu, &delivered_perf)) > + delivered_perf = cpu_data->perf_ctrls.desired_perf; > + > + return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); > + } > + > + return 0; > } > > static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) > --- > > Thanks indeed! > Jie