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;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.160 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.160; helo=mail.nvidia.com; pr=C
Message-ID: <0d0ce727-d801-1c63-253d-815f324fac24@nvidia.com>
Date:   Wed, 26 Apr 2023 21:22:39 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [Patch 6/6] cpufreq: CPPC: use wq to read amu counters on target
 cpu
Content-Language: en-US
To:     Ionela Voinescu <ionela.voinescu@arm.com>
CC:     <viresh.kumar@linaro.org>, <rafael@kernel.org>,
        <mark.rutland@arm.com>, <sudeep.holla@arm.com>,
        <lpieralisi@kernel.org>, <catalin.marinas@arm.com>,
        <will@kernel.org>, <linux-pm@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-tegra@vger.kernel.org>,
        <treding@nvidia.com>, <jonathanh@nvidia.com>, <vsethi@nvidia.com>,
        <sdonthineni@nvidia.com>, <sanjayc@nvidia.com>,
        <ksitaraman@nvidia.com>, <bbasu@nvidia.com>,
        Sumit Gupta <sumitg@nvidia.com>
References: <20230418113459.12860-1-sumitg@nvidia.com>
 <20230418113459.12860-7-sumitg@nvidia.com> <ZEY+kENTxCvHlhvK@arm.com>
From:   Sumit Gupta <sumitg@nvidia.com>
In-Reply-To: <ZEY+kENTxCvHlhvK@arm.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2023 15:52:58.9941
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: b7633815-4c6c-474d-cfc5-08db466e4f26
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.160];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT003.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB7431
X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham
        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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Ionela,

Thankyou for the inputs.

On 24/04/23 14:02, Ionela Voinescu wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hi Sumit,
> 
> Thank you for the patches!
> 
> On Tuesday 18 Apr 2023 at 17:04:59 (+0530), Sumit Gupta wrote:
>> ARM cores which implement the Activity Monitor Unit (AMU)
>> use Functional Fixed Hardware (FFH) to map AMU counters to
>> Delivered_Counter and Reference_Counter registers. Each
>> sysreg is read separately with a smp_call_function_single
>> call. So, total four IPI's are used, one per register.
>> Due to this, the AMU's core counter and constant counter
>> sampling can happen at a non-consistent time interval if
>> an IPI is handled late. This results in unstable frequency
>> value from "cpuinfo_cur_req" node sometimes. To fix, queue
>> work on target CPU to read all counters synchronously in
>> sequence. This helps to remove the inter-IPI latency and
>> make sure that both the counters are sampled at a close
>> time interval.
>> Without this change we observed that the re-generated value
>> of CPU Frequency from AMU counters sometimes deviates by
>> ~25% as the counters are read at non-determenistic time.
>> Currently, kept the change specific to Tegra241. It can be
>> applied to other SoC's having AMU if same issue is observed.
>>
> 
> To be honest I never liked the need for IPIs but it was the most
> generic solution I could find for an FFH implementation that does not
> assume a dependency between different reads, which is usecase specific.
> 
> Also, for any kind of caching of the counters I'd have to introduce some
> logic that would assume we'd always have two consecutive reads - one for
> the cycle counter and one for the constant counter, and there should be no
> update between them. And then there's the problem of potentially returning
> the same values if there's no update between two sets of reads.
> 
> The only feasible idea based on caching would be to piggy back on the
> frequency invariance engine (FIE) which computes a performance scale
> factor on the tick which can be translated to frequency. But the
> frequency obtained would be an average frequency for the past 4ms, which
> can in turn be at up to 4ms old (or more, if the CPU was idle).
> 
> Would something like this work for you?
> 
> This could also help with a similar issue described at [1] - not an IPI
> related issue, but an issue with similar symptoms.
> 
> [1] https://lore.kernel.org/lkml/20230328193846.8757-1-yang@os.amperecomputing.com/
> 
> Thanks,
> Ionela.
> 

I think yes that will help as it will increase the time period and also
remove the IPI's ?

One thing I am not sure is whether there can be any impact when CPU is
IDLE w.r.t. the delta between the frequency set and get from the counters.

Thank you,
Sumit Gupta

>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 53 +++++++++++++++++++++++++++-------
>>   1 file changed, 43 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 5e6a132a525e..52b93ac6225e 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -46,6 +46,8 @@ static bool boost_supported;
>>   /* default 2usec delay between sampling */
>>   static unsigned int sampling_delay_us = 2;
>>
>> +static bool get_rate_use_wq;
>> +
>>   static void cppc_check_hisi_workaround(void);
>>   static void cppc_nvidia_workaround(void);
>>
>> @@ -99,6 +101,12 @@ struct cppc_freq_invariance {
>>   static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
>>   static struct kthread_worker *kworker_fie;
>>
>> +struct feedback_ctrs {
>> +     u32 cpu;
>> +     struct cppc_perf_fb_ctrs fb_ctrs_t0;
>> +     struct cppc_perf_fb_ctrs fb_ctrs_t1;
>> +};
>> +
>>   static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu);
>>   static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>                                 struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>> @@ -851,28 +859,44 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>        return (reference_perf * delta_delivered) / delta_reference;
>>   }
>>
>> +static int cppc_get_perf_ctrs_sync(void *fb_ctrs)
>> +{
>> +     struct feedback_ctrs *ctrs = fb_ctrs;
>> +     int ret;
>> +
>> +     ret = cppc_get_perf_ctrs(ctrs->cpu, &(ctrs->fb_ctrs_t0));
>> +     if (ret)
>> +             return ret;
>> +
>> +     udelay(sampling_delay_us);
>> +
>> +     ret = cppc_get_perf_ctrs(ctrs->cpu, &(ctrs->fb_ctrs_t1));
>> +     if (ret)
>> +             return ret;
>> +
>> +     return ret;
>> +}
>> +
>>   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 = policy->driver_data;
>> +     struct feedback_ctrs fb_ctrs = {0};
>>        u64 delivered_perf;
>>        int ret;
>>
>>        cpufreq_cpu_put(policy);
>> +     fb_ctrs.cpu = cpu;
>>
>> -     ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>> -     if (ret)
>> -             return ret;
>> -
>> -     udelay(sampling_delay_us);
>> -
>> -     ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>> +     if (get_rate_use_wq)
>> +             ret = smp_call_on_cpu(cpu, cppc_get_perf_ctrs_sync, &fb_ctrs, false);
>> +     else
>> +             ret = cppc_get_perf_ctrs_sync(&fb_ctrs);
>>        if (ret)
>>                return ret;
>>
>> -     delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>> -                                            &fb_ctrs_t1);
>> +     delivered_perf = cppc_perf_from_fbctrs(cpu_data, &(fb_ctrs.fb_ctrs_t0),
>> +                                            &(fb_ctrs.fb_ctrs_t1));
>>
>>        return cppc_cpufreq_perf_to_khz(cpu_data, delivered_perf);
>>   }
>> @@ -953,7 +977,16 @@ static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
>>
>>   static void cppc_nvidia_workaround(void)
>>   {
>> +     int cpu;
>> +
>>        sampling_delay_us = 25;
>> +
>> +#ifdef CONFIG_ARM64_AMU_EXTN
>> +     cpu = get_cpu_with_amu_feat();
>> +
>> +     if (cpu < nr_cpu_ids)
>> +             get_rate_use_wq = true;
>> +#endif
>>   }
>>
>>   static void cppc_check_hisi_workaround(void)
>> --
>> 2.17.1
>>