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.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
Message-ID: <691d3eb2-cd93-f0fc-a7a4-2a8c0d44262c@nvidia.com>
Date:   Fri, 23 Jun 2023 20:03:56 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] arm64: Provide an AMU-based version of
 arch_freq_get_on_cpu
To:     Beata Michalska <beata.michalska@arm.com>
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>,
        <viresh.kumar@linaro.org>, <sudeep.holla@arm.com>,
        <ionela.voinescu@arm.com>, <yang@os.amperecomputing.com>,
        <linux-tegra@vger.kernel.org>
References: <20230606155754.245998-1-beata.michalska@arm.com>
 <8e755438-4b1f-b3d6-b2b8-a5efcca813bc@nvidia.com>
 <ZIwxB5ao96pVPaCc@e120325.cambridge.arm.com>
Content-Language: en-US
From:   Sumit Gupta <sumitg@nvidia.com>
In-Reply-To: <ZIwxB5ao96pVPaCc@e120325.cambridge.arm.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jun 2023 14:34:15.7626
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 7bfe2e87-560d-4edf-eacf-08db73f6ebe1
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.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT054.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB6389
X-Spam-Status: No, score=-1.2 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=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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 16/06/23 15:23, Beata Michalska wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Jun 15, 2023 at 12:29:57AM +0530, Sumit Gupta wrote:
>>
>>
>> On 06/06/23 21:27, Beata Michalska wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> With the Frequency Invariance Engine (FIE) being already wired up with
>>> sched tick and making use of relevant (core counter and constant
>>> counter) AMU counters, getting the current frequency for a given CPU
>>> on supported platforms, can be achieved by utilizing the frequency scale
>>> factor which reflects an average CPU frequency for the last tick period
>>> length.
>>>
>>> With that at hand, arch_freq_get_on_cpu dedicated implementation
>>> gets enrolled into cpuinfo_cur_freq policy sysfs attribute handler,
>>> which is expected to represent the current frequency of a given CPU,
>>> as obtained by the hardware. This is exactly the type of feedback that
>>> cycle counters provide.
>>>
>>> In order to avoid calling arch_freq_get_on_cpu from the scaling_cur_freq
>>> attribute handler for platforms that do provide cpuinfo_cur_freq, and
>>> yet keeping things intact for those platform that do not, its use gets
>>> conditioned on the presence of cpufreq_driver (*get) callback (which also
>>> seems to be the case for creating cpuinfo_cur_freq attribute).
>>>
>>
>> Tested the change with frequency switch stress test but was getting big
>> delta between set and get freq.
> Would you mind sharing some more data re your testing ?
> The arch_freq_get_on_cpu will provided an average freq for last tick period,
> with an updated occurring on each sched tick so the differences between set
> and get might show up. With your stress testing, if the frequency change comes
> at the end of current tick period, it might not be reflected until next one
> elapses.
> In case of idle states - if the CPU for which the current frequency is being
> requested is in idle mode, the frequency returned will be the last one before
> entering idle, which seems reasonable (?).
> I guess the question here would be what is your tolerance level for those
> differences.

The test waits for 50ms before reading back the set frequency which is
much more than a tick period.

High delta value might be due to the reference counter increments
happening in bursts in Tegra241 SoC as mentioned in [1].
If the CPU is idle for most of the period then observation window will
be small due to AMU counters stopping at WFI. This can cause more error
in Tegra241 as increasing the observation time interval reduces error
percentage.

I created 100% CPU load and then the test is passing. So, looks like we 
will have to create load before the frequency switch test. Please share
if more suggestions.
[1] https://lore.kernel.org/lkml/20230418113459.12860-5-sumitg@nvidia.com/

>> After passing "nohz=off" and commenting "wfi" in "cpu_do_idle()", the
>> delta is less. This confirms that more delta is due to AMU counters
>> stopping at "WFI".
>>
>>    +++ b/arch/arm64/kernel/idle.c
>>    @@ -27,7 +27,7 @@ void noinstr cpu_do_idle(void)
>>            arm_cpuidle_save_irq_context(&context);
>>
>>            dsb(sy);
>>    -       wfi();
>>    +//     wfi();
>>
>> I am not sure if the expected behavior here is right.
> Both CPU_CYCLES and CNT_CYCLES are not incremented in WFI.
>> In our tests, we compare the last set frequency against the re-generated
>> value from counters to confirm that the CPU is actually running at the
>> requested frequency and the counters are working correct. But that won't
>> happen with this change.
>>
>> In [1] and later in the updated patch within [2], we are busy looping
>> on the target CPU and avoid WFI to get the actual frequency.
>>
>> Please share what you think is the right expected behavior.
>>
>> [1] https://lore.kernel.org/lkml/20230418113459.12860-7-sumitg@nvidia.com/
>> [2] https://lore.kernel.org/lkml/cde1d8a9-3a21-e82b-7895-40603a14d898@nvidia.com/T/#mb898a75fd0c72d166b26b04da3ad162afe068a82