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 A68F7C7EE2E for ; Thu, 8 Jun 2023 05:18:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=YszMUDWLoh1BzuTCqYctCpVMbqTXHa8uNd6zt9cYcdA=; b=bF3kJ/n8KEV3DJ C6o+8XKJuK50bpxjfK4AixkklevEJwggr8q9UFNwvisO/j7owmKHgjTWZ9M3wunwctASzQS9q6pnx SFaJGRbWNIeikdNtFWPUDlErBh0F7ffDe7si/LeB6t60htgF0ormgqvAlv3PFK0TiL5PBp2VC2bKG FDSypeicuf2qX39ub+K4IG3fg6zs7NjZQhpaxiB3LVuj7SlR46S4FDaxbhhqPzUYQXNsTGyZAFoRI TiqxdmVIYS6LakBnKPUIw9kJXLRKr50N9jx+CLVZcAI6uRsteo3Sp446QILMivw/n5MIARAJ9bgLV U4fhgF4uIRs1XND0ooVw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q782T-0088DD-2K; Thu, 08 Jun 2023 05:18:25 +0000 Received: from mail-oi1-x22c.google.com ([2607:f8b0:4864:20::22c]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1q782Q-0088CP-1W for linux-arm-kernel@lists.infradead.org; Thu, 08 Jun 2023 05:18:23 +0000 Received: by mail-oi1-x22c.google.com with SMTP id 5614622812f47-39a3f2668d5so166424b6e.1 for ; Wed, 07 Jun 2023 22:18:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1686201499; x=1688793499; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=cero/nNmUyyTgbddJ18ROOGs9bd2i20p7IPIh5GwALY=; b=sJAzPiDAKc2OSCHZVzx3H0wpsmp/FJHYaIIEZAgwY1kpL4alxyP3pc7oBnw4k/dz+V NWTEyv1Jy+Et3u3kOB0JZexCi/0r6WUvAE4vlpfSrCJzOvvgvYuf7MURiLKlL998U8LB PykSgw36P4KLWwnlUwneNMoJDVqyQQFrubVe7J1nW1hODvl/+qOKlMuVaUcowBj3o7SO b3MyLJprq2jFSmqH5p8zgE+8vrf7TLtg1T4I2POirHBL/pOvtLavAFZPVQUGM0LHWzRP Fhww+p+MbEl4yBZBYE2zxtLh3R6821S3ZW0x81figdxQbBPJknY6vW54gsQ80khr3gEa ADDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686201499; x=1688793499; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=cero/nNmUyyTgbddJ18ROOGs9bd2i20p7IPIh5GwALY=; b=a7I/6infXlcFM9AGEJ0AlH7AGlMpunug09ngUYNDHhDtrAMnyOneWdMoSNncS2a5nC 5j/NvPE7AWj3Lzq/XEjXdErvl/UQ5F1KyOnnUuvVjkKz9JwGA+SayisPcU3C7MyQXTwA 1TfhuZo6vuoU+iJCzFr9mADBGg+7ApWHfEf+IDrIj2ut1olHKOU8FqJA/NY/Z2ZFSmQW CNsfzymPXEPFTFOyzc9HrthPZkxe4Swz3GXjbwVNemQn5TPb0THAlqkY3GuEZxc8RKHB 6La71BDfyN8qKU5d4AFO9mx1kuTUE2vWS022iGwT9QNwQlvoh64samVD+Zc2HUzuD7gu yvxQ== X-Gm-Message-State: AC+VfDzspgsKNPlJmoELbM9byrD8hHCyuVG1gL8U8wweFhV/WMxIPFMz Dbxt+zVP6zha0T3vFItbF941tA== X-Google-Smtp-Source: ACHHUZ5rn21cbWOU6lHWA9Ma4Ci1eR/2PI2FFfCu7VnVX/7t5MXa1P6VbXK9vLrFVhvjiq4AU48z4Q== X-Received: by 2002:a05:6808:a96:b0:39a:babe:a7e with SMTP id q22-20020a0568080a9600b0039ababe0a7emr4581980oij.35.1686201499410; Wed, 07 Jun 2023 22:18:19 -0700 (PDT) Received: from localhost ([122.172.87.195]) by smtp.gmail.com with ESMTPSA id m4-20020a17090a71c400b0025671de4606sm2192660pjs.4.2023.06.07.22.18.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Jun 2023 22:18:18 -0700 (PDT) Date: Thu, 8 Jun 2023 10:48:16 +0530 From: Viresh Kumar To: Beata Michalska Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, catalin.marinas@arm.com, mark.rutland@arm.com, will@kernel.org, rafael@kernel.org, sudeep.holla@arm.com, ionela.voinescu@arm.com, sumitg@nvidia.com, yang@os.amperecomputing.com, Len Brown , vincent.guittot@linaro.org Subject: Re: [PATCH] arm64: Provide an AMU-based version of arch_freq_get_on_cpu Message-ID: <20230608051816.2ww7ncg65qo7kcuk@vireshk-i7> References: <20230606155754.245998-1-beata.michalska@arm.com> <20230608051509.h4a6gn572mjgdusv@vireshk-i7> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230608051509.h4a6gn572mjgdusv@vireshk-i7> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230607_221822_523805_5B55CD70 X-CRM114-Status: GOOD ( 31.32 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org +Vincent On 08-06-23, 10:45, Viresh Kumar wrote: > +Len > > On 06-06-23, 16:57, Beata Michalska wrote: > > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > > +unsigned int arch_freq_get_on_cpu(int cpu) > > +{ > > + unsigned int freq; > > + u64 scale; > > + > > + if (!cpumask_test_cpu(cpu, amu_fie_cpus)) > > + return 0; > > + > > + if (!housekeeping_cpu(cpu, HK_TYPE_TICK)) { > > I am not sure what we are doing in the `if` block here, at least a comment would > be useful. > > > + struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); > > + int ref_cpu = nr_cpu_ids; > > + > > + if (cpumask_intersects(housekeeping_cpumask(HK_TYPE_TICK), > > + policy->cpus)) > > + ref_cpu = cpumask_nth_and(cpu, policy->cpus, > > + housekeeping_cpumask(HK_TYPE_TICK)); > > + cpufreq_cpu_put(policy); > > + if (ref_cpu >= nr_cpu_ids) > > + return 0; > > + cpu = ref_cpu; > > + } > > A blank line here please. > > > + /* > > + * Reversed computation to the one used to determine > > + * the arch_freq_scale value > > + * (see amu_scale_freq_tick for details) > > + */ > > + scale = per_cpu(arch_freq_scale, cpu); > > + scale *= cpufreq_get_hw_max_freq(cpu); > > + freq = scale >> SCHED_CAPACITY_SHIFT; > > + > > + return freq; > > +} > > + > > #ifdef CONFIG_ACPI_CPPC_LIB > > #include > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 6b52ebe5a890..9f2cf45bf190 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -710,7 +710,8 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf) > > ssize_t ret; > > unsigned int freq; > > > > - freq = arch_freq_get_on_cpu(policy->cpu); > > + freq = !cpufreq_driver->get ? arch_freq_get_on_cpu(policy->cpu) > > + : 0; > > You may have changed the logic for X86 parts as well here. For a x86 platform > with setpolicy() and get() callbacks, we will not call arch_freq_get_on_cpu() > anymore ? > > > if (freq) > > ret = sprintf(buf, "%u\n", freq); > > else if (cpufreq_driver->setpolicy && cpufreq_driver->get) > > @@ -747,7 +748,11 @@ store_one(scaling_max_freq, max); > > static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy, > > char *buf) > > { > > - unsigned int cur_freq = __cpufreq_get(policy); > > + unsigned int cur_freq; > > + > > + cur_freq = arch_freq_get_on_cpu(policy->cpu); > > + if (!cur_freq) > > + cur_freq = __cpufreq_get(policy); > > For this and the above change, I am not sure what is the right thing to do. > > >From Len's commit [1]: > > Here we provide an x86 routine to make this calculation > on supported hardware, and use it in preference to any > driver driver-specific cpufreq_driver.get() routine. > > I am not sure why Len updated `show_scaling_cur_freq()` and not > `show_cpuinfo_cur_freq()` ? Maybe we should update both these routines ? > > Also, I don't think this is something that should have different logic for ARM > and X86, we should be consistent here as a cpufreq decision. Since both these > routines are reached via a read operation to a sysfs file, we shouldn't be > concerned about performance too. > > What about doing this for both the routines, for all platforms now: > > cur_freq = arch_freq_get_on_cpu(policy->cpu); > if (!cur_freq) > ... get freq via policy->get() or policy->cur; > > -- > viresh > > [1] commit f8475cef9008 ("x86: use common aperfmperf_khz_on_cpu() to calculate KHz using APERF/MPERF") -- viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel