All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sumit Gupta <sumitg@nvidia.com>
To: Ionela Voinescu <ionela.voinescu@arm.com>,
	Beata Michalska <beata.michalska@arm.com>
Cc: <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-pm@vger.kernel.org>, <sudeep.holla@arm.covm>,
	<will@kernel.org>, <catalin.marinas@arm.com>,
	<viresh.kumar@linaro.org>, <rafael@kernel.org>,
	<yang@os.amperecomputing.com>, <linux-tegra@vger.kernel.org>,
	Sumit Gupta <sumitg@nvidia.com>
Subject: Re: [PATCH v2 2/2] cpufreq: Wire-up arch-flavored freq info into cpufreq_verify_current_freq
Date: Fri, 1 Dec 2023 18:32:10 +0530	[thread overview]
Message-ID: <3e6077bb-907c-057f-0896-d0a5814a4229@nvidia.com> (raw)
In-Reply-To: <ZWXy0h/fFfQh+Rhy@arm.com>

Hi Ionela,

>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1756,7 +1756,8 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
>>   {
>>        unsigned int new_freq;
>>
>> -     new_freq = cpufreq_driver->get(policy->cpu);
>> +     new_freq = arch_freq_get_on_cpu(policy->cpu);
>> +     new_freq = new_freq ?: cpufreq_driver->get(policy->cpu);
> 
> Given that arch_freq_get_on_cpu() is an average frequency, it does not
> seem right to me to trigger the sync & update process of
> cpufreq_verify_current_freq() based on it.
> 
> cpufreq_verify_current_freq() will at least modify the internal state of
> the policy and send PRE and POST notifications, if not do a full frequency
> update, based on this average frequency, which is likely different from
> the current frequency, even beyond the 1MHz threshold.
> 
> While I believe it's okay to return this average frequency in
> cpuinfo_cur_freq, I don't think it should be used as an indication of
> an accurate current frequency, which is what
> cpufreq_verify_current_freq() expects.
> 
> Sumit, can you give more details on the issue at [1] and why this change
> fixes it?
> 
> [1] https://lore.kernel.org/lkml/6a5710f6-bfbb-5dfd-11cd-0cd02220cee7@nvidia.com/
> 
> Thank you,
> Ionela.
>
cpufreq_verify_current_freq() also updates 'policy->cur' in POST
notification if the frequency from hardware has more delta (out of sync).

As the value from 'cpufreq_driver->get()' is not reliable due to [1],
calling the 'get' hook can update the 'policy->cur' with a wrong value 
when governor starts in cpufreq_start_governor().
And if the frequency is never changed after the governor starts during
boot e.g. when performance governor is set as default, then 
'scaling_cur_freq' always returns wrong value.

Instead, the arch_freq_get_on_cpu() API updates 'policy->cur' with a 
more stable freq value.

[1] https://lore.kernel.org/lkml/20230418113459.12860-7-sumitg@nvidia.com/

Best regards,
Sumit Gupta

>>        if (!new_freq)
>>                return 0;
>>
>> --
>> 2.25.1
>>

WARNING: multiple messages have this Message-ID (diff)
From: Sumit Gupta <sumitg@nvidia.com>
To: Ionela Voinescu <ionela.voinescu@arm.com>,
	Beata Michalska <beata.michalska@arm.com>
Cc: <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-pm@vger.kernel.org>, <sudeep.holla@arm.covm>,
	<will@kernel.org>, <catalin.marinas@arm.com>,
	<viresh.kumar@linaro.org>, <rafael@kernel.org>,
	<yang@os.amperecomputing.com>, <linux-tegra@vger.kernel.org>,
	Sumit Gupta <sumitg@nvidia.com>
Subject: Re: [PATCH v2 2/2] cpufreq: Wire-up arch-flavored freq info into cpufreq_verify_current_freq
Date: Fri, 1 Dec 2023 18:32:10 +0530	[thread overview]
Message-ID: <3e6077bb-907c-057f-0896-d0a5814a4229@nvidia.com> (raw)
In-Reply-To: <ZWXy0h/fFfQh+Rhy@arm.com>

Hi Ionela,

>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1756,7 +1756,8 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
>>   {
>>        unsigned int new_freq;
>>
>> -     new_freq = cpufreq_driver->get(policy->cpu);
>> +     new_freq = arch_freq_get_on_cpu(policy->cpu);
>> +     new_freq = new_freq ?: cpufreq_driver->get(policy->cpu);
> 
> Given that arch_freq_get_on_cpu() is an average frequency, it does not
> seem right to me to trigger the sync & update process of
> cpufreq_verify_current_freq() based on it.
> 
> cpufreq_verify_current_freq() will at least modify the internal state of
> the policy and send PRE and POST notifications, if not do a full frequency
> update, based on this average frequency, which is likely different from
> the current frequency, even beyond the 1MHz threshold.
> 
> While I believe it's okay to return this average frequency in
> cpuinfo_cur_freq, I don't think it should be used as an indication of
> an accurate current frequency, which is what
> cpufreq_verify_current_freq() expects.
> 
> Sumit, can you give more details on the issue at [1] and why this change
> fixes it?
> 
> [1] https://lore.kernel.org/lkml/6a5710f6-bfbb-5dfd-11cd-0cd02220cee7@nvidia.com/
> 
> Thank you,
> Ionela.
>
cpufreq_verify_current_freq() also updates 'policy->cur' in POST
notification if the frequency from hardware has more delta (out of sync).

As the value from 'cpufreq_driver->get()' is not reliable due to [1],
calling the 'get' hook can update the 'policy->cur' with a wrong value 
when governor starts in cpufreq_start_governor().
And if the frequency is never changed after the governor starts during
boot e.g. when performance governor is set as default, then 
'scaling_cur_freq' always returns wrong value.

Instead, the arch_freq_get_on_cpu() API updates 'policy->cur' with a 
more stable freq value.

[1] https://lore.kernel.org/lkml/20230418113459.12860-7-sumitg@nvidia.com/

Best regards,
Sumit Gupta

>>        if (!new_freq)
>>                return 0;
>>
>> --
>> 2.25.1
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2023-12-01 13:02 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-27 16:08 [PATCH v2 0/2] Add support for AArch64 AMUv1-based arch_freq_get_on_cpu Beata Michalska
2023-11-27 16:08 ` Beata Michalska
2023-11-27 16:08 ` [PATCH v2 1/2] arm64: Provide an AMU-based version of arch_freq_get_on_cpu Beata Michalska
2023-11-27 16:08   ` Beata Michalska
2023-11-28 15:13   ` Ionela Voinescu
2023-11-28 15:13     ` Ionela Voinescu
2024-02-02  9:20     ` Beata Michalska
2024-02-02  9:20       ` Beata Michalska
2024-02-22 19:55   ` Vanshidhar Konda
2024-02-22 19:55     ` Vanshidhar Konda
2023-11-27 16:08 ` [PATCH v2 2/2] cpufreq: Wire-up arch-flavored freq info into cpufreq_verify_current_freq Beata Michalska
2023-11-27 16:08   ` Beata Michalska
2023-11-28 14:01   ` Ionela Voinescu
2023-11-28 14:01     ` Ionela Voinescu
2023-12-01 13:02     ` Sumit Gupta [this message]
2023-12-01 13:02       ` Sumit Gupta
2023-12-05 11:05       ` Ionela Voinescu
2023-12-05 11:05         ` Ionela Voinescu
2023-12-06 13:28         ` Sumit Gupta
2023-12-06 13:28           ` Sumit Gupta
2023-12-07  9:22           ` Ionela Voinescu
2023-12-07  9:22             ` Ionela Voinescu
2023-12-08 15:34             ` Sumit Gupta
2023-12-08 15:34               ` Sumit Gupta
2024-02-02  9:14     ` Beata Michalska
2024-02-02  9:14       ` Beata Michalska
2023-12-06 20:41   ` Rafael J. Wysocki
2023-12-06 20:41     ` Rafael J. Wysocki
2024-02-02  9:05     ` Beata Michalska
2024-02-02  9:05       ` Beata Michalska

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3e6077bb-907c-057f-0896-d0a5814a4229@nvidia.com \
    --to=sumitg@nvidia.com \
    --cc=beata.michalska@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=ionela.voinescu@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=sudeep.holla@arm.covm \
    --cc=viresh.kumar@linaro.org \
    --cc=will@kernel.org \
    --cc=yang@os.amperecomputing.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.